zig icon indicating copy to clipboard operation
zig copied to clipboard

llvm: Fix line number information to prevent spurious breakpoint hits

Open tau-dev opened this issue 1 year ago • 5 comments

Fixes #19443. Reset the line number to its previous state after genBodyDebugScope, so that instructions between its end and the next dbg_stmt do not leak inside the block. This core fix is very simple, but surfaces a secondary issue: the LLVM codegen generates blocks in a different order than Clang would for the same CFG in C, which ultimately results in GDB skipping the loop header altogether (see comments for some more details). So we reorder some basic blocks too.

tau-dev avatar Jul 08 '24 21:07 tau-dev

Is patching Builder really the correct solution here? I think the usage code, if anything, should be updated. Also I'm not convinced the order of basic blocks is the problem.

andrewrk avatar Jul 21 '24 08:07 andrewrk

Also I'm not convinced the order of basic blocks is the problem.

If you move the placeBlock so that we get the same order of blocks again as we have in status quo:

--- /tmp/llvm.zig       2024-07-21 18:02:01.645028639 +0200
+++ src/codegen/llvm.zig        2024-07-21 18:01:12.664540800 +0200
@@ -5962,10 +5962,10 @@
             .breaks = &breaks,
         });
         defer assert(self.blocks.remove(inst));
+        try self.wip.placeBlock(parent_bb);

         try self.genBodyDebugScope(maybe_inline_func, body);

-        try self.wip.placeBlock(parent_bb);
         self.wip.cursor = .{ .block = parent_bb };

         // Create a phi node only if the block returns a value.

an then, for example, try stepping through a for loop—this is my test code:

export fn foo() u8 {
    const s = [3]u32{4,5,6};

    var x: u8 = 1;
    for (s) |c| {
        if (c == 5)
            x = 2;
        x = 3;
    }
    return x;
}

—then you can see that GDB doesn't stop on the for (s) |c| { line anymore after the first iteration. You can see the reason for this if you compare the LLVM and objdump of this function

with correct block order
define dso_local zeroext i8 @foo() #0 !dbg !6 {
Entry:
  %0 = alloca i32, align 4
  %1 = alloca [8 x i8], align 8
  %2 = alloca [1 x i8], align 1
  call void @llvm.dbg.declare(metadata ptr @0, metadata !8, metadata !DIExpression()), !dbg !7
  store i8 1, ptr %2, align 1, !dbg !9
  call void @llvm.dbg.declare(metadata ptr %2, metadata !10, metadata !DIExpression()), !dbg !9
  store i64 0, ptr %1, align 8, !dbg !9
  br label %Loop, !dbg !11

Loop:
  %3 = load i64, ptr %1, align 8, !dbg !12
  %4 = icmp ult i64 %3, 3, !dbg !13
  br i1 %4, label %Then, label %Else, !dbg !13

Then:
  %5 = getelementptr inbounds [3 x i32], ptr @0, i64 0, i64 %3, !dbg !14
  %6 = load i32, ptr %5, !dbg !14
  store i32 %6, ptr %0, align 4, !dbg !14
  call void @llvm.dbg.declare(metadata ptr %0, metadata !15, metadata !DIExpression()), !dbg !14
  %7 = icmp eq i32 %6, 5, !dbg !16
  br i1 %7, label %Then1, label %Else1, !dbg !16

Else:
  br label %Block2, !dbg !14

Then1:
  store i8 2, ptr %2, align 1, !dbg !17
  br label %Block, !dbg !17

Else1:
  br label %Block, !dbg !18

Block:
  store i8 3, ptr %2, align 1, !dbg !19
  br label %Block1, !dbg !19

Block1:
  %8 = add nuw i64 %3, 1, !dbg !12
  store i64 %8, ptr %1, align 8, !dbg !12
  br label %Loop, !dbg !11

