[InstCombine] Treat passing undef to noundef params as UB

[aeubanks]

Summary

Skip on msan instrumented functions since this hurts msan's usefulness.

Add a flag to disable while ToT LLVM users fix regressions (the flag may be useful for bisecting files). This flag will be removed in the future.

[nikic]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3027

Call.isPassingUndefUB(I). You probably want to flip the checks, because the UndefValue check is cheaper. Also add tests with dereferenceable instead of noundef.

[aeubanks]

update

[aeubanks]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3027

isa<UndefValue> seems much less likely to be true though. anyway, flipped

added dereferenceable tests on top of noundef since ints can't be dereferenceable

[nikic]

LG

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3027

nit: unnecessary braces

[aeubanks]

fix clang test

[mstorsjo]

FYI, this change broke Wine (at least on arm and aarch64). Not saying that it is legit breakage of code that actually was UB to begin with though - it's going to take some time to figure out what's broken though.

[omjavaid]

This change has broken LLDB Arm/AArch64 Linux buildbots. I dont really understand
the underlying reason. Reverting for now to make buildbot green.

[omjavaid]

Apparently some problem occuring due to trouble with ASAN exception
https://lab.llvm.org/buildbot/#/builders/96/builds/28300

[TimNN]

This also broke Rust when compiled at LLVM head: https://buildkite.com/llvm-project/rust-llvm-integrate-prototype/builds/13166#0182fb4a-0f2d-4f2e-830f-f62b463b8d48. (I don't know whether this was some existing UB that got only exposed by this patch or not, but wanted to make you aware. Reproduces with 2+ codegen units and a 1+ optimization level).

[sbc100]

This change also broken emscripten in some odd ways: https://app.circleci.com/pipelines/github/emscripten-core/emscripten/23359/workflows/4080be5f-bd82-45b9-a355-3a5d4f4ef977/jobs/564543. The revert seems to have fixed it.

[aeubanks]

"fix" lldb test (hopefully)
skip when msan
add flag to disable (to be removed in the future)

[aeubanks]

add flag to disable (to be removed in the future)
"fix" lldb test
skip on msan instrumented functions

[aeubanks]

extra parens

[nikic]

I'd like to block this on -fsanitize-memory-param-retval (aka msan eager checks) being enabled by default. It seems pretty clear that there is a *lot* of UB due to uninitialized parameters floating around, so we should at least make sure that it is detected in default sanitizer configurations before we start exploiting it this aggressively.

[aeubanks]

@vitalybuka can we turn on -fsanitize-memory-param-retval by default upstream?

[aeubanks]

In D133036#3816002, @aeubanks wrote:

@vitalybuka can we turn on -fsanitize-memory-param-retval by default upstream?

attempt in https://reviews.llvm.org/D134669

[nikic]

The default switch has happened, so unblocking this.

clang/test/CodeGenOpenCL/overload.cl

23 ↗

