Start the P4 ownership torture suite and fix the two leaks it found - #14
Conversation
Three factual updates to the CI and ownership sections, no reassessment of existing checkboxes beyond one note. P0 CI matrix gains the SIGILL fix: the backend selected a target CPU by name and passed an empty feature string, so LLVM enabled everything the CPUID model implied without checking the machine exposed it. The entry records that the symptom looked intermittent but was deterministic, and that an environmental trigger means a fleet rotation cannot be excluded as the reason the runs went green. The both-gates item stays open but now says where it actually stands: the hardening gate is green in both parallel runs without rerun, and only macos-smoke holds the CI gate. P4 names the single remaining ownership-suite failure and that it is macOS-only, and P0 ownership records the std.collections copy-on-grow leak that was found and fixed in an area whose items were already checked off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
TypeNeedsCleanup looked for the member key "destroy()", but the methods map is keyed by CallableKey, which joins the bare name with "$"-separated parameter types and so stores "destroy". The literal was a key the map could never hold, leaving that branch dead: a type whose only resource was its own destroy() was classified as needing no cleanup, and the hook never ran. The analyzer's TypeOwnsResources matches the same literal correctly, because analyzer member maps really are keyed that way. Agreement between the two spellings is what hid this. Scope measured against the built compiler. A managed class with destroy() was already cleaned up, and so was a struct that additionally owned a managed field, because the field recursion decided those cases before the dead branch was reached. Only a struct whose sole resource was destroy() was skipped, and it was skipped everywhere: as a local at scope exit and as a field of a class being deleted, whose synthesized destructor was emitted with an empty body. tests/native-handle-wrapper.abs is the visible casualty. NativeHandle holds a raw void*, which is explicitly not a resource, so its destroy() never ran and every handle leaked. The test asserts tags before destruction and never that the release happened, so it passed throughout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
Covers three P4 items that had no dedicated regression test: destroying an owner expires subscribers taken at any depth and weak aliases from either place, a weak back-edge neither keeps its target alive nor survives it, and generation-slot reuse never revives an expired handle across 500 rounds. Cleanup order is observed rather than inferred, using the digit-trace idiom from tests/defer.abs: each destroy() appends its tag to a shared int32, so the final value spells the order storage was released in. This is what exposed the destroy() defect fixed in the previous commit — the trace came back 0. Reverse declaration order is asserted exactly (321) for fields of one object, which is what docs/resource-ownership.md specifies. For a chain through a managed field the test asserts only that every level is released exactly once: the observed order is root-first (123), not the documented reverse, and pinning an interleaving that cannot be justified as intended would bake in behaviour that may itself be wrong. The discrepancy is noted in the test. Registered four ways, matching the existing ownership tests: semantic, IR emission, lli execution, and an AddressSanitizer build that must exit clean, which is a first step on the P4 item asking for the corpus under sanitizers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
The test observed the wrapper's tag before cleanup and described the release in a comment: "destroy() runs when a leaves this scope." Nothing checked it, so when destroy() silently stopped being called the test kept passing and the handle leaked on every run. The C side now counts live handles, and the Absolute side asserts the count: zero before the test, one while a handle is open, zero once its owner leaves scope, and one after a move, which also pins that a move transfers the handle rather than duplicating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
A constructor leaving through an exception ran scope cleanup for its locals and returned. The fields it had already stored into `this` are not a scope, and no caller holds a name for them, so every resource acquired before the throw leaked. Measured on four shapes before the change: a throw after one field leaked one resource, after two fields two, a derived constructor throwing after a completed base leaked both, and a throwing base leaked what it had acquired. Ordinary scope unwinding was already correct. The exception path out of a constructor now releases the object itself. Two properties make that exactly right rather than approximately. Object storage is zero-initialized before the constructor runs, so destroying the whole object releases precisely the initialized prefix — uninitialized fields are null or zero and their cleanup is a no-op. And a derived class's field list starts as a copy of its parent's, so one destructor covers inherited fields too, which is what the throwing-base and throwing-derived cases need. Cleanup zeroes each field as it releases it, so a later destructor pass over the same object does nothing. That keeps this idempotent against whatever the allocation site does on failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
Adds the P4 item for partially constructed objects and exceptions in constructor and base constructor, which nothing in the suite reached: no existing test throws from a constructor at all. Five shapes, each counting live resources through a raw int32 the caller owns so a leak fails an assertion instead of waiting for a sanitizer: a throw after one field, after two, in a derived constructor once its base completed, in the base itself, and a throw crossing two live scopes. The last one passed before the fix in the previous commit and is kept as the control — it is what shows the defect was specific to constructors rather than to unwinding in general. Registered semantic, IR, lli and AddressSanitizer, matching the graph torture test added earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
Adds the P4 item for move(owner) through return values, parameters, fields, generic wrappers and interface dispatch, plus the case the item singles out: an exception that lands after ownership has been taken but before the operation that took it finished. At that point the source is already invalidated and the destination does not exist, so the frame the exception leaves is the only place that can still release the object. Six shapes, counting live objects through a raw int32 the caller owns so a leak fails an assertion rather than waiting for a sanitizer. The interface case checks that deleting through an interface pointer reaches the most-derived destructor via slot zero rather than stopping at the interface. All six pass unchanged, so unlike the two defects the earlier torture tests found, transfer needed no compiler fix — the scope unwinding that already worked covers the throw-after-take case once a constructor no longer leaks. Registered semantic, IR, lli and AddressSanitizer, matching the other torture tests. Full suite 516/516. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
![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: 10cd0a0723
ℹ️ 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".
| if (!currentConstructorClass.empty() && currentThis) { | ||
| if (const auto found = classes.find(currentConstructorClass); | ||
| found != classes.end() && TypeNeedsCleanup(currentConstructorClass)) { | ||
| builder.CreateCall(DeclareClassDestructor(found->second), {currentThis}); |
There was a problem hiding this comment.
When a base constructor throws, its own exception path reaches this call and destroys its initialized fields; control then returns to the derived constructor's EmitExceptionCheck() at codegen_module.cpp:873, which reaches this call again with currentConstructorClass set to the derived class. Because derived field layouts include the base fields, the base resources and inherited user destroy() hook run twice, causing double-free or duplicate side effects whenever that cleanup is not idempotent.
Useful? React with 👍 / 👎.
| if (!currentConstructorClass.empty() && currentThis) { | ||
| if (const auto found = classes.find(currentConstructorClass); | ||
| found != classes.end() && TypeNeedsCleanup(currentConstructorClass)) { | ||
| builder.CreateCall(DeclareClassDestructor(found->second), {currentThis}); |
There was a problem hiding this comment.
For a class with a user-defined destroy(), DeclareClassDestructor invokes that hook before cleaning fields, so a constructor that throws before establishing its invariants now executes arbitrary cleanup code on a partially initialized object. Zero-initialization only makes generated field cleanup safe; it does not make a hook that dereferences a field or passes a not-yet-acquired native handle to an FFI release function safe, which can turn an ordinary constructor failure into a crash or invalid external operation.
Useful? React with 👍 / 👎.
| const auto oldSubstitutions = currentGenericSubstitutions; | ||
| currentGenericSubstitutions = info.substitutions; | ||
| currentClassName = info.name; | ||
| currentConstructorClass = info.name; |
There was a problem hiding this comment.
This state remains set while a lambda expression in the constructor is emitted; Visit(LambdaExpr) saves and replaces currentClassName and currentThis but never saves or clears currentConstructorClass. If such a lambda captures this and throws, its generated exception path therefore calls the enclosing class destructor, even when the lambda is invoked after construction, prematurely releasing the object and allowing its eventual normal cleanup to run again.
Useful? React with 👍 / 👎.
Review of the previous constructor-unwinding commit raised three defects in it, all real. This addresses them and adds the coverage that should have caught them. The exception path called the whole class destructor, and that destructor runs the type's own destroy() hook before touching fields. A constructor that threw never established the invariants such a hook assumes, so this executed user cleanup on an object that was never built — the zero-initialization argument justifies generated field cleanup only, never an arbitrary hook body. Fields are now released directly and the hook is not called. The same call also ran twice when a base constructor threw: the base cleaned its own fields on its own path, then the derived frame reached the exception check after the base call and cleaned them again through its flattened field list. Rather than rely on cleanup being idempotent, currentConstructorClass now stays empty until the base constructor has succeeded. Before that point no field of the derived class is initialized, so there is nothing for that frame to release. Third, the flag leaked into lambda bodies emitted inside a constructor. Those are separate functions that can run long after construction, and Visit(LambdaExpr) saved currentClassName and currentThis but not this one, so a throwing lambda would have released the enclosing object. It is now saved and cleared like the rest of the constructor state. Three cases added, each counting hook invocations through a caller-owned raw int32 with a deliberately non-idempotent hook so a second call is visible: a throw in a hooked constructor, a derived throw over a hooked base, and a hooked base that throws. Verified against the previous implementation, where they abort as expected. Separately, the clean sanitizer harness asked for detect_leaks on macOS, where LeakSanitizer does not exist. The runtime then fails at startup and abort_on_error turns that into a non-zero exit, which is why every run-sanitizer-clean case fails there — including run-sanitizer-ownership-stability, long read as an ownership defect. Darwin now joins the Windows and Termux guard. Unverified without a Mac; CI decides. Full suite 516/516 on Linux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SPqntJmyXNSmZNzuQWj5QS
|
All three review findings were correct and are fixed in Running the user hook on an incomplete object. Correct, and it exposed a hole in my own reasoning. I justified reusing the class destructor by pointing at zero-initialized storage — but that argument only covers generated field cleanup. It says nothing about an arbitrary Double cleanup when a base constructor throws. Also correct. My defence was that cleanup is idempotent, and for generated field cleanup it is — fields are zeroed as they go. But a struct field with a user State leaking into lambdas. Confirmed at My original tests missed all three because no class in them had a An unrelated finding in the same run
The harness asks for If that is right, then Generated by Claude Code |
2 participants
Works the
P4 — ownership torture suitesection of TODO.md, which had all eleven items open and nothing written against it. Six are now covered, and writing the coverage turned up two real leaks in the compiler.The two defects
A user
destroy()was never called when it was the only reason to clean up.TypeNeedsCleanuplooked for the key"destroy()", but the method map is keyed byCallableKey("destroy", {}), which produces"destroy"— a key the map can never hold, so the branch was dead code. The analyzer's copy of the same test is correct because its member map uses a different key format, which is why nothing caught the divergence.The boundary is exact: a managed class with
destroy()was cleaned up correctly, and so was a struct that additionally owned a managed field — only a struct whose sole claim to cleanup was its owndestroy()was skipped.tests/native-handle-wrapper.abssits precisely in that gap:NativeHandleholds oneraw void*, and raw pointers are explicitly not resources. It leaked every handle. The test passed anyway because it asserted nothing about destruction; the comment "destroy() runs when a leaves this scope" was simply untrue. It now counts live handles on the C side and asserts the release.A constructor that threw leaked every field it had already initialized. Scope unwinding worked, but the fields of
thisare not a scope and no caller has a name for them yet. Four shapes all leaked: a throw after one field, after two, in a derived constructor once its base completed, and in the base itself.The fix releases the object on the constructor's exception path, resting on two properties I verified rather than assumed: object storage is zero-initialized before the constructor runs, so destroying it releases exactly the initialized prefix while uninitialized fields are null and cost a no-op; and a derived class's field list is flattened (
info.fields = parent.fields), so its destructor covers base fields. Cleanup zeroes each field as it goes, so a later destructor pass stays idempotent and cannot double-free.Coverage added
ownership-torture-graphownership-torture-constructionownership-torture-transfermove(owner)through return, parameters, fields, generic wrappers, interface dispatch, and a throw after ownership was takenEach is registered four ways — semantic, IR,
lli, and AddressSanitizer — which also starts the P4 item about running the corpus under sanitizers. Leaks fail an assertion by counting live resources through a caller-ownedraw int32, rather than waiting for a sanitizer to notice.The transfer test passed unchanged, which is a result rather than a gap: it shows the throw-after-take case is already covered by the scope unwinding that worked all along, once constructors stop leaking.
One thing deliberately not pinned
docs/resource-ownership.mdsays fields are destroyed in reverse declaration order. That holds within one object — the three-probe test traces321— but a chainroot → child → grandchildthrough a managed field unwinds root-first, tracing123. I could not ground that order as intentional, so the test asserts what is unambiguous, that every level is released exactly once, and neither enshrines nor hides the discrepancy. Either the codegen order or the document needs a decision.Verification
Full suite 516/516 locally, up from 501, including the AddressSanitizer runs. Both compiler changes were run against the whole suite separately; the constructor change is the riskier of the two since it adds destructor calls on every constructor's exception path, and it broke nothing.
CI has not seen this batch yet.
macos-smokewill fail with its single pre-existingrun-sanitizer-ownership-stabilitytest, so the required gate cannot go green here regardless.Generated by Claude Code