|
| 1 | +# w108: AtomicExpandPass insertRMWCmpXchgLoop InitLoaded drops !tbaa/!noalias/!alias.scope |
| 2 | + |
| 3 | +## Root cause |
| 4 | +`AtomicExpandImpl::insertRMWCmpXchgLoop` (llvm/lib/CodeGen/AtomicExpandPass.cpp:1769) |
| 5 | +synthesizes an initial pre-loop load: |
| 6 | + |
| 7 | +``` |
| 8 | +LoadInst *InitLoaded = Builder.CreateAlignedLoad(ResultTy, Addr, AddrAlign); |
| 9 | +``` |
| 10 | + |
| 11 | +It then calls `InitLoaded->setVolatile(IsVolatile)` and conditionally |
| 12 | +`InitLoaded->setAtomic(Monotonic, SSID)`. The subsequent cmpxchg (created via |
| 13 | +the `CreateCmpXchg` callback) goes through `createCmpXchgInstFun` which |
| 14 | +explicitly calls `copyMetadataForAtomic(*Pair, *MetadataSrc)` at line 758 to |
| 15 | +carry `!tbaa`, `!alias.scope`, `!noalias`, `!noalias.addrspace`, |
| 16 | +`!access_group`, `!mmra` onto the cmpxchg. |
| 17 | + |
| 18 | +The `InitLoaded` seed load does NOT receive any of that metadata. It only |
| 19 | +gets the metadata collected by `ReplacementIRBuilder` (which is just |
| 20 | +`!pcsections` and `!annotation`, see ctor at line 64). |
| 21 | + |
| 22 | +The omission is invisible in a peephole reading of the file because the |
| 23 | +copy is on the cmpxchg, not the load. But the InitLoaded participates in the |
| 24 | +SAME memory location and aliasing semantics as the cmpxchg and the original |
| 25 | +atomicrmw. |
| 26 | + |
| 27 | +## Trigger condition (x86) |
| 28 | +`X86TargetLowering::shouldExpandAtomicRMWInIR` returns `CmpXChg` for |
| 29 | +`Nand`, `Max`, `Min`, `UMax`, `UMin`, `FAdd`, `FSub`, `FMax`, `FMin`, |
| 30 | +`UIncWrap`, `UDecWrap`, `USubCond`, `USubSat` (see X86ISelLowering.cpp:32979-32995). |
| 31 | +These RMW ops always require a cmpxchg loop on x86, regardless of size. |
| 32 | + |
| 33 | +Reproducer: |
| 34 | + |
| 35 | +``` |
| 36 | +target triple = "x86_64-unknown-linux-gnu" |
| 37 | +
|
| 38 | +define i32 @test_tbaa_nand(ptr %p) { |
| 39 | + %r = atomicrmw nand ptr %p, i32 1 seq_cst, align 4, !tbaa !2, !noalias !6 |
| 40 | + ret i32 %r |
| 41 | +} |
| 42 | +
|
| 43 | +define i32 @test_tbaa_uincwrap(ptr %p) { |
| 44 | + %r = atomicrmw uinc_wrap ptr %p, i32 7 seq_cst, align 4, !tbaa !2, !noalias !6 |
| 45 | + ret i32 %r |
| 46 | +} |
| 47 | +
|
| 48 | +!0 = !{!"alias-domain"} |
| 49 | +!1 = !{!"alias-scope-a", !0} |
| 50 | +!2 = !{!3, !3, i64 0} |
| 51 | +!3 = !{!"int", !4, i64 0} |
| 52 | +!4 = !{!"omnipotent char", !5, i64 0} |
| 53 | +!5 = !{!"Simple C/C++ TBAA"} |
| 54 | +!6 = !{!1} |
| 55 | +``` |
| 56 | + |
| 57 | +After `llc -mtriple=x86_64-unknown-linux-gnu -stop-after=atomic-expand`: |
| 58 | + |
| 59 | +``` |
| 60 | +define i32 @test_tbaa_nand(ptr %p) { |
| 61 | + %1 = load i32, ptr %p, align 4 ; <-- BUG: no !tbaa, no !noalias |
| 62 | + br label %atomicrmw.start |
| 63 | +atomicrmw.start: |
| 64 | + %loaded = phi i32 [ %1, %0 ], [ %newloaded, %atomicrmw.start ] |
| 65 | + %2 = and i32 %loaded, 1 |
| 66 | + %new = xor i32 %2, -1 |
| 67 | + %3 = cmpxchg ptr %p, i32 %loaded, i32 %new seq_cst seq_cst, align 4, !tbaa !0, !noalias !4 |
| 68 | + %success = extractvalue { i32, i1 } %3, 1 |
| 69 | + %newloaded = extractvalue { i32, i1 } %3, 0 |
| 70 | + br i1 %success, label %atomicrmw.end, label %atomicrmw.start |
| 71 | +atomicrmw.end: |
| 72 | + ret i32 %newloaded |
| 73 | +} |
| 74 | +
|
| 75 | +define i32 @test_tbaa_uincwrap(ptr %p) { |
| 76 | + %1 = load i32, ptr %p, align 4 ; <-- BUG: no !tbaa, no !noalias |
| 77 | + br label %atomicrmw.start |
| 78 | + ... |
| 79 | +} |
| 80 | +``` |
| 81 | + |
| 82 | +`%1` (the InitLoaded seed) lacks `!tbaa` and `!noalias` while the cmpxchg |
| 83 | +in the same loop carries both. |
| 84 | + |
| 85 | +## Why this is a miscompile (not a quality issue) |
| 86 | +After AtomicExpand, the IR is handed back to the codegen pipeline and to the |
| 87 | +DAG combiner. The `!noalias` metadata on the cmpxchg promises the compiler |
| 88 | +that the cmpxchg's memory access does not alias the noalias-scope-tagged |
| 89 | +pointers in scope `!1`. But the SAME pointer `%p` is now accessed by an |
| 90 | +untagged load. A later pass that ingests the IR (e.g. a LICM/CSE under a |
| 91 | +PGSO/LTO recompile, or a manual `opt -tbaa -mldst-motion` run after this |
| 92 | +file is dumped) may: |
| 93 | + |
| 94 | +1. CSE/merge the untagged `%1 = load i32, ptr %p` with a TBAA-tagged load of |
| 95 | + a different alias type at `ptr %p` (e.g. `load i32 ... !tbaa <other-type>`) |
| 96 | + that the original atomicrmw with TBAA `!tbaa !2` was explicitly distinct |
| 97 | + from. CSE looks for syntactic equality of the address; with no TBAA on the |
| 98 | + InitLoaded, the metadata-strip semantics of `MDNode::getMostGenericTBAA` |
| 99 | + make this load identical to any other untagged i32 load at `%p` even |
| 100 | + across alias domains. |
| 101 | +2. Use `BasicAA + TBAA` to conclude an aliasing store BEFORE the InitLoaded |
| 102 | + does NOT alias (because the cmpxchg with TBAA `!tbaa !2` says so), then |
| 103 | + sink the store past it; but for the InitLoaded the AA query collapses to |
| 104 | + plain BasicAA which DOES report MayAlias, producing inconsistent |
| 105 | + reorderings between the seed load and the cmpxchg in the loop. The two |
| 106 | + accesses to `%p` are now treated differently by aliasing analysis even |
| 107 | + though they are the SAME atomicrmw decomposition. |
| 108 | +3. With `!noalias !6`, an interloping load tagged `!alias.scope !6` MUST |
| 109 | + not be moved across the original RMW. After expansion the cmpxchg still |
| 110 | + forbids the move, but the InitLoaded does not, so the interloping load |
| 111 | + may now be sunk between the InitLoaded and the cmpxchg loop -- a |
| 112 | + semantic move that crosses the *original* RMW boundary. |
| 113 | + |
| 114 | +This is the same class of metadata-loss miscompile as #088 |
| 115 | +(`w88-rmwcmpxchgloop-initload-drops-md.md`) but for the more impactful |
| 116 | +TBAA/noalias/alias.scope metadata rather than `pcsections`. |
| 117 | + |
| 118 | +## Fix |
| 119 | +At line 1769, after creating `InitLoaded`, call |
| 120 | +`copyMetadataForAtomic(*InitLoaded, *MetadataSrc)` (with the `MetadataSrc` |
| 121 | +argument that the function already receives at line 1740). The cmpxchg in |
| 122 | +the loop already does this at line 758; the seed load must too. |
| 123 | + |
| 124 | +## Related bugs |
| 125 | +- #088 (`w88-rmwcmpxchgloop-initload-drops-md.md`): same call site, focuses |
| 126 | + on `!pcsections`; this entry covers the more dangerous AA metadata. |
| 127 | +- #066: convertAtomicLoadToIntegerType / expandAtomicLoadToCmpXchg drop |
| 128 | + syncscope/volatile -- sibling call sites in same file. |
| 129 | +- #024: widenPartwordAtomicRMW drops volatile. |
| 130 | + |
| 131 | +The fix in #088 (if applied) does not necessarily cover this case because |
| 132 | +the metadata families are different switch arms in `copyMetadataForAtomic`. |
0 commit comments