Skip to content

Commit dda0fe0

Browse files
committed
inline: do not auto-inline a body that reads a parameter inside a
can_shadow block A can_shadow block argument is one the shadowing check deliberately lets cover an outer name of the same spelling. Hosts set it on generated query blocks (dagor's ecs marks every [es] component argument), and daslib/linq_boost sets it on the blocks it generates. Splicing a callee that reads a parameter inside such a block is unsafe. The parameter becomes the caller's argument expression, and when that expression names a variable the block also declares, it rebinds to the block's variable. Nothing reports it, because can_shadow is exactly what switches error 30701 off, so the program is silently wrong rather than rejected: def callee(squad_team : int) return apply_team() <| $ [shadowable] (team : int) return squad_team * 100 + team def caller(team : int) return callee(team) // 505, should be 705 The 505 is the block's own team (5) used for both operands. In dagor's enlisted this turned `squad_team != team` into `team != team`; that one happened to trip -Wtautological-compare in the AOT output, but a collision between two different variables compiles clean and just computes the wrong value. Keep those callees out of the heuristic tier. An explicit [inline] still splices - that contract is the author's call. The added test fails with 505 before the change.
1 parent 3f4e6d4 commit dda0fe0

3 files changed

Lines changed: 69 additions & 0 deletions

File tree

src/ast/ast_inline.cpp

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,32 @@ namespace das {
565565
// shapes: to_array_move's heap-mode array literal spliced into a generated
566566
// `let <-` re-infers under the in-place make-local protocol and corrupts
567567
// the array header (probe-verified: garbage sums, SIGSEGV)
568+
// a can_shadow block argument is one the shadowing check deliberately lets cover an outer
569+
// name of the same spelling - host query blocks set it on every component argument (dagor's
570+
// ecs does), and daslib/linq_boost sets it on its generated blocks. Splicing a body that
571+
// READS A PARAMETER inside such a block is unsafe: the parameter becomes the caller's
572+
// argument expression, and if that expression names a variable the block also declares, it
573+
// rebinds to the block's variable. Nothing diagnoses it, because can_shadow is exactly what
574+
// switches the shadowing error off, so the result is a silently wrong program - dagor hit
575+
// `squad_team != team` emitted as `team != team`. Keep such callees out of the auto tier;
576+
// an explicit [inline] contract still splices, that is the author's call.
577+
bool captureHazardFn ( Function * fn ) {
578+
bool hazard = false;
579+
lookupExpressions(fn->body, [&](Expression * e) {
580+
if ( hazard || !e->rtti_isBlock() ) return;
581+
auto blk = static_cast<ExprBlock *>(e);
582+
bool anyCanShadow = false;
583+
for ( auto & a : blk->arguments ) {
584+
if ( a->can_shadow ) { anyCanShadow = true; break; }
585+
}
586+
if ( !anyCanShadow ) return;
587+
lookupExpressions(blk, [&](Expression * inner) {
588+
if ( inner->rtti_isVar() && static_cast<ExprVar *>(inner)->argument ) hazard = true;
589+
});
590+
});
591+
return hazard;
592+
}
593+
568594
bool autoEligibleFn ( Function * fn, const AutoInlineCfg & cfg ) {
569595
if ( !fn || fn->mustInline || fn->builtIn ) return false;
570596
if ( !fn->body || fn->isTemplate ) return false;
@@ -582,6 +608,7 @@ namespace das {
582608
// module's visible-function set. Cross-module inlining is the author's
583609
// explicit [inline] contract, not a heuristic's call
584610
if ( fn->module != cfg.thisModule ) return false;
611+
if ( captureHazardFn(fn) ) return false;
585612
return worthAutoInline(fn, cfg);
586613
}
587614

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
options gen2
2+
options indenting = 4
3+
module _inline_shadow_helper public
4+
require daslib/ast public
5+
require daslib/ast_boost
6+
7+
// Marks the annotated block's arguments can_shadow, which is what a host ECS does for query blocks
8+
// (dagor: gameLibs/ecs/scripts/das/das_es.cpp sets arg->can_shadow on every [es] block argument) and
9+
// what daslib/linq_boost does for its generated blocks.
10+
[block_macro(name="shadowable")]
11+
class ShadowableBlock : AstBlockAnnotation {
12+
def override apply(var blk : ExprBlock?; var group : ModuleGroup;
13+
args : AnnotationArgumentList; var errors : das_string) : bool {
14+
for (a in blk.arguments) {
15+
a.flags.can_shadow = true
16+
}
17+
return true
18+
}
19+
def override finish(var blk : ExprBlock?; var group : ModuleGroup;
20+
args, progArgs : AnnotationArgumentList; var errors : das_string) : bool {
21+
return true
22+
}
23+
}
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
options gen2
2+
options auto_inline_functions = true
3+
require dastest/testing_boost
4+
require _inline_shadow_helper
5+
6+
[never_inline] def apply_team(blk : block<(team : int) : int>) : int { return invoke(blk, 5) }
7+
8+
def callee(squad_team : int) : int {
9+
return apply_team() <| $ [shadowable] (team : int) : int {
10+
return squad_team * 100 + team
11+
}
12+
}
13+
14+
def caller(team : int) : int { return callee(team) }
15+
16+
[test]
17+
def test_no_capture_when_inlined(t : T?) {
18+
t |> equal(caller(7), 705)
19+
}

0 commit comments

Comments
 (0)