(On Diff #462249)

Maybe initialize the relevant variables instead? I'd assume just NULL would work here.

[TimNN]

I'm still trying to properly minimize this, but this definitely interacts badly with other optimizations (which triggers the Rust CI failure I mentioned above):

• We start with two functions, outer and inner. outer calls inner. outer has a noundef argument that it forwards to inner (where the argument is also noundef).
• PostOrderFunctionAttrsPass decides, for both functions, that the argument is readnone. The readnone attribute is only added to the function definitions. The readnone attribute is not added at the call to inner from outer.
• The DeadArgumentEliminationPass decides that when outer calls inner the argument should be poison.
• This patch sees the poison being passed to a noundef argument and kills the call.

I don't know which component should be considered "at fault" here (or if the readnone bit is even relevant).

[nikic]

@TimNN Do you have an IR sample for this? DAE is supposed to strip UB-implying attributes when converting arguments to poison here: https://github.com/llvm/llvm-project/blob/cbe5b2dd914b7ee47bb4d0f67af154a40be4d208/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp#LL291C17-L291C37

[TimNN]

I've included excerpts from the IR below. It will take me a bit to provide something compilable. Though you are right, the noundef did indeed get removed from the call.

*** IR Dump Before DeadArgumentEliminationPass on [module] ***
; Function Attrs: nonlazybind uwtable
define void @_RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_(i1 noundef zeroext %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias nocapture noundef nonnull readnone align 1 %2) unnamed_addr #0 !dbg !18135 {
  %4 = zext i1 %0 to i8, !dbg !18137
  call void @_RNvXs0_NtNtCs840rfDNPFol_10proc_macro6bridge3rpchINtB5_6EncodeuE6encodeB9_(i8 %4, ptr noalias noundef nonnull align 8 dereferenceable(40) %1, ptr noalias noundef nonnull align 1 %2), !dbg !18137
  ret void, !dbg !18138
}
*** IR Dump After DeadArgumentEliminationPass on [module] ***
; Function Attrs: nonlazybind uwtable
define void @_RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_(i1 noundef zeroext %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias nocapture noundef nonnull readnone align 1 %2) unnamed_addr #0 !dbg !18136 {
  %4 = zext i1 %0 to i8, !dbg !18138
  call void @_RNvXs0_NtNtCs840rfDNPFol_10proc_macro6bridge3rpchINtB5_6EncodeuE6encodeB9_(i8 %4, ptr noalias noundef nonnull align 8 dereferenceable(40) %1, ptr noalias nonnull align 1 poison), !dbg !18138
  ret void, !dbg !18139
}
*** IR Dump Before InstCombinePass on _RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_ ***
; Function Attrs: nonlazybind uwtable
define available_externally void @_RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_(i1 noundef zeroext %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias nocapture noundef nonnull readnone align 1 %2) unnamed_addr #2 !dbg !20759 {
  %4 = zext i1 %0 to i8, !dbg !20762
  call void @_RNvXs0_NtNtCs840rfDNPFol_10proc_macro6bridge3rpchINtB5_6EncodeuE6encodeB9_(i8 %4, ptr noalias noundef nonnull align 8 dereferenceable(40) %1, ptr noalias nonnull align 1 poison), !dbg !20762
  ret void, !dbg !20763
}
*** IR Dump After InstCombinePass on _RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_ ***
; Function Attrs: nonlazybind uwtable
define available_externally void @_RNvXs2_NtNtCs840rfDNPFol_10proc_macro6bridge3rpcbINtB5_6EncodeuE6encodeB9_(i1 noundef zeroext %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias nocapture noundef nonnull readnone align 1 %2) unnamed_addr #2 !dbg !20759 {
  store i1 true, ptr poison, align 1
  ret void, !dbg !20762
}

[TimNN]

I didn't manage to repro with opt, so still no compilable IR. I did some more debugging, though:

• Inside removeDeadArgumentsFromCallers, CB->getCalledFunction()->dump() is define void @_RNvXs0_NtNtCs840rfDNPFol_10proc_macro6bridge3rpchINtB5_6EncodeuE6encodeB9_(i8 %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias nocapture nonnull readnone align 1 %2) unnamed_addr #0 personality ptr @rust_eh_personality !dbg !18034 { ... }. Definition, no noundef.
• Inside callPassesUndefToPassingUndefUBParam, Call.getCalledFunction()->dump() is declare void @_RNvXs0_NtNtCs840rfDNPFol_10proc_macro6bridge3rpchINtB5_6EncodeuE6encodeB9_(i8 %0, ptr noalias noundef align 8 dereferenceable(40) %1, ptr noalias noundef nonnull align 1 %2) unnamed_addr #2. Declaration, noundef is back.

[TimNN]

I'm sorry for the noise. Further investigation has shown that this happens when Rust is doing (thin) LTO, and I don't think this patch can be considered in any way "at fault" here, so this is the last you'll hear from me on the topic here.

I don't know whether the fault is with how Rust implements (thin) LTO or something on the LLVM side. Basically, after running the FunctionImporter on a module, we end up with a define available_externally void outer and a declare void inner, presumably coming from different modules. outer contains the call to inner with poison, but the parameter is noundef on the declaration on inner.

edit: Issue in the Rust repo: https://github.com/rust-lang/rust/issues/104377

edit2: LLVM issue: https://github.com/llvm/llvm-project/issues/58976

[aeubanks]

I somehow missed the previous comments

rebased on top of fixing UB in tests changes

[aeubanks]

I'll hold off on submitting this until that bug is figured out

[TimNN]

The ThinLTO related breakage I mentioned above should be fixed as of https://github.com/llvm/llvm-project/commit/451799bb8261bde52bbfef226d019caf1d82aa42.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

 STATISTIC(NumSimplified, "Number of library calls simplified");
 
 static cl::opt<unsigned> GuardWideningWindow(
     "instcombine-guard-widening-window",
     cl::init(3),
     cl::desc("How wide an instruction window to bypass looking for "
              "another guard"));
 
+// TODO: remove this flag after LLVM ToT users have resolved issues around this.
+static cl::opt<bool> DisableOptimizePassingUndefUB(
+    "instcombine-disable-optimize-passing-undef-ub", cl::init(false),
+    cl::desc("Disable optimizing away undefined behavior around passing undef "
+             "to noundef params"));
+
 namespace llvm {
 /// enable preservation of attributes in assume like:
 /// call void @llvm.assume(i1 true) [ "nonnull"(i32* %PTR) ]
 extern cl::opt<bool> EnableKnowledgeRetention;
 } // namespace llvm
 
 /// Return the specified type promoted as it would be to pass though a va_arg
 /// area.
static IntrinsicInst *findInitTrampoline(Value *Callee) {
 
   if (IntrinsicInst *IT = findInitTrampolineFromAlloca(TrampMem))
     return IT;
   if (IntrinsicInst *IT = findInitTrampolineFromBB(AdjustTramp, TrampMem))
     return IT;
   return nullptr;
 }
 
+static bool callPassesUndefToPassingUndefUBParam(CallBase &Call) {
+  if (DisableOptimizePassingUndefUB)
+    return false;
+  // Optimizing this hurts msan's usefulness.
+  if (Call.getParent()->getParent()->hasFnAttribute(Attribute::SanitizeMemory))
+    return false;
+  for (unsigned I = 0; I < Call.arg_size(); ++I) {
+    if (isa<UndefValue>(Call.getArgOperand(I)) && Call.isPassingUndefUB(I))
+      return true;
+  }
+  return false;
+}
+
 bool InstCombinerImpl::annotateAnyAllocSite(CallBase &Call,
                                             const TargetLibraryInfo *TLI) {
   // Note: We only handle cases which can't be driven from generic attributes
   // here.  So, for example, nonnull and noalias (which are common properties
   // of some allocation functions) are expected to be handled via annotation
   // of the respective allocator declaration with generic attributes.
   bool Changed = false;
 
if ((CalleeF->getCallingConv() != Call.getCallingConv() &&
           CalleeF->getFunctionType(),
           Constant::getNullValue(CalleeF->getType()));
       return nullptr;
     }
   }
 
   // Calling a null function pointer is undefined if a null address isn't
   // dereferenceable.
+  // Passing undef/poison to any parameter where doing so is UB is undefined (of
+  // course).
   if ((isa<ConstantPointerNull>(Callee) &&
        !NullPointerIsDefined(Call.getFunction())) ||
-      isa<UndefValue>(Callee)) {
+      isa<UndefValue>(Callee) || callPassesUndefToPassingUndefUBParam(Call)) {
     // If Call does not return void then replaceInstUsesWith poison.
     // This allows ValueHandlers and custom metadata to adjust itself.
     if (!Call.getType()->isVoidTy())
       replaceInstUsesWith(Call, PoisonValue::get(Call.getType()));
 
     if (Call.isTerminator()) {
       // Can't remove an invoke or callbr because we cannot change the CFG.
       return nullptr;

llvm/test/Transforms/InstCombine/call-undef.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes=instcombine -S -instcombine-disable-optimize-passing-undef-ub | FileCheck %s --check-prefix=DISABLE
 
 declare void @c(i32 noundef)
 declare void @d(ptr dereferenceable(1))
 declare void @e(i32)
 declare void @f(ptr)
 
 define void @test1() {
 ; CHECK-LABEL: @test1(
-; CHECK-NEXT:    call void @c(i32 undef)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test1(
+; DISABLE-NEXT:    call void @c(i32 undef)
+; DISABLE-NEXT:    ret void
+;
   call void @c(i32 undef)
   ret void
 }
 
 define void @test2() {
 ; CHECK-LABEL: @test2(
-; CHECK-NEXT:    call void @c(i32 poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test2(
+; DISABLE-NEXT:    call void @c(i32 poison)
+; DISABLE-NEXT:    ret void
+;
   call void @c(i32 poison)
   ret void
 }
 
 define void @test3() {
 ; CHECK-LABEL: @test3(
-; CHECK-NEXT:    call void @e(i32 noundef undef)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test3(
+; DISABLE-NEXT:    call void @e(i32 noundef undef)
+; DISABLE-NEXT:    ret void
+;
   call void @e(i32 noundef undef)
   ret void
 }
 
 define void @test4() {
 ; CHECK-LABEL: @test4(
-; CHECK-NEXT:    call void @e(i32 noundef poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test4(
+; DISABLE-NEXT:    call void @e(i32 noundef poison)
+; DISABLE-NEXT:    ret void
+;
   call void @e(i32 noundef poison)
   ret void
 }
 
 define void @test5() {
 ; CHECK-LABEL: @test5(
-; CHECK-NEXT:    call void @d(ptr undef)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test5(
+; DISABLE-NEXT:    call void @d(ptr undef)
+; DISABLE-NEXT:    ret void
+;
   call void @d(ptr undef)
   ret void
 }
 
 define void @test6() {
 ; CHECK-LABEL: @test6(
-; CHECK-NEXT:    call void @d(ptr poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test6(
+; DISABLE-NEXT:    call void @d(ptr poison)
+; DISABLE-NEXT:    ret void
+;
   call void @d(ptr poison)
   ret void
 }
 
 define void @test7() {
 ; CHECK-LABEL: @test7(
-; CHECK-NEXT:    call void @f(ptr dereferenceable(1) undef)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test7(
+; DISABLE-NEXT:    call void @f(ptr dereferenceable(1) undef)
+; DISABLE-NEXT:    ret void
+;
   call void @f(ptr dereferenceable(1) undef)
   ret void
 }
 
 define void @test8() {
 ; CHECK-LABEL: @test8(
-; CHECK-NEXT:    call void @f(ptr dereferenceable(1) poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test8(
+; DISABLE-NEXT:    call void @f(ptr dereferenceable(1) poison)
+; DISABLE-NEXT:    ret void
+;
   call void @f(ptr dereferenceable(1) poison)
   ret void
 }
 
+define void @test9() sanitize_memory {
+; CHECK-LABEL: @test9(
+; CHECK-NEXT:    call void @c(i32 undef)
+; CHECK-NEXT:    ret void
+;
+; DISABLE-LABEL: @test9(
+; DISABLE-NEXT:    call void @c(i32 undef)
+; DISABLE-NEXT:    ret void
+;
+  call void @c(i32 undef)
+  ret void
+}
+
 define void @test_mismatched_call() {
 ; CHECK-LABEL: @test_mismatched_call(
 ; CHECK-NEXT:    call void @e(i8 poison)
 ; CHECK-NEXT:    ret void
 ;
+; DISABLE-LABEL: @test_mismatched_call(
+; DISABLE-NEXT:    call void @e(i8 poison)
+; DISABLE-NEXT:    ret void
+;
   call void @e(i8 poison)
   ret void
 }

llvm/test/Transforms/InstCombine/out-of-bounds-indexes.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -passes=instcombine < %s | FileCheck %s
 ; Check that we don't crash on unreasonable constant indexes
 
 define i32 @test_out_of_bounds(i32 %a, i1 %x, i1 %y) {
 ; CHECK-LABEL: @test_out_of_bounds(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[A:%.*]], 3
-; CHECK-NEXT:    tail call void @llvm.assume(i1 poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret i32 [[AND1]]
 ;
 entry:
   %and1 = and i32 %a, 3
   %B = lshr i32 %and1, -2147483648
   %cmp = icmp eq i32 %B, 1
   tail call void @llvm.assume(i1 %cmp)
   ret i32 %and1
 }
 
 define i128 @test_non64bit(i128 %a) {
 ; CHECK-LABEL: @test_non64bit(
 ; CHECK-NEXT:    [[AND1:%.*]] = and i128 [[A:%.*]], 3
-; CHECK-NEXT:    tail call void @llvm.assume(i1 poison)
+; CHECK-NEXT:    store i1 true, ptr poison, align 1
 ; CHECK-NEXT:    ret i128 [[AND1]]
 ;
   %and1 = and i128 %a, 3
   %B = lshr i128 %and1, -1
   %cmp = icmp eq i128 %B, 1
   tail call void @llvm.assume(i1 %cmp)
   ret i128 %and1
 }