[RFC][Transforms] Prefer unreachable insn over optimizaiton with undef

[yonghong-song]

In some cases, the code has uninitialized variable and the uninitialized variable actually has an impact on the code. For example,

  $ cat t.c
  __attribute__((always_inline)) int bar(int a, int *b) {
    if ((*b) == 0) return a;
    else return 2 * a;
  }
  void tar(int);
  int foo(int a) {
    int b;
    return bar(a, &b);
  }

In the above variable 'b' is uninitialized. With the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -mllvm -print-after-all
The EarlyCSEPass changes the input IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    %cmp.i = icmp eq i32 undef, 0
    %mul.i = shl nsw i32 %a, 1
    %retval.0.i = select i1 %cmp.i, i32 %a, i32 %mul.i
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    ret i32 %a
  }

In the above, the llvm generates code without any 'undef' values. On the other hand, with the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -fsanitize=undefined -mllvm -print-after-all
The SCCPPass changes the input IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #3 !func_sanitize !12 {
  entry:
    br i1 undef, label %bar.exit, label %if.else.i

  if.else.i:                                        ; preds = %entry
    %0 = shl i32 %a, 1
    %1 = add i32 %a, 1073741824
    %2 = icmp sgt i32 %1, -1
    br i1 %2, label %bar.exit, label %handler.mul_overflow.i, !prof !7, !nosanitize !6

  handler.mul_overflow.i:                           ; preds = %if.else.i
    %3 = zext i32 %a to i64, !nosanitize !6
    tail call void @__ubsan_handle_mul_overflow(ptr nonnull @2, i64 2, i64 %3) #5, !nosanitize !6
    br label %bar.exit, !nosanitize !6

  bar.exit:                                         ; preds = %entry, %if.else.i, %handler.mul_overflow.i
    %retval.0.i = phi i32 [ %a, %entry ], [ %0, %handler.mul_overflow.i ], [ %0, %if.else.i ]
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #3 !func_sanitize !12 {
  entry:
    unreachable
  }

Note that in the above example, the unitialized variable is used cross function boundary which makes frontend harder to emit error messages. And bpf prog needs to pass verifier which makes runtime sanitize not suitable. On the other hand, bpf prog typically is not that big and bpf prog also tends have quite some inlining for performance reason or to avoid kernel verification failure. So detecting and reporting impactful uninit var can help improve bpf developer productivity greatly.

There are more discussion in [1] and [2]. To maximize chances to report proper unit var warnings (beyond -Wall -Werror), I would like to discuss two things related to the above example:

1. Avoid generating legal code from 'undef' code. This is needed so the 'undef' code can be carried through entire compilation. And in many cases, 'undef' is eventually transformed to 'unreachable' insn. Generating legal code (without 'undef') will prevent later catching 'undef/unreachable' cases.
2. As in discussions in [2], looks like clang-format does not like BPF Backend to check undef values. So if possible, it would be great to convert 'undef' related code to 'unreachable', e.g. in the above SCCPPass.

This RFC intends to have some upstream discussion on how to achieve the above two goals.

With this patch, for the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -mllvm -print-after-all
The EarlyCSEPass changes the input IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    %cmp.i = icmp eq i32 undef, 0
    %mul.i = shl nsw i32 %a, 1
    %retval.0.i = select i1 %cmp.i, i32 %a, i32 %mul.i
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    unreachable
  }

And '%mul.i = shl nsw i32 %a, 1' is removed in SimplifyCFGPass.

  ; *** IR Dump After CorrelatedValuePropagationPass on foo ***
  ; Function Attrs: nounwind uwtable
  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    unreachable
  }
  ; *** IR Dump After SimplifyCFGPass on foo ***
  ; Function Attrs: nounwind uwtable
  define dso_local i32 @foo(i32 noundef %a) local_unnamed_addr #1 {
  entry:
    unreachable
  }

[1] https://discourse.llvm.org/t/detect-undefined-behavior-due-to-uninitialized-variables-in-bpf-programs/84116/4?u=yonghong-song
[2] #126858

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: None (yonghong-song)

Changes

In some cases, the code has uninitialized variable and the uninitialized variable actually has an impact on the code. For example,

  $ cat t.c
  __attribute__((always_inline)) int bar(int a, int *b) {
    if ((*b) == 0) return a;
    else return 2 * a;
  }
  void tar(int);
  int foo(int a) {
    int b;
    return bar(a, &b);
  }

In the above variable 'b' is uninitialized. With the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -mllvm -print-after-all
The EarlyCSEPass changes the input IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    %cmp.i = icmp eq i32 undef, 0
    %mul.i = shl nsw i32 %a, 1
    %retval.0.i = select i1 %cmp.i, i32 %a, i32 %mul.i
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    ret i32 %a
  }

In the above, the llvm generates code without any 'undef' values. On the other hand, with the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -fsanitize=undefined -mllvm -print-after-all
The SCCPPass changes the input IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->3 !func_sanitize !12 {
  entry:
    br i1 undef, label %bar.exit, label %if.else.i

  if.else.i:                                        ; preds = %entry
    %0 = shl i32 %a, 1
    %1 = add i32 %a, 1073741824
    %2 = icmp sgt i32 %1, -1
    br i1 %2, label %bar.exit, label %handler.mul_overflow.i, !prof !7, !nosanitize !6

  handler.mul_overflow.i:                           ; preds = %if.else.i
    %3 = zext i32 %a to i64, !nosanitize !6
    tail call void @<!-- -->__ubsan_handle_mul_overflow(ptr nonnull @<!-- -->2, i64 2, i64 %3) #<!-- -->5, !nosanitize !6
    br label %bar.exit, !nosanitize !6

  bar.exit:                                         ; preds = %entry, %if.else.i, %handler.mul_overflow.i
    %retval.0.i = phi i32 [ %a, %entry ], [ %0, %handler.mul_overflow.i ], [ %0, %if.else.i ]
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->3 !func_sanitize !12 {
  entry:
    unreachable
  }

Note that in the above example, the unitialized variable is used cross function boundary which makes frontend harder to emit error messages. And bpf prog needs to pass verifier which makes runtime sanitize not suitable. On the other hand, bpf prog typically is not that big and bpf prog also tends have quite some inlining for performance reason or to avoid kernel verification failure. So detecting and reporting impactful uninit var can help improve bpf developer productivity greatly.

There are more discussion in [1] and [2]. To maximize chances to report proper unit var warnings (beyond -Wall -Werror), I would like to discuss two things related to the above example:

1. Avoid generating legal code from 'undef' code. This is needed so the 'undef' code can be carried through entire compilation. And in many cases, 'undef' is eventually transformed to 'unreachable' insn. Generating legal code (without 'undef') will prevent later catching 'undef/unreachable' cases.
2. As in discussions in [2], looks like clang-format does not like BPF Backend to check undef values. So if possible, it would be great to convert 'undef' related code to 'unreachable', e.g. in the above SCCPPass.

This RFC intends to have some upstream discussion on how to achieve the above two goals.

With this patch, for the following compilation flag:
clang -O2 -S -emit-llvm -Wall -Werror t.c -mllvm -print-after-all
The EarlyCSEPass changes the input IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    %cmp.i = icmp eq i32 undef, 0
    %mul.i = shl nsw i32 %a, 1
    %retval.0.i = select i1 %cmp.i, i32 %a, i32 %mul.i
    ret i32 %retval.0.i
  }

to output IR

  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    unreachable
  }

And '%mul.i = shl nsw i32 %a, 1' is removed in SimplifyCFGPass.

  ; *** IR Dump After CorrelatedValuePropagationPass on foo ***
  ; Function Attrs: nounwind uwtable
  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    %mul.i = shl nsw i32 %a, 1
    unreachable
  }
  ; *** IR Dump After SimplifyCFGPass on foo ***
  ; Function Attrs: nounwind uwtable
  define dso_local i32 @<!-- -->foo(i32 noundef %a) local_unnamed_addr #<!-- -->1 {
  entry:
    unreachable
  }

[1] https://discourse.llvm.org/t/detect-undefined-behavior-due-to-uninitialized-variables-in-bpf-programs/84116/4?u=yonghong-song
[2] #126858

---

Full diff: https://github.com/llvm/llvm-project/pull/131020.diff

2 Files Affected:

• (modified) llvm/lib/Analysis/InstructionSimplify.cpp (+2-2)
• (modified) llvm/lib/Transforms/Scalar/EarlyCSE.cpp (+11)

diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 1c33c6bebdd1b..b09ac42945a4a 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -4829,9 +4829,9 @@ static Value *simplifySelectInst(Value *Cond, Value *TrueVal, Value *FalseVal,
     if (isa<PoisonValue>(CondC))
       return PoisonValue::get(TrueVal->getType());
 
-    // select undef, X, Y -> X or Y
+    // select undef, X, Y -> undef
     if (Q.isUndefValue(CondC))
-      return isa<Constant>(FalseVal) ? FalseVal : TrueVal;
+      return UndefValue::get(TrueVal->getType());
 
     // select true,  X, Y --> X
     // select false, X, Y --> Y
diff --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
index 3a0ae6b01a114..0bf8a89894e60 100644
--- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
+++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
@@ -1780,6 +1780,17 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
           LastStore = nullptr;
       }
     }
+
+    // If the ret insn returns an undefined (but not a poinson) value,
+    // change it to unreachable.
+    if (Inst.getOpcode() == Instruction::Ret) {
+      auto *RI = cast<ReturnInst>(&Inst);
+      auto *RetVal = RI->getReturnValue();
+      if (isa<UndefValue>(RetVal) && !isa<PoisonValue>(RetVal)) {
+        changeToUnreachable(&Inst, false, NULL, &*MSSAUpdater);
+        Changed = true;
+      }
+    }
   }
 
   return Changed;

[yonghong-song]

cc @4ast @eddyz87 @anakryiko @jemarch

[github-actions]

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:

git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 606e9fa444559923a9e03cbdffc1abd9e2582d60 8b46b555f1a959d81890f8a8d087789655ef5678 llvm/lib/Analysis/InstructionSimplify.cpp llvm/lib/Transforms/Scalar/EarlyCSE.cpp

The following files introduce new uses of undef:

• llvm/lib/Analysis/InstructionSimplify.cpp

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

[nikic]

Both of the proposed transforms are incorrect. Please see https://llvm.org/docs/InstCombineContributorGuide.html#proofs to learn how you can prove the correctness of proposed transforms.

[nikic]

This is incorrect. The result of the select must be one of X or Y.

[yonghong-song]

Thanks for clarification. Do we have any way to expose such a transformation to user? For example, through some Diagnostic or something else? This will be very useful for users to get early signal about their codes.

[nikic]

This is also incorrect. A ret undef is perfectly legal by itself. It could be converted to unreachable if the return value has a noundef attribute. As written, the transform is incorrect.