[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Hyeongyu Kim via Phabricator via cfe-commits]

hyeongyukim added a comment.

In D105169#3116810 <https://reviews.llvm.org/D105169#3116810>, @nathanchance wrote:

> Prior to the latest revert (fd9b099906c61e46574d1ea2d99b973321fe1d21 <https://reviews.llvm.org/rGfd9b099906c61e46574d1ea2d99b973321fe1d21>), the Linux kernel's binary verifier (`objtool`) points out an issue when building with ThinLTO and `-fsanitize=integer-divide-by-zero`. I have no idea if this is an issue with the tool or this series. A simplified reproducer:
>
>   $ cat ravb_main.i
>   int ravb_set_gti_ndev_rate;
>   unsigned int ravb_set_gti_ndev_inc;
>   void ravb_set_gti_ndev() {
>     ravb_set_gti_ndev_inc = 1000000000;
>     ravb_set_gti_ndev_inc = ravb_set_gti_ndev_inc / ravb_set_gti_ndev_rate;
>     if (ravb_set_gti_ndev_inc)
>       _dev_err(ravb_set_gti_ndev_inc);
>   }
>   
>   $ clang -std=gnu89 -O2 -flto=thin -fsanitize=integer-divide-by-zero -c -o ravb_main.o ravb_main.i
>   
>   $ llvm-ar cDPrsT ravb.o ravb_main.o
>   
>   $ ld.lld -m elf_x86_64 -r -o ravb.lto.o --whole-archive ravb.o
>   
>   $ ./objtool orc generate --no-fp --no-unreachable --retpoline --uaccess --mcount --module ravb.lto.o
>   ravb.lto.o: warning: objtool: .text.ravb_set_gti_ndev: unexpected end of section
>
> With LLVM 13.0.0, there is no warning with those commands. The original and reduced `.i` file, interestingness test, and static `objtool` binary are available here <https://github.com/nathanchance/creduce-files/tree/2010d2dc4fd217948a734d8bfb6be5460222b6b4/D105169>.

I checked the reason why Objtool makes a warning.

  cont.thread:                                      ; preds = %entry
    tail call void @__ubsan_handle_divrem_overflow(i8* bitcast ({ { [12 x i8]*, i32, i32 }, { i16, i16, [15 x i8] }* }* @1 to i8*), i64 1000000000, i64 0) #3, !nosanitize !8
    br label %if.then
  
  cont:                                             ; preds = %entry
    %div = udiv i32 1000000000, %0
    store i32 %div, i32* @ravb_set_gti_ndev_inc, align 4, !tbaa !4
    %tobool.not = icmp ugt i32 %0, 1000000000
    br i1 %tobool.not, label %if.end, label %if.then
  
  if.then:                                          ; preds = %cont.thread, %cont
    %div3 = phi i32 [ poison, %cont.thread ], [ %div, %cont ]
    %call = tail call i32 (i32, ...) bitcast (i32 (...)* @_dev_err to i32 (i32, ...)*)(i32 noundef %div3) #3
    br label %if.end

This IR code is an IR that has not passed the optimization pass completely.
This code calculates the division only if `ravb_set_gti_ndev_rate` is non-zero and it calls `ubsan_handle_divrem_overflow` function to handle UB if `ravb_set_gti_ndev_rate` is zero.
So far, there is no warning. But a warning occurs when this code passes the SimpleCFG optimization.

  cont.thread:                                      ; preds = %entry
    tail call void @__ubsan_handle_divrem_overflow(i8* bitcast ({ { [12 x i8]*, i32, i32 }, { i16, i16, [15 x i8] }* }* @1 to i8*), i64 1000000000, i64 0) #3, !nosanitize !8
    unreachable
  
  cont:                                             ; preds = %entry
    %div = udiv i32 1000000000, %0
    store i32 %div, i32* @ravb_set_gti_ndev_inc, align 4, !tbaa !4
    %tobool.not = icmp ugt i32 %0, 1000000000
    br i1 %tobool.not, label %if.end, label %if.then
  
  if.then:                                          ; preds = %cont
    %call = tail call i32 (i32, ...) bitcast (i32 (...)* @_dev_err to i32 (i32, ...)*)(i32 noundef %div) #3
    br label %if.end
  
  if.end:                                           ; preds = %if.then, %cont
    ret void
  }

After it passes the SimplyCFG, the `br` instruction was changed to the `unreachable` instruction in `cont.thread` block.

This patch added noundef to the parameter of the `_dev_err` function, making the `%div3` unable to be Poison.
It is impossible to jump from the `cont.thread` block to `if.then` block, so `br` instruction was changed to `unreachable` instruction.
It would be nice to remove the unreachable block, but the above IR is not wrong because it is UB when `ravb_set_gti_ndev_rate` is zero.

There seems to be no existing problem in clang, and I think we can bypass this warning by adding a code that checks whether the `gravb_set_gti_ndev_rate` is zero or not as follows.

  int ravb_set_gti_ndev_rate;
  unsigned int ravb_set_gti_ndev_inc;
  void ravb_set_gti_ndev() {
    ravb_set_gti_ndev_inc = 1000000000;
    ravb_set_gti_ndev_inc = ravb_set_gti_ndev_inc / ravb_set_gti_ndev_rate;
    if (ravb_set_gti_ndev_rate != 0)
      if (ravb_set_gti_ndev_inc)
        _dev_err(ravb_set_gti_ndev_inc);
  }

@nathanchance How about changing the existing test code as above?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105169/new/

https://reviews.llvm.org/D105169