Skip to content

Commit 0247958

Browse files
committed
inline: bind storage for a parameter the body hands to a ref parameter
The splice substitutes the caller's argument expression for a parameter. That breaks when the body passes the parameter on to a REF parameter: the substituted value has no storage, so the inner call stops matching ("can't pass non-ref to ref"), and a program that compiled before the pass now does not. Record, per callee parameter, whether it is read as the argument of a ref parameter, and bind a temp for those instead of substituting - for the constant case as well as the computed one. A by-value parameter is a copy in the real call anyway, so the temp is what the un-inlined code did. Worse than a diagnostic when the substituted value is constant: the folder then evaluates the native with a value where the C++ signature wants `const T &` and dereferences storage that was never materialized, which segfaults the compiler: das::FoldingVisitor::evalAndFold -> Context::evalWithCatch -> <native> The added test covers that shape - a private (so budget-exempt) callee whose parameter goes to an implicit const-ref native, called with an expression built from const locals. It crashed the compiler before this change. Found in dagor's enlisted: traceray_normalized in shell_fx_common.das reported as no matching function after get_floor_material_name spliced.
1 parent dda0fe0 commit 0247958

2 files changed

Lines changed: 74 additions & 1 deletion

File tree

src/ast/ast_inline.cpp

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,10 @@ namespace das {
175175
struct ParamReadStats {
176176
das_hash_map<Variable *, int> readCount;
177177
das_hash_set<Variable *> readUnderLoop;
178+
// read as the argument of a REF parameter: such a read needs real storage, so the splice
179+
// must bind a temp rather than substitute a value. an implicit const-ref native parameter
180+
// is the case that bites - it cannot bind a bare value ("can't pass non-ref to ref")
181+
das_hash_set<Variable *> readAsRefArg;
178182
};
179183

180184
class ParamReadScan : public Visitor {
@@ -201,6 +205,22 @@ namespace das {
201205
if ( loopDepth ) stats.readUnderLoop.insert(expr->variable);
202206
}
203207
}
208+
void notePos ( ExprCallFunc * call ) {
209+
if ( !call->func ) return;
210+
for ( size_t i=0, is=call->arguments.size(); i!=is && i!=call->func->arguments.size(); ++i ) {
211+
auto & fa = call->func->arguments[i];
212+
if ( !fa->type || !fa->type->isRef() ) continue;
213+
Expression * a = call->arguments[i];
214+
if ( a && a->rtti_isR2V() ) a = static_cast<ExprRef2Value *>(a)->subexpr;
215+
if ( a && a->rtti_isVar() ) {
216+
auto v = static_cast<ExprVar *>(a)->variable;
217+
if ( v && params.find(v)!=params.end() ) stats.readAsRefArg.insert(v);
218+
}
219+
}
220+
}
221+
virtual void preVisit ( ExprCall * expr ) override { Visitor::preVisit(expr); notePos(expr); }
222+
virtual void preVisit ( ExprOp1 * expr ) override { Visitor::preVisit(expr); notePos(expr); }
223+
virtual void preVisit ( ExprOp2 * expr ) override { Visitor::preVisit(expr); notePos(expr); }
204224
};
205225

206226
// ----- cross-module reference scan (splice gate) -----
@@ -2264,6 +2284,11 @@ namespace das {
22642284
// a mutable by-value param IS a local copy
22652285
makeArgTemp(A->clone(), false, false, A->type && !A->type->canCopy());
22662286
} else if ( leafConst ) {
2287+
// a constant substituted where the body hands the parameter to a ref parameter
2288+
// would stop matching, same as the computed case - bind storage for it
2289+
if ( stats.readAsRefArg.find(P)!=stats.readAsRefArg.end() ) {
2290+
makeArgTemp(A->clone(), true, false, false);
2291+
} else
22672292
// an enum constant re-resolves its enumeration by name - under a
22682293
// module-scope wrap that name lands in the wrong module; bind it out
22692294
if ( needScope && leafA->type && leafA->type->isEnumT() ) {
@@ -2311,7 +2336,13 @@ namespace das {
23112336
if ( rit != stats.readCount.end() ) reads = rit->second;
23122337
bool underLoop = stats.readUnderLoop.find(P)!=stats.readUnderLoop.end();
23132338
bool orderSafe = writeFree || argReadsOnlyPrivateLocals(A, sa);
2314-
if ( reads<=1 && !underLoop && inlinePure(A) && orderSafe && !needScope ) {
2339+
// a parameter the body hands to a REF parameter needs real storage: substituting
2340+
// a value there stops matching ("can't pass non-ref to ref"). a by-value param is
2341+
// a copy in the real call anyway, so binding the temp is what the un-inlined code
2342+
// did
2343+
bool needsStorage = stats.readAsRefArg.find(P)!=stats.readAsRefArg.end()
2344+
&& !(A->type && A->type->ref);
2345+
if ( reads<=1 && !underLoop && inlinePure(A) && orderSafe && !needScope && !needsStorage ) {
23152346
sub.substitute = A;
23162347
} else {
23172348
makeArgTemp(A->clone(), true, false, A->type && !A->type->canCopy());

tests/language/inline_ref_arg.das

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
options gen2
2+
options auto_inline_functions = true
3+
require dastest/testing_boost
4+
require UnitTest
5+
6+
// REPRO (crashes the compiler before the fix, see below).
7+
//
8+
// doubleSamplePoint3 is bound from C++ as `Point3 doubleSamplePoint3(const Point3 &)`, so its
9+
// parameter is an implicit const ref.
10+
//
11+
// `private` + referenced once makes this callee budget-exempt, so it splices even though the body is
12+
// over the heuristic budget. The splice substitutes the caller's argument EXPRESSION for `p`, and the
13+
// caller passes `a + b` built from two const locals - which constant-folds. So the folder ends up
14+
// evaluating doubleSamplePoint3(<constant>) at compile time and dereferences a `const Point3 &` that
15+
// was never materialized:
16+
//
17+
// CRASH: SIGSEGV
18+
// das::FoldingVisitor::evalAndFold -> das::Context::evalWithCatch -> doubleSamplePoint3
19+
//
20+
// Same family as fold_extern_at_compile.das - the compiler must not execute a C++-bound function - but
21+
// this one crashes rather than baking a wrong constant, so it is the sharper case. Dagor closes it by
22+
// refusing to fold builtIn (C++-bound) calls at all.
23+
//
24+
// A non-constant argument (two globals instead of const locals) does NOT crash: the value binds and
25+
// runs fine. So the trigger is specifically inlining + constant propagation + a native const-ref param.
26+
def private big_callee(p : float3; k : float) : float3 {
27+
var acc = float3()
28+
var i = 0
29+
while (i < 2) { acc += float3(float(i)); i++ }
30+
let scaled = acc * k
31+
let doubled = UnitTest::doubleSamplePoint3(p)
32+
var out = doubled + scaled
33+
out.x += 0.
34+
return out
35+
}
36+
37+
[test]
38+
def test_no_compile_time_call_of_native_const_ref(t : T?) {
39+
let a = float3(1, 2, 3)
40+
let b = float3(1, 0, 0)
41+
t |> equal(big_callee(a + b, 0.), float3(4, 4, 6))
42+
}

0 commit comments

Comments
 (0)