Skip to content

Lower and lift without crashing, sret included - #2

Closed
ZweZeya wants to merge 3 commits into
mainfrom
fix/lift-lower-crashes
Closed

ZweZeya wants to merge 3 commits into
mainfrom
fix/lift-lower-crashes

Conversation

@ZweZeya

@ZweZeya ZweZeya commented Oct 18, 2026 •

Copy link
Copy Markdown
Collaborator

First of a stack of seven. Each PR targets the one below it, so each diff is only its own change.

What this changes

Lowering and lifting no longer crash on input they did not expect.

  • Lowering dereferenced fallback_impl and the source type unconditionally, though both are optional and every ownership-analysis test builds ops without them. It now names the op and the missing attribute.
  • Lifting read operands by fixed index and called getResult() on try_pop, which segfaults where the result comes back through a hidden llvm.sret argument. wtc.queue_pop gains an optional result_slot operand (its result becomes optional; exactly one of the two is present), and lowering puts the slot back where the callee marks it, with the callee's argument attributes, sret included.
  • A pop's element type is recovered from std::shared_ptr<T>'s layout as the first pointer reached through its leading members: libc++ puts T* first, libstdc++ puts it first in the std::__shared_ptr base. CI on Linux found the second case (separate commit, LiftCIRToWTC/libstdcxx-shared-ptr.mlir).
  • Measured at the pin: the hidden argument depends on the target. On AArch64 macOS, try_pop and spawn return their records directly and the old indices happen to work; the same file emitted for x86-64 puts the slot first. docs/audit.md records this.

How it was checked

16 of 16 pass, no warnings. The five new tests (LiftCIRToWTC/libstdcxx-shared-ptr, LowerWTCToCIR/missing-attributes, LowerWTCToCIR/push-callee-returns-value, LiftCIRToWTC/sret-try-pop, LiftCIRToWTC/arity-mismatch) fail against main: three crash, one refuses the libstdc++ shape, one builds a call whose result disagrees with its callee. The prototype's example lifts and lowers back for both targets.

Checklist

  • CI is green
  • New behaviour has a test; a fixed bug has a test that failed before
  • No file moves or renames mixed in with substantive changes
  • The pinned LLVM revision is untouched, or this PR does nothing else

🤖 Generated with Claude Code

ZweZeya and others added 2 commits October 18, 2026 12:27
fallback_impl and the source type are optional on all four wtc ops, and
every ownership-analysis test builds ops without them, yet each lowering
pattern dereferenced both unconditionally. The pass now checks first and
names the op and the missing attribute; the patterns also refuse rather
than dereference, in case anything reaches them another way.

The push lowering now builds its call with the callee's return type. The
op has no result, but a substituted implementation may report whether the
push succeeded, and a call whose result count disagrees with its callee
does not verify.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Where the target returns a non-trivial record indirectly, try_pop's call
has no result and a hidden first argument, and the queue is the second.
Lifting read operands by fixed index and called getResult() on that call,
which segfaulted on the prototype's own example.

The callee marks the hidden argument llvm.sret, so lifting now reads the
queue from the argument after it and lifts to a wtc.queue_pop that writes
through the slot: queue_pop gains an optional result_slot operand and its
result becomes optional, exactly one of the two being present. Lowering
puts the slot back where the callee marks it and gives every rebuilt call
its callee's argument attributes; a call that drops sret no longer
matches the ABI of the function it calls.

Every call's shape is now checked before anything is read from it, and
anything unexpected is named rather than cast blindly.

Measured at the pin: whether the hidden argument exists depends on the
target. On this AArch64 macOS host, try_pop and spawn return their records
directly and the old indices happen to be right; the same file emitted for
x86-64 puts the slot first. The audit now says so.

Verified: 15 of 15 pass, no warnings. The four new tests fail against dev
(three crash, one builds a call whose result disagrees with its callee).
The prototype's example lifts and lowers back for both targets.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ZweZeya
ZweZeya requested a review from apaznikov as a code owner October 18, 2026 05:15
@ZweZeya ZweZeya mentioned this pull request Oct 18, 2026
4 tasks done
CI on Linux refused every pop in the end-to-end tests: "cannot recover the
queue's element type". Lifting took T* to be std::shared_ptr<T>'s first
member, which it is under libc++, the only library these had been run
against. Under libstdc++ that member is the std::__shared_ptr base, and
T* is its first member.

Lifting now looks through leading records until it reaches a pointer. It
is still an assumption about layout, and the audit says so.

Verified: 16 of 16 pass. The new test, reduced from the libstdc++ shape,
fails without the change with the error CI reported.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@apaznikov

Copy link
Copy Markdown
Collaborator

Closing this as superseded by #10, which carries a second line of work on the same slice that had been developed alongside this stack and goes further in the same direction.

What this PR fixes is covered there: lifting reads operands through the calling convention, so the hidden return slot of try_pop is handled on every target (mlir: Prove queue ownership and specialise on it), and lowering an operation without a fallback reports an error instead of crashing. The element type no longer needs recovering from shared_ptr's layout, because on that branch !wtc.queue<T> carries the queue object's type, not the element's. Thank you for finding the x86-64 shape by hand; it is why the sret handling there is exercised from real compiler output.

@apaznikov apaznikov closed this Oct 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants