midhuncodes7 wrote:
@efriedma-quic @nikic @antoniofrighetto
Per the discussion above, I've reworked this to drop the
`contains_returns_twice_call` attribute entirely and instead cache the check
locally on `EarliestEscapeAnalysis: containsReturnsTwiceCall()` lazily computes
and memoizes `Function::callsFunctionThatReturnsTwice()` once per
`EarliestEscapeAnalysis` instance, replacing the
`hasFnAttribute(Attribute::ContainsReturnsTwiceCall)` check in
`getCapturesBefore`.
While testing this, two existing Clang tests started failing:
```
- Clang :: CodeGenObjC/exceptions.m
- Clang :: CodeGenObjC/synchronized.m
```
Both exercise the Darwin "fragile" ObjC exception model, where
`@try/@synchronized` lower to a hand-built `_setjmp` call in `CGObjCMac.cpp`
via EmitNounwindRuntimeCall + SetJmpResult->`setCanReturnTwice()`, bypassing
`CodeGenFunction::EmitCall` entirely. That matters here under the old
attribute-based approach, Clang only ever set `contains_returns_twice_call`
from inside `EmitCall`, so it was never set on these ObjC functions, and
BasicAA never engaged its returns-twice handling for them. With the new
scan-based check, `callsFunctionThatReturnsTwice()` finds this `_setjmp` call
directly from its IR-level `returns_twice` attribute (set via
`setCanReturnTwice()`), regardless of how it was emitted, so BasicAA now
correctly treats locals as capturable across it.
Concretely, in `exceptions.m`'s f2(), the test comment for the failing check
even documents the assumption being invalidated ("Landing pad. Note that we
elide the re-enter.") the old codegen forwarded `x++` to a plain `store i32 6,
ptr %x` right after the landing pad, which is only sound if `%x` isn't visibly
mutated via a `longjmp` driven re-entry at the `_setjmp` which is the exact
assumption issue #[198967](https://github.com/llvm/llvm-project/issues/198967)
is about, just reached through ObjC's exception lowering instead of libc
`setjmp`/`longjmp` directly.
So this looks like a real instance of the same bug class, now correctly caught
by the more thorough scan-based check rather than a regression to work around.
My inclination is that the right fix is to update the CHECK lines in both tests
to match the new, more conservative codegen but before I do that, I'd like a
second opinion: is that the right call here, or is there a preference for
handling ObjC's exception-lowering `setjmp` differently (e.g., should
`CGObjCMac.cpp`'s hand built call route through the normal attribute machinery
instead, for consistency with source-level `setjmp` calls)?
https://github.com/llvm/llvm-project/pull/212297
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
