Keep stack traces out of compiler-owned declarations - #1305
Conversation
The Jass body of a keyed table is built on a Table, whose instances come from a finite pool, so a keyed set that is cleared or discarded would burn one permanently - clearing cannot empty the table in place because that needs pairs(), which desyncs a lockstep game. Lua has a collector and nothing to free, so the stub is empty, but the call still has to lower there: left alone, the Jass body would try to destroy a Table that does not exist on that backend.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb4346f97c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A native stub is an analysis barrier, so the inliner would not cross it and every clear and every destroy kept a call doing no work - and clearing is not rare: a spatial index reusing its sets clears them as often as it refills them. Emptying the function leaves an ordinary one, which the inliner removes along with the call, while the argument is still evaluated.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Stack-trace injection appends a parameter to every affected function, and on Lua that is every non-native function. The keyed-table operations are recognised by their exact IM signature, so once the injector had run nothing matched and the lowering quietly did not happen: the Jass bodies survived onto Lua, where wurstKeyOf is never lowered and answers with its placeholder, so every element shared one key and a set claimed to contain everything it was asked about. Nothing reported it, and a release build emits stack traces by default, so this was the common case rather than an exotic one. The Jass side was never affected - JassKeyOfLowering already runs before the injector. The lowering moves into its own pass, run before injection. The test harness gains a stacktraces() toggle for the Lua path, which had no way to emit them and so could not have caught this.
Frotty
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fbca014e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Freeing a keyed table means nothing on Lua, so the lowering empties it, but it stayed an ordinary function and stack-trace injection then put the cost back: a trace argument at every call site, and a push and pop around a body that does nothing. That bookkeeping is also what stopped the inliner removing the call, so the operation was free only in a build without stack traces - which a release build is not. There is nothing to trace in an empty body, so the injector skips it. The regression now emits stack traces, since without them it could not have seen this.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed6cb20024
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Emptying the function only makes the call removable, not removed: inlining runs only under -inline, and even then the Lua register budget can refuse a caller, so a call to a function that means nothing on Lua survived into an ordinary build. The call sites are rewritten directly now. Arguments move into a statement expression so anything they do still happens, which is the same shape UselessFunctionCallsRemover uses to drop a call it does not need. The regression drops -inline, so it fails if the call ever depends on it again.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Three separate defects on this branch had one cause: stack trace injection rewrites every non-native function on Lua, and the lowerings identify a compiler-owned declaration by its exact signature. An instrumented one carries an extra parameter, so it stops being recognised and its lowering silently does not happen - which is how a keyed set kept its Jass body on Lua, where the key projection is never lowered and every element ended up sharing a key. Fixed where the mutation happens rather than at each thing it broke: the injector now leaves any @compilerintrinsic declaration alone, on both backends. A frame for one says nothing about where a program went wrong anyway, since its body is plumbing or a placeholder a lowering replaces. The rule is also self-enforcing. The injector records the parameter count of every compiler-owned declaration before it runs and fails with a readable error if one differs afterwards, so the next thing to break this cannot break it quietly.
Frotty
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24c4f73da8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The affected set is seeded from the functions which ask for a stack trace before any filtering happens, so filtering what the traversal adds missed an intrinsic seeded that way. Jass removed those and Lua did not, and on Lua the signature check then failed the build rather than letting the lowering quietly not happen - either way -lua -stacktraces was broken for that input. The removal now happens once, after both branches.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Three defects with one cause, plus the operation the branch set out to add.
The cause
Stack-trace injection appends a parameter to every affected function, and on Lua every non-native function is affected. The lowerings identify a compiler-owned declaration by its exact IM signature, so an instrumented one carries an extra parameter, stops being recognised, and its lowering silently does not happen.
That shipped as a correctness bug.
keyedTableContains(int, T)became three parameters, nothing matched, and the Jass body survived onto Lua:wurstKeyOfis never lowered on Lua, so it answers with its placeholder — the literal0above. Every element shares one key, and a keyed set reports that it contains anything it is asked about. A release build emits stack traces by default, so this was the common case.It was found by building a benchmark map with
grilland reading the emitted Lua — the first time this code had been through a normal release build rather than the test harness.The fix
The injector now leaves any
@compilerintrinsicdeclaration alone, on both backends. A frame for one says nothing about where a program went wrong: its body is plumbing, or a placeholder that a lowering replaces.The rule is self-enforcing. The injector records the parameter count of every compiler-owned declaration before it runs and fails with a readable error if one differs afterwards. The failure it guards against is silent by nature — the lowering simply does not fire and the unlowered body ships — so the next thing to break this cannot break it quietly.
Two narrower changes from earlier rounds stay, because they are right independently of the injector:
keyedTableDestroycall sites are rewritten away rather than left for the inliner — inlining runs only under-inline, and even then the Lua register budget can refuse a caller. Arguments move into a statement expression so their side effects still happen, the shapeUselessFunctionCallsRemoveralready uses.The operation itself
The Jass body of a keyed table is built on a
Table, whose instances come from a finite pool.clear()has to replace the table rather than empty it — emptying a Lua table needspairs(), whose order follows internal hash layout and so differs between clients, desyncing a lockstep game. Without a destroy operation, everyclear()and every discarded set burns a slot permanently. On Lua there is nothing to free, and now nothing is emitted either.Tests
The harness had no way to emit stack traces on the Lua path — which is why none of this was caught.
TestConfiggainsstacktraces(), and:stackTracesLeaveCompilerOwnedDeclarationsAlone— the general rule, using an intrinsic no lowering touches, so it is not about keyed tables. Asserts no trace parameter and no stack bookkeeping, and that the surrounding program is instrumented, so it cannot pass by stack traces being off.keyedTableStaysNativeWithStackTraces— membership still lowers under-stacktraces; the caller does not reach the hashtable natives.keyedTableDestroyCostsNothingOnLua— runs withstacktraces()and withoutinline(), so it fails if the call ever depends on the optimiser again.keyedTableDestroyoccurs zero times in the emitted script.Gate: 631 tests, 0 failures — LuaTranslation, LuaBackendAudit, Optimizer, KeyedTable, LuaNatives, Closure, Interpreter, Classes, StdLibOwn, and FieldIteration, which covers the other compiler intrinsics the exclusion now applies to.