Repository navigation
Refuse a destroy through a stale reference on Lua - #1593
Conversation
Lua hands out the most recently freed id from one id space for every class, so a destroy through a stale reference frees whatever object holds that id now, of any class, and the next allocation shares it with the live one. The deallocator now receives the class the object is destroyed as and raises before freeing an object of another class. It records the destroy first, and the new staleDestroyReport intrinsic lets a map read the record (count|class|destroyed as) after the raising thread has ended.
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. |
The deallocator ran after the destructor, and a destroy through dispatch picked the live replacement's destructor first, so a stale destroy still ran another object's ondestroy before it was refused. Each destroy on Lua now evaluates its receiver once and checks it against the type the caller destroys it as, before the destroy call. A failure records count|id|class|destroyed as|file:line and raises; a free id goes on to the deallocator's double-free error as before.
|
In-game regression, before and after this change. Setup
Verdict: both runs still report a verdict problem from the fixture's caught-error accounting. The baseline reports Artifacts: |
The site was an IM literal, so the incremental cache kept the old line after an edit moved the destroy (IncrementalHotReloadCompilationTests). It is now taken from the call's trace in the Lua translation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53dde2827d
ℹ️ 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 parameterized instanceof observes an instantiation's identity and makes EliminateGenerics specialise for it; with a recursive generic class (Chain<Chain<T>> destroyed in ondestroy) the worklist never finished. The check names the erased class, which every instantiation is an instance of.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
Why
On Lua every class shares one id space, and an allocation hands out the most recently freed id. A destroy through a stale reference therefore runs the destructor of whatever object holds that id now, of any class, frees it while it is in use, and the next allocation shares the id. WurstStdlib2 #522 (
StdlibIngameAllocatorTests.testStaleDestroyPreservesCodeList, commit 6cd0011) shows this in the real game: a stale owner reference and a liveCodeListboth had id 8104, and after the stale destroyoriginal.run()called the replacement's callback.What
destroyon Lua evaluates its receiver once into a local and, if it is non-null and notinstanceofthe type the caller destroys it as, calls__wurst_staleDestroyinstead of destroying. The check runs before dispatch picks a destructor and before anyondestroy, so a refused destroy runs no destructor and frees nothing. It is emitted in IM, so devirtualisation and inlining keep it.count|id|actual class|destroyed as|file:lineand raisesStale destroy: object N is a X, destroyed as a Y at File.wurst:L. A map reads the record throughstaleDestroyReport() returns string, matched by declaration like the CodeList intrinsics. Its source body (Jass, older compilers) should return"".Not covered
Tests
LuaBackendAuditTests:aStaleDestroyOfAnotherClassIsAnErroraStaleDestroyFreesNothingAndIsReportedaStaleDestroyRunsNoDestructorand itsOptimisedvariant (inline + local optimisations), covering base-class, interface and generic-class references, a receiver evaluated once, and valid destroys through class, base class and interface each running their destructor exactly onceaDoubleDestroyKeepsItsErrordestroyThroughASuperclassReferenceIsNotStaleThe full suite passed locally except one hot-reload position test. That is fixed in this push by treating the site string as a stack position; CI is the check for it.
Not done yet: the stdlib in-game regression (#522) on the compiler before and after this change.