[DebugInfo][GVNSink] Wrong branch execution shown by debugger due to instruction sinking

[Apochens]

Please check this @SLTozer @dwblaikie

void func(int a, int *b) {
    if (a) {
        *b = 1;
    } else {
        *b = 1;
    }
}

int main() {
    int *b;
    func(0, b);
    return 0;
}

$ clang -g -S -emit-llvm -Xclang -disable-O0-optnone main.c -o main.ll
$ opt -S -passes=mem2reg,gvn-sink main.ll -o main-opt.ll
$ clang main-opt.ll -o a.out

$ mylldb a.out 
(lldb) target create "a.out"
Current executable set to '/data1/llvm-test/GVNSink/a.out' (x86_64).
(lldb) b func 
Breakpoint 1: where = a.out`func + 8 at gvnsink.c:2:9, address = 0x0000000000001138
(lldb) r
Process 2786226 launched: '/data1/llvm-test/GVNSink/a.out' (x86_64)
Process 2786226 stopped
* thread #1, name = 'a.out', stop reason = breakpoint 1.1
    frame #0: 0x0000555555555138 a.out`func(a=0, b=0x00007fffffffdac8) at gvnsink.c:2:9
   1    void func(int a, int *b) {
-> 2        if (a) {
   3            *b = 1;
   4        } else {
   5            *b = 1;
   6        }
   7    }
(lldb) s    
Process 2786226 stopped
* thread #1, name = 'a.out', stop reason = step in
    frame #0: 0x000055555555514f a.out`func(a=0, b=0x00007fffffffdac8) at gvnsink.c:3:12
   1    void func(int a, int *b) {
   2        if (a) {
-> 3            *b = 1;
   4        } else {
   5            *b = 1;
   6        }
   7    }
(lldb) v
(int) a = 0
(int *) b = 0x00007fffffffdac8
(lldb)

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: Shan Huang (Apochens)

Please check this @SLTozer @dwblaikie

void func(int a, int *b) {
    if (a) {
        *b = 1;
    } else {
        *b = 1;
    }
}

int main() {
    int *b;
    func(0, b);
    return 0;
}

$ clang -S -emit-llvm -Xclang -disable-O0-optnone main.c -o main.ll
$ opt -S -passes=mem2reg,gvn-sink main.ll -o main-opt.ll
$ clang main-opt.ll -o main

$ mylldb a.out 
(lldb) target create "a.out"
Current executable set to '/data1/llvm-test/GVNSink/a.out' (x86_64).
(lldb) b func 
Breakpoint 1: where = a.out`func + 8 at gvnsink.c:2:9, address = 0x0000000000001138
(lldb) r
Process 2786226 launched: '/data1/llvm-test/GVNSink/a.out' (x86_64)
Process 2786226 stopped
* thread #<!-- -->1, name = 'a.out', stop reason = breakpoint 1.1
    frame #<!-- -->0: 0x0000555555555138 a.out`func(a=0, b=0x00007fffffffdac8) at gvnsink.c:2:9
   1    void func(int a, int *b) {
-&gt; 2        if (a) {
   3            *b = 1;
   4        } else {
   5            *b = 1;
   6        }
   7    }
(lldb) s    
Process 2786226 stopped
* thread #<!-- -->1, name = 'a.out', stop reason = step in
    frame #<!-- -->0: 0x000055555555514f a.out`func(a=0, b=0x00007fffffffdac8) at gvnsink.c:3:12
   1    void func(int a, int *b) {
   2        if (a) {
-&gt; 3            *b = 1;
   4        } else {
   5            *b = 1;
   6        }
   7    }
(lldb) v
(int) a = 0
(int *) b = 0x00007fffffffdac8
(lldb)

[SLTozer]

Looks like a real bug to me - though this might be another case where we value preserving the memory access attribution over preventing the bad stepping behaviour, it's worth noting for if/when we can address these problems.

[Apochens]

This issue can be triggered even if the instruction is not a memory access. The sinking happens at this line.

Or maybe we can mitigate this by merging locations when the sinked instruction is not a memory access? @SLTozer

[dwblaikie]

If the instruction becomes reachable through a path it wasn't before, we should drop the location - load or not a load. As per https://llvm.org/docs/HowToUpdateDebugInfo.html

[Apochens]

@dwblaikie I agree with that, and I also found out some commits that update debug locations (especially merging and dropping) of memory access instructions (eg., load and store). And the concern related to sanitizers is also reasonable. If the consensus on updating is reached, I am willing to give a patch at any time.

[dwblaikie]

The concern related to sanitizers is reasonable - and it's a problem we'd like to solve, but with the tools (current DWARF representation, and subsequently the LLVM IR representation that maps to it) we have available, we are specifically not trying to solve the sanitizer case within that current representation. We should never produce "bonus coverage" (have a place reachable according to the line table, despite that line actually being unreachable due to an unsatisfied condition) - that's the formal rule we've adopted.

[jryans]

And the concern related to sanitizers is also reasonable

@Apochens, what do you mean about sanitizers here? I think I am maybe missing a bit of context, since @dwblaikie seems to know what you mean... 😅

[dwblaikie]

And the concern related to sanitizers is also reasonable

@Apochens, what do you mean about sanitizers here? I think I am maybe missing a bit of context, since @dwblaikie seems to know what you mean... 😅

Think we were discussing it on another bug - but for sanitizer failures, or crashes, and many other situations, it may be useful to know the original location of a problematic instruction even if the instruction was sunk/moved out into an execution path that doesn't (necessarily) execute the original block/location/condiiton. (like, if you got a "null dereference" failure without a location, that's unfortunate)

But I still contend that until we have a way to say that the location is speculated or otherwise not repreresntative of the control flow/conditions of the program, it'd be worse to show someone "null dereference on line 5" (when line 5 isn't executed).

[hiraditya]

Agreed with @dwblaikie, we should drop debug info in this case. Pretty misleading i'd say. I was able to repro on latest clang.

void func(int a, int *b) {
    if (a) {
        *b = 1;
    } else {
        *b = 1;
    }
}

int main() {
    int *b;
    func(0, b);
    func(1, b);
    return 0;
}

gdb will show the same line in both the cases.

Breakpoint 1, func (a=0, b=0x7fffffffd568) at g.cpp:2
2           if (a) {
(gdb) n
5               *b = 1;
(gdb) 
7       }
(gdb) 
main () at g.cpp:12
12          func(1, b);
(gdb) 

Breakpoint 1, func (a=1, b=0x7fffffffd568) at g.cpp:2
2           if (a) {
(gdb) 
4           } else {
(gdb) 
5               *b = 1;
(gdb) 

[dwblaikie]

Even weirder that it goes to the else on the true case and not the false case... But that's probably a different bug/questionable choice

[Apochens]

Since the non-determinism issue was addressed, I'd like to fix this misleading debug location. @dwblaikie @hiraditya