Author: Midhunesh Date: 2026-10-08T16:06:29+05:30 New Revision: e2a6fc57b2aeefaf86c3c2a59607148335fc58e4
URL: https://github.com/llvm/llvm-project/commit/e2a6fc57b2aeefaf86c3c2a59607148335fc58e4 DIFF: https://github.com/llvm/llvm-project/commit/e2a6fc57b2aeefaf86c3c2a59607148335fc58e4.diff LOG: [BasicAA] Fix miscompilation with setjmp/longjmp due to missing longjmp re-entry paths in alias analysis (#212297) Fixes [#198967](https://github.com/llvm/llvm-project/issues/198967). `EarliestEscapeAnalysis::getCapturesBefore` (used by GVN, DSE, MemCpyOpt) determines whether an object is captured before a given instruction using `isPotentiallyReachable()/isNotInCycle()`, both of which only see the forward CFG. In a function containing a `returns_twice` call (e.g. `setjmp`), a `longjmp` can re-enter at that call site — a back-edge invisible to both checks. This causes `BasicAA` to incorrectly return `NoAlias` for a local alloca whose address was captured on a branch that only runs before the `longjmp` re-entry. Downstream passes (GVN, DSE) then miscompile the function by eliminating stores that are still live on the re-entry path. Fix: `EarliestEscapeAnalysis` now caches whether its function contains a call that may return twice (`callsReturnsTwiceFn()`, backed by the existing `Function::callsFunctionThatReturnsTwice()` scan, memoized per instance). If so, `getCapturesBefore` conservatively treats the object as captured, ahead of both the cycle and reachability checks, since neither can see the `longjmp`-induced back edge. This was originally implemented as a new `contains_returns_twice_call` IR function attribute (set by Clang, propagated on inlining, threaded through bitcode), but per review discussion that was dropped in favor of the caching approach above: `EarliestEscapeAnalysis` is constructed in only a handful of places (GVN, DSE, MemCpyOpt), so scanning once per instance is cheap enough without the extra attribute plumbing (Attributes.td, bitcode encode/decode, Clang emission, inlining propagation, CodeExtractor handling). Trade-off: the check is function-wide — every identified-local object in a function containing any `returns_twice` call is treated conservatively, regardless of its position relative to the call. A per-site reachability check would be more precise but more expensive; the community preferred this simpler, coarser fix. This PR includes suggestions from AI which were carefully reviewed before adapting. The PR description is updated as per AI suggestion --------- Co-authored-by: Midhunesh <[email protected]> Added: llvm/test/Analysis/BasicAA/setjmp-longjmp.ll Modified: clang/test/CodeGenObjC/exceptions.m clang/test/CodeGenObjC/synchronized.m llvm/include/llvm/Analysis/AliasAnalysis.h llvm/lib/Analysis/BasicAliasAnalysis.cpp Removed: ################################################################################ diff --git a/clang/test/CodeGenObjC/exceptions.m b/clang/test/CodeGenObjC/exceptions.m index 66dc0515af03e..e8dd7ffaad55b 100644 --- a/clang/test/CodeGenObjC/exceptions.m +++ b/clang/test/CodeGenObjC/exceptions.m @@ -52,23 +52,29 @@ int f2(void) { // CHECK-NEXT: [[CAUGHT:%.*]] = icmp eq i32 [[SETJMP]], 0 // CHECK-NEXT: br i1 [[CAUGHT]] @try { - // Landing pad. Note that we elide the re-enter. + // Landing pad. // CHECK: call void asm sideeffect "", "=*m,=*m"(ptr nonnull elementtype(i32) [[X]] // CHECK-NEXT: call ptr @objc_exception_extract // CHECK-NEXT: [[T1:%.*]] = load i32, ptr [[X]] - // CHECK-NEXT: [[T2:%.*]] = add nsw i32 [[T1]], -1 - - // CHECK: store i32 6, ptr [[X]] + // CHECK-NEXT: [[DEC:%.*]] = add nsw i32 [[T1]], -1 + + // A `longjmp` may re-enter at the setjmp above, so [[X]] can no longer be + // assumed unmodified across it: the increment below is a real + // load/add/store instead of being folded to a constant store. + // CHECK: [[T2:%.*]] = load i32, ptr [[X]] + // CHECK-NEXT: [[INC:%.*]] = add nsw i32 [[T2]], 1 + // CHECK-NEXT: store i32 [[INC]], ptr [[X]] x++; // CHECK-NEXT: call void asm sideeffect "", "*m,*m"(ptr nonnull elementtype(i32) [[X]] // CHECK-NEXT: call void @foo() // CHECK-NEXT: call void @objc_exception_try_exit - // CHECK-NEXT: [[T:%.*]] = load i32, ptr [[X]] + // CHECK-NEXT: [[T3:%.*]] = load i32, ptr [[X]] foo(); } @catch (id) { x--; } + // CHECK: {{%.*}} = phi i32 [ [[DEC]], %{{.*}} ], [ [[T3]], %{{.*}} ] return x; } diff --git a/clang/test/CodeGenObjC/synchronized.m b/clang/test/CodeGenObjC/synchronized.m index 25feb958c73ab..7f72d87667eae 100644 --- a/clang/test/CodeGenObjC/synchronized.m +++ b/clang/test/CodeGenObjC/synchronized.m @@ -39,8 +39,9 @@ void foo(id a) { // CHECK: unreachable // CHECK: call void @objc_exception_try_exit - // CHECK-NEXT: call i32 @objc_sync_exit - // CHECK: ret void + // CHECK-NEXT: [[SYNCVAL:%.*]] = load ptr, ptr [[SYNC]] + // CHECK-NEXT: call i32 @objc_sync_exit(ptr [[SYNCVAL]]) + // CHECK-NEXT: ret void return; } @@ -48,16 +49,20 @@ void foo(id a) { // CHECK-LABEL: define{{.*}} i32 @f0( int f0(id a) { - // We can optimize the ret to a constant as we can figure out - // that x isn't stored to within the synchronized block. + // x isn't stored to within the synchronized block itself (x++ happens + // before the setjmp, while entering it), but BasicAA conservatively + // treats any local as capturable across a `returns_twice` call anywhere + // in the function, so the ret can no longer be folded to a constant. // CHECK: [[X:%.*]] = alloca i32 // CHECK: store i32 1, ptr [[X]] int x = 0; - @synchronized((x++, a)) { + @synchronized((x++, a)) { } - // CHECK: ret i32 1 + // CHECK: [[RET:%.*]] = load i32, ptr [[X]] + // CHECK-NEXT: call void @llvm.lifetime.end.p0(ptr nonnull [[X]]) + // CHECK-NEXT: ret i32 [[RET]] return x; } diff --git a/llvm/include/llvm/Analysis/AliasAnalysis.h b/llvm/include/llvm/Analysis/AliasAnalysis.h index 281ddc9128160..b6cae7c1cba85 100644 --- a/llvm/include/llvm/Analysis/AliasAnalysis.h +++ b/llvm/include/llvm/Analysis/AliasAnalysis.h @@ -192,6 +192,13 @@ class LLVM_ABI EarliestEscapeAnalysis final : public CaptureAnalysis { /// This is used for cache invalidation purposes. DenseMap<Instruction *, TinyPtrVector<const Value *>> Inst2Obj; + /// Cached result of Function::callsFunctionThatReturnsTwice(), used to + /// account for longjmp re-entry paths not visible to the forward CFG. + std::optional<bool> CallsReturnsTwiceFn; + + /// Whether the function contains a call that may return twice (e.g., setjmp). + bool callsReturnsTwiceFn(); + public: EarliestEscapeAnalysis(DominatorTree &DT, const LoopInfo *LI = nullptr, const CycleInfo *CI = nullptr) diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp index 3aece3ca7d769..7a79b0d6e24f1 100644 --- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp +++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp @@ -254,6 +254,10 @@ CaptureComponents EarliestEscapeAnalysis::getCapturesBefore( if (!I) return false; + // Handle longjmp during re-entry. + if (callsReturnsTwiceFn()) + return false; + if (I == CaptureInst) { if (OrAt) return false; @@ -267,6 +271,13 @@ CaptureComponents EarliestEscapeAnalysis::getCapturesBefore( return Iter.first->second.second.WithoutRet; } +bool EarliestEscapeAnalysis::callsReturnsTwiceFn() { + if (!CallsReturnsTwiceFn) + CallsReturnsTwiceFn = + DT.getRoot()->getParent()->callsFunctionThatReturnsTwice(); + return *CallsReturnsTwiceFn; +} + void EarliestEscapeAnalysis::removeInstruction(Instruction *I) { auto Iter = Inst2Obj.find(I); if (Iter != Inst2Obj.end()) { diff --git a/llvm/test/Analysis/BasicAA/setjmp-longjmp.ll b/llvm/test/Analysis/BasicAA/setjmp-longjmp.ll new file mode 100644 index 0000000000000..57bdf4826feff --- /dev/null +++ b/llvm/test/Analysis/BasicAA/setjmp-longjmp.ll @@ -0,0 +1,133 @@ +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6 +; RUN: opt < %s -aa-pipeline=basic-aa -passes=gvn,dse -S | FileCheck %s + +; Test that BasicAA conservatively returns MayAlias for local allocas in +; functions containing returns_twice calls (e.g. setjmp/sigsetjmp), to prevent +; miscompilation via longjmp re-entry paths invisible in the forward CFG. +; +; Reproduces: https://github.com/llvm/llvm-project/issues/198967 +; +; Without the fix, GVN incorrectly concludes that the store through %p_val +; (which may alias %i via a longjmp re-entry) cannot modify %i, then DSE +; eliminates the "store i32 13, ptr %i" as dead. The fix makes +; EarliestEscapeAnalysis conservatively bail out at the point where +; isNotCapturedBefore would otherwise return true, if the function contains +; any returns_twice call. + +@x = external global i32 +@ii = external global i32 +@p = external global ptr + +declare void @redo() +declare void @checkpoint() returns_twice + +; Control case: function has the same CFG structure but calls no +; returns_twice function. GVN/DSE can legally eliminate "store i32 13, ptr %i" +; because in the forward CFG the only path to if.else does not pass through +; if.then (where &i is stored to @p). BasicAA correctly returns NoAlias for +; (%i, %p_val) here. +; +; Fix case CHECK directives are listed here so that CHECK-NOT is correctly +; scoped between the two CHECK-LABELs: it checks that the store is absent in +; bar_no_returns_twice and present in bar_with_returns_twice. +; +define void @bar_no_returns_twice() { +; CHECK-LABEL: define void @bar_no_returns_twice() { +; CHECK-NEXT: [[ENTRY:.*:]] +; CHECK-NEXT: [[I:%.*]] = alloca i32, align 4 +; CHECK-NEXT: [[X_VAL:%.*]] = load i32, ptr @x, align 4 +; CHECK-NEXT: [[COND:%.*]] = icmp ne i32 [[X_VAL]], 0 +; CHECK-NEXT: br i1 [[COND]], label %[[IF_THEN:.*]], label %[[IF_ELSE:.*]] +; CHECK: [[IF_THEN]]: +; CHECK-NEXT: store ptr [[I]], ptr @p, align 8 +; CHECK-NEXT: call void @redo() +; CHECK-NEXT: br label %[[IF_END:.*]] +; CHECK: [[IF_ELSE]]: +; CHECK-NEXT: [[P_VAL:%.*]] = load ptr, ptr @p, align 8 +; CHECK-NEXT: store i32 42, ptr [[P_VAL]], align 4 +; CHECK-NEXT: store i32 13, ptr @ii, align 4 +; CHECK-NEXT: br label %[[IF_END]] +; CHECK: [[IF_END]]: +; CHECK-NEXT: ret void +; +entry: + %i = alloca i32, align 4 + %x_val = load i32, ptr @x + %cond = icmp ne i32 %x_val, 0 + br i1 %cond, label %if.then, label %if.else + +if.then: + store ptr %i, ptr @p + call void @redo() + br label %if.end + +if.else: + store i32 13, ptr %i + %p_val = load ptr, ptr @p + store i32 42, ptr %p_val + %ii_val = load i32, ptr %i + store i32 %ii_val, ptr @ii + br label %if.end + +if.end: + ret void +} + +; Fix case: same structure, but the function calls @checkpoint(), which is +; returns_twice (as Clang emits for setjmp / sigsetjmp / any returns_twice +; call). BasicAA conservatively returns MayAlias for (%i, %p_val) because a +; longjmp can re-enter at the checkpoint() site with @p already holding &i, +; making the if.then capture visible to if.else. +; +; The "store i32 13, ptr %i" MUST be preserved; removing it is a +; miscompilation. (CHECKs for this function are listed above to correctly +; scope the CHECK-NOT between the two CHECK-LABELs.) +define void @bar_with_returns_twice() { +; CHECK-LABEL: define void @bar_with_returns_twice() { +; CHECK-NEXT: [[ENTRY:.*:]] +; CHECK-NEXT: [[I:%.*]] = alloca i32, align 4 +; CHECK-NEXT: call void @checkpoint() +; CHECK-NEXT: [[X_VAL:%.*]] = load i32, ptr @x, align 4 +; CHECK-NEXT: [[COND:%.*]] = icmp ne i32 [[X_VAL]], 0 +; CHECK-NEXT: br i1 [[COND]], label %[[IF_THEN:.*]], label %[[IF_ELSE:.*]] +; CHECK: [[IF_THEN]]: +; CHECK-NEXT: store ptr [[I]], ptr @p, align 8 +; CHECK-NEXT: call void @redo() +; CHECK-NEXT: br label %[[IF_END:.*]] +; CHECK: [[IF_ELSE]]: +; CHECK-NEXT: store i32 13, ptr [[I]], align 4 +; CHECK-NEXT: [[P_VAL:%.*]] = load ptr, ptr @p, align 8 +; CHECK-NEXT: store i32 42, ptr [[P_VAL]], align 4 +; CHECK-NEXT: [[II_VAL:%.*]] = load i32, ptr [[I]], align 4 +; CHECK-NEXT: store i32 [[II_VAL]], ptr @ii, align 4 +; CHECK-NEXT: br label %[[IF_END]] +; CHECK: [[IF_END]]: +; CHECK-NEXT: ret void +; +entry: + %i = alloca i32, align 4 + call void @checkpoint() + %x_val = load i32, ptr @x + %cond = icmp ne i32 %x_val, 0 + br i1 %cond, label %if.then, label %if.else + +if.then: + ; On the first pass: stores &i into @p, then calls redo(). + ; redo() performs longjmp(buf) which re-enters at checkpoint() above. + ; On re-entry: if.then runs again setting p = &i, then if.else runs + ; with @p == &i, making "store i32 42, ptr %p_val" alias "store i32 13, ptr %i". + store ptr %i, ptr @p + call void @redo() + br label %if.end + +if.else: + store i32 13, ptr %i + %p_val = load ptr, ptr @p + store i32 42, ptr %p_val ; may write to %i via longjmp re-entry path + %ii_val = load i32, ptr %i + store i32 %ii_val, ptr @ii + br label %if.end + +if.end: + ret void +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