Block2:
  %9 = load i8, ptr %2, align 1, !dbg !20
  ret i8 %9, !dbg !20
}
export fn foo() u8 {
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 83 ec 20             sub    $0x20,%rsp
    const s = [3]u32{4,5,6};

    var x: u8 = 1;
   8:   c6 45 ef 01             movb   $0x1,-0x11(%rbp)
   c:   48 c7 45 f0 00 00 00    movq   $0x0,-0x10(%rbp)
  13:   00 
    for (s) |c| {
  14:   48 8b 45 f0             mov    -0x10(%rbp),%rax
  18:   48 89 45 e0             mov    %rax,-0x20(%rbp)
  1c:   48 83 f8 03             cmp    $0x3,%rax
  20:   73 18                   jae    3a <foo+0x3a>
  22:   48 8b 4d e0             mov    -0x20(%rbp),%rcx
  26:   48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 2d <foo+0x2d>
  2d:   8b 04 88                mov    (%rax,%rcx,4),%eax
  30:   89 45 fc                mov    %eax,-0x4(%rbp)
        if (c == 5)
  33:   83 f8 05                cmp    $0x5,%eax
  36:   74 04                   je     3c <foo+0x3c>
  38:   eb 08                   jmp    42 <foo+0x42>
    for (s) |c| {
  3a:   eb 1a                   jmp    56 <foo+0x56>
            x = 2;
  3c:   c6 45 ef 02             movb   $0x2,-0x11(%rbp)
  40:   eb 02                   jmp    44 <foo+0x44>
        if (c == 5)
  42:   eb 00                   jmp    44 <foo+0x44>
        x = 3;
  44:   c6 45 ef 03             movb   $0x3,-0x11(%rbp)
  48:   48 8b 45 e0             mov    -0x20(%rbp),%rax
    for (s) |c| {
  4c:   48 83 c0 01             add    $0x1,%rax
  50:   48 89 45 f0             mov    %rax,-0x10(%rbp)
  54:   eb be                   jmp    14 <foo+0x14>
    }
    return x;
  56:   0f b6 45 ef             movzbl -0x11(%rbp),%eax
  5a:   48 83 c4 20             add    $0x20,%rsp
  5e:   5d                      pop    %rbp
  5f:   c3                      ret
and without
define dso_local zeroext i8 @foo() #0 !dbg !6 {
Entry:
  %0 = alloca i32, align 4
  %1 = alloca [8 x i8], align 8
  %2 = alloca [1 x i8], align 1
  call void @llvm.dbg.declare(metadata ptr @0, metadata !8, metadata !DIExpression()), !dbg !7
  store i8 1, ptr %2, align 1, !dbg !9
  call void @llvm.dbg.declare(metadata ptr %2, metadata !10, metadata !DIExpression()), !dbg !9
  store i64 0, ptr %1, align 8, !dbg !9
  br label %Loop, !dbg !11

Block:
  %3 = load i8, ptr %2, align 1, !dbg !12
  ret i8 %3, !dbg !12

Loop:
  %4 = load i64, ptr %1, align 8, !dbg !13
  %5 = icmp ult i64 %4, 3, !dbg !14
  br i1 %5, label %Then, label %Else, !dbg !14

Block1:
  %6 = add nuw i64 %4, 1, !dbg !13
  store i64 %6, ptr %1, align 8, !dbg !13
  br label %Loop, !dbg !11

Then:
  %7 = getelementptr inbounds [3 x i32], ptr @0, i64 0, i64 %4, !dbg !15
  %8 = load i32, ptr %7, !dbg !15
  store i32 %8, ptr %0, align 4, !dbg !15
  call void @llvm.dbg.declare(metadata ptr %0, metadata !16, metadata !DIExpression()), !dbg !15
  %9 = icmp eq i32 %8, 5, !dbg !17
  br i1 %9, label %Then1, label %Else1, !dbg !17

Else:
  br label %Block, !dbg !15

Block2:
  store i8 3, ptr %2, align 1, !dbg !18
  br label %Block1, !dbg !18

Then1:
  store i8 2, ptr %2, align 1, !dbg !19
  br label %Block2, !dbg !19

Else1:
  br label %Block2, !dbg !20
}
export fn foo() u8 {
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 83 ec 20             sub    $0x20,%rsp
    const s = [3]u32{4,5,6};

    var x: u8 = 1;
   8:   c6 45 ef 01             movb   $0x1,-0x11(%rbp)
   c:   48 c7 45 f0 00 00 00    movq   $0x0,-0x10(%rbp)
  13:   00 
    for (s) |c| {
  14:   eb 0a                   jmp    20 <foo+0x20>
        if (c == 5)
            x = 2;
        x = 3;
    }
    return x;
  16:   0f b6 45 ef             movzbl -0x11(%rbp),%eax
  1a:   48 83 c4 20             add    $0x20,%rsp
  1e:   5d                      pop    %rbp
  1f:   c3                      ret
    for (s) |c| {
  20:   48 8b 45 f0             mov    -0x10(%rbp),%rax
  24:   48 89 45 e0             mov    %rax,-0x20(%rbp)
  28:   48 83 f8 03             cmp    $0x3,%rax
  2c:   72 10                   jb     3e <foo+0x3e>
  2e:   eb 26                   jmp    56 <foo+0x56>
  30:   48 8b 45 e0             mov    -0x20(%rbp),%rax
  34:   48 83 c0 01             add    $0x1,%rax
  38:   48 89 45 f0             mov    %rax,-0x10(%rbp)
  3c:   eb e2                   jmp    20 <foo+0x20>
  3e:   48 8b 4d e0             mov    -0x20(%rbp),%rcx
  42:   48 8d 05 00 00 00 00    lea    0x0(%rip),%rax        # 49 <foo+0x49>
  49:   8b 04 88                mov    (%rax,%rcx,4),%eax
  4c:   89 45 fc                mov    %eax,-0x4(%rbp)
        if (c == 5)
  4f:   83 f8 05                cmp    $0x5,%eax
  52:   74 0a                   je     5e <foo+0x5e>
  54:   eb 0e                   jmp    64 <foo+0x64>
    for (s) |c| {
  56:   eb be                   jmp    16 <foo+0x16>
        x = 3;
  58:   c6 45 ef 03             movb   $0x3,-0x11(%rbp)
  5c:   eb d2                   jmp    30 <foo+0x30>
            x = 2;
  5e:   c6 45 ef 02             movb   $0x2,-0x11(%rbp)
  62:   eb f4                   jmp    58 <foo+0x58>
        if (c == 5)
  64:   eb f2                   jmp    58 <foo+0x58>

to GDB's stepping logic, which only stops on the first instruction of a new line. Took me some head-scratching to figure that one out : )

How could I influence the block ordering without touching the Builder?

tau-dev avatar Jul 21 '24 16:07 tau-dev

I think it would be informative to look at it in terms of the LLVM IR that is being generated.

andrewrk avatar Jul 21 '24 20:07 andrewrk

The LLVM is the same, differing only in the order of the blocks. Hang on a sec, I can show you too...

tau-dev avatar Jul 21 '24 20:07 tau-dev

Updated the details tags in my previous comment.

tau-dev avatar Jul 21 '24 21:07 tau-dev

@jacobly0 If you are referring to the remapped_index field, we could get rid of that by doing a remapping scan over all instructions in finish(); would that help?

doesn't emit equivalent textual llvm IR

Equivalent to what?

tau-dev avatar Mar 08 '25 14:03 tau-dev