[DWARF] Incorrect prologue end line record.
ClosedPublic

Authored by CarlosAlbertoEnciso on Jan 5 2018, 4:34 AM.

Details

Summary

The prologue-end line record must be emitted after the last instruction that is part of the function frame setup code and before the instruction that marks the beginning of the function body.

For the below test:

1 extern int get_arg();
2 extern void func(int x);
3
4 int main()
5 {
6 int a;
7 func(get_arg());
8 }
9

The prologue-end line record is emitted with an incorrect associated address, which causes a debugger to show the beginning of function body to be inside the prologue.

This can be seen in the following trimmed assembler output:

main:
  ...
  .loc	1 7 0 prologue_end
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  callq	_Z7get_argv
  ...
  retq

The instruction 'pushq %rax' is part of the frame setup code.

The correct location for the prologue-end line information is just before the call to '_Z7get_argv', as illustrated in the following trimmed assembler output:

main:
  ...
  pushq	%rax
.Lcfi0:
  .cfi_def_cfa_offset 16
  .loc	1 7 0 prologue_end
  callq	_Z7get_argv
  ...
  retq

Notes:

  • This patch does not change the LLDB test cases that were failing with a previous patch.
  • As per reference, there are 2 previous related revisions:

https://reviews.llvm.org/D37625

Closed by commit rL313047
Reverted in rL313057 as it was causing buildbot failure (lldb inline stepping tests).

https://reviews.llvm.org/D39283

Patch with the required changes to the failing tests.
After some discussions with Tamas, it was clear that changing the test cases was not an optimal solution.
This revision should be abandoned.

Diff Detail

Repository
rL LLVM

Just a general comment: It is easier to review patches with more context. I usually upload the output of git diff HEAD~1 -U9999.

Is this an X86-only problem or is this a fix for a general problem on X86 only?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1172 ↗(On Diff #128717)

Can you explain why this is necessary / convince me that this is also true for shrink-wrapped code? I could imagine that for shrink-wrapped code, where the frame setup is sunk to the latest possible point it would still be useful to know accurate line numbers for code before the frame setup.

Just a general comment: It is easier to review patches with more context. I usually upload the output of git diff HEAD~1 -U9999.

Thanks very much for your suggestion. Would you like for me to update te patch?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1172 ↗(On Diff #128717)

I will have a look at it. Thanks.

Due to the uploaded patch, the following test case fails:

llvm/test/DebugInfo/COFF/types-array.ll

Before the patch:

# %bb.0:                                # %entry
	pushl	%ebp
	.cv_fpo_pushreg	%ebp
	movl	%esp, %ebp
	.cv_fpo_setframe	%ebp
Ltmp0:
	subl	$20, %esp
	.cv_loc	0 1 3 7                 # t.cpp:3:7
	movl	"?a@?1??f@@YAXXZ@3PAHA"+16, %eax

After the patch:

# %bb.0:                                # %entry
	pushl	%ebp
	.cv_fpo_pushreg	%ebp
	movl	%esp, %ebp
	.cv_fpo_setframe	%ebp
	subl	$20, %esp
Ltmp0:
	.cv_loc	0 1 3 7                 # t.cpp:3:7
	movl	"?a@?1??f@@YAXXZ@3PAHA"+16, %eax

The reason for the failure is because now the frame setup code is correct. The instruction

subl	$20, %esp

now is part of the prologue. Before the patch it was the first instruction of the function body.

The new patch will include the modified test case.

CarlosAlbertoEnciso marked an inline comment as done.Jan 15 2018, 4:30 AM

As the patch is intended to fix the invalid prologue-end records, the added check is looking for instructions associated with the frame setup: MachineInstr::FrameSetup flag.

When generating Debug Location (line records) at the start of an instruction, that check stops emitting a line record for instructions with no correspondence to user code, otherwise a normal line record will be emitted.

In the case of 'shrink-wrapped code', the check is valid, as instructions before the frame setup will have correspondence to user code, hence accurate line records will be emitted.

Essentially line records for instructions that are not part of the epilogue/prologue are not affected by this change.

Apologies as the original test case for the patch was incorrect, as I uploaded an old test case.

This update contains:

Fix for a failed test case: llvm/test/DebugInfo/COFF/types-array.ll
Correct test case for the patch.

In the case of 'shrink-wrapped code', the check is valid, as instructions before the frame setup will have correspondence to user code, hence accurate line records will be emitted.

This explanation does not validate against my mental model of shrink-wrapping. It's possible that my mental model is incorrect, but my understanding is that shrink-wrapping defers the frame setup until the first instruction that needs access to something in the frame. But that does not imply that there is no correspondence to user code before the frame setup is complete.

But that does not imply that there is no correspondence to user code before the frame setup is complete.

Adrian is correct, there could be a correspondence to user code before the frame setup is complete.

There could be a correspondence to user code before the frame setup is complete.

Hi Adrian & Quentin,

Thanks very much for your comments. Now I can see your point of concern.

I will try to create a test case using the ones committed with the changes to support 'shrink-wrapped code'.

Hi Adrian & Quentin,

For the below test

extern int doSomething(int a,int *b);

int foo(int a,int b)
{
  int tmp4 = a;
  if (a < b) {
    int tmp = a;
    tmp4 = doSomething(0,&tmp);
  }

  return tmp4;
}

that generates similar IR as the first test case from

llvm/test/CodeGen/X86/x86-shrink-wrapping.ll

and generating only debug line table

define i32 @_Z3fooii(i32 %a, i32 %b) !dbg !7 {
entry:
  %tmp = alloca i32, align 4
  %cmp = icmp slt i32 %a, %b, !dbg !9
  br i1 %cmp, label %if.then, label %if.end, !dbg !10

if.then:                                          ; preds = %entry
  store i32 %a, i32* %tmp, align 4, !dbg !11
  %call = call i32 @_Z11doSomethingiPi(i32 0, i32* %tmp), !dbg !12
  br label %if.end, !dbg !13

if.end:                                           ; preds = %if.then, %entry
  %tmp4.0 = phi i32 [ %call, %if.then ], [ %a, %entry ]
  ret i32 %tmp4.0, !dbg !14
}

declare i32 @_Z11doSomethingiPi(i32, i32*)
...

The line records generated with '-enable-shrink-wrap=false/true' are the same.

Hi Adrian & Quentin,

For the below test (which combines the original test case used to reproduce the invalid prologue end issue)

extern int doSomething(int a,int *b);
extern int getArg();

int foo(int a,int b)
{
  int tmp4 = a;
  if (a < b) {
    int tmp = a;
    tmp4 = doSomething(getArg(),&tmp);
  }

  return tmp4;
}

that generates similar IR as the first test case from

llvm/test/CodeGen/X86/x86-shrink-wrapping.ll

and generating only debug line table

define i32 @_Z3fooii(i32 %a, i32 %b) !dbg !7 {
entry:
  %tmp = alloca i32, align 4
  %cmp = icmp slt i32 %a, %b, !dbg !9
  br i1 %cmp, label %if.then, label %if.end, !dbg !10

if.then:                                          ; preds = %entry
  store i32 %a, i32* %tmp, align 4, !dbg !11
  %call = call i32 @_Z6getArgv(), !dbg !12
  %call1 = call i32 @_Z11doSomethingiPi(i32 %call, i32* %tmp), !dbg !13
  br label %if.end, !dbg !14

if.end:                                           ; preds = %if.then, %entry
  %tmp4.0 = phi i32 [ %call1, %if.then ], [ %a, %entry ]
  ret i32 %tmp4.0, !dbg !15
}

declare i32 @_Z11doSomethingiPi(i32, i32*)
declare i32 @_Z6getArgv()

The line records generated with '-enable-shrink-wrap=false/true' are the same.

Hi Adrian & Quentin,

For the below test (which combines the original test case used to reproduce the invalid prologue end issue)

extern int doSomething(int a,int *b);
extern int getArg();

int foo(int a,int b)
{
  int tmp4 = a;
  if (a < b) {
    int tmp = a;
    tmp4 = doSomething(getArg(),&tmp);
  }

  return tmp4;
}

that generates similar IR as the first test case from

llvm/test/CodeGen/X86/x86-shrink-wrapping.ll

and generating only debug line table

define i32 @_Z3fooii(i32 %a, i32 %b) !dbg !7 {
entry:
  %tmp = alloca i32, align 4
  %cmp = icmp slt i32 %a, %b, !dbg !9
  br i1 %cmp, label %if.then, label %if.end, !dbg !10

if.then:                                          ; preds = %entry
  store i32 %a, i32* %tmp, align 4, !dbg !11
  %call = call i32 @_Z6getArgv(), !dbg !12
  %call1 = call i32 @_Z11doSomethingiPi(i32 %call, i32* %tmp), !dbg !13
  br label %if.end, !dbg !14

if.end:                                           ; preds = %if.then, %entry
  %tmp4.0 = phi i32 [ %call1, %if.then ], [ %a, %entry ]
  ret i32 %tmp4.0, !dbg !15
}

declare i32 @_Z11doSomethingiPi(i32, i32*)
declare i32 @_Z6getArgv()

The line records generated with '-enable-shrink-wrap=false/true' are the same.

Hi Quentin,

If you have a more complex test case (from your work on shrink-wrapping code', that I can use to test the changes when the '-enable-shrink-wrap' option is enable, it would be a great help.

Thanks
-Carlos

Hi Adrian & Quentin,

These are the line records output (llvm-dwarfdump):

First test case: -enable-shrink-wrap=false

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      4      0      1   0             0  is_stmt
0x0000000000000003      6      9      1   0             0  is_stmt prologue_end
0x0000000000000005      6      7      1   0             0 
0x0000000000000007      7      9      1   0             0  is_stmt
0x0000000000000010      8     12      1   0             0  is_stmt
0x0000000000000017     11      3      1   0             0  is_stmt
0x0000000000000019     11      3      1   0             0  is_stmt end_sequence

First test case: -enable-shrink-wrap=true

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      4      0      1   0             0  is_stmt
0x0000000000000002      6      9      1   0             0  is_stmt prologue_end
0x0000000000000004      6      7      1   0             0 
0x0000000000000007      7      9      1   0             0  is_stmt
0x0000000000000010      8     12      1   0             0  is_stmt
0x000000000000001b     11      3      1   0             0  is_stmt
0x000000000000001c     11      3      1   0             0  is_stmt end_sequence

Second test case: -enable-shrink-wrap=false

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      5      0      1   0             0  is_stmt
0x0000000000000003      7      9      1   0             0  is_stmt prologue_end
0x0000000000000005      7      7      1   0             0 
0x0000000000000007      8      9      1   0             0  is_stmt
0x000000000000000b      9     24      1   0             0  is_stmt
0x0000000000000015      9     12      1   0             0 
0x000000000000001c     12      3      1   0             0  is_stmt
0x000000000000001e     12      3      1   0             0  is_stmt end_sequence

Second test case: -enable-shrink-wrap=true

Address            Line   Column File   ISA Discriminator Flags
------------------ ------ ------ ------ --- ------------- -------------
0x0000000000000000      5      0      1   0             0  is_stmt
0x0000000000000002      7      9      1   0             0  is_stmt prologue_end
0x0000000000000004      7      7      1   0             0 
0x0000000000000007      8      9      1   0             0  is_stmt
0x000000000000000b      9     24      1   0             0  is_stmt
0x0000000000000015      9     12      1   0             0 
0x0000000000000020     12      3      1   0             0  is_stmt
0x0000000000000021     12      3      1   0             0  is_stmt end_sequence

Thanks

Hi Carlos,

Does shrink-wrapping change the assembly in your examples?
If you want to have more examples, you can take a look at test/CodeGen/X86/x86-shrink-wrapping.ll and/or run the llvm test-suite with and without the shrink-wrapping option and look at the diffs.

Cheers,
-Quentin

Hi Quentin,

Does shrink-wrapping change the assembly in your examples?

Yes; it does. I have copied edited assembly (removed some temporal labels and debug info).

First test case:
-enable-shrink-wrap=false

# %bb.0:                        # %entry
	pushq	%rax
	movl	%edi, %eax
	cmpl	%esi, %eax
	jge	.LBB0_2
# %bb.1:                        # %if.then
	movl	%eax, 4(%rsp)
	leaq	4(%rsp), %rsi
	xorl	%edi, %edi
	callq	_Z11doSomethingiPi
.LBB0_2:                        # %if.end
	popq	%rcx
	retq

-enable-shrink-wrap=true

# %bb.0:                        # %entry
	movl	%edi, %eax
	cmpl	%esi, %eax
	jge	.LBB0_2
# %bb.1:                        # %if.then
	pushq	%rax
	movl	%eax, 4(%rsp)
	leaq	4(%rsp), %rsi
	xorl	%edi, %edi
	callq	_Z11doSomethingiPi
	addq	$8, %rsp
.LBB0_2:                        # %if.end
	retq

The following instructions have changed and moved:

'pushq	%rax'             move from %bb.0 to %bb.1
'popq	%rcx'             changed into  'addq	$8, %rsp',

Second test case:
-enable-shrink-wrap=false

# %bb.0:                                # %entry
pushq	%rax
	movl	%edi, %eax
	cmpl	%esi, %eax
	jge	.LBB0_2
# %bb.1:                                # %if.then
	movl	%eax, 4(%rsp)
	callq	_Z6getArgv
	leaq	4(%rsp), %rsi
	movl	%eax, %edi
	callq	_Z11doSomethingiPi
.LBB0_2:                                # %if.end
	popq	%rcx
	retq

-enable-shrink-wrap=true

# %bb.0:                                # %entry
	movl	%edi, %eax
	cmpl	%esi, %eax
	jge	.LBB0_2
# %bb.1:                                # %if.then
	pushq	%rax
	movl	%eax, 4(%rsp)
	callq	_Z6getArgv
	leaq	4(%rsp), %rsi
	movl	%eax, %edi
	callq	_Z11doSomethingiPi
	addq	$8, %rsp
.LBB0_2:                                # %if.end
	retq

The following instructions have changed and moved:

'pushq	%rax'             moved from %bb.0 to %bb.1
'popq	%rcx'             changed into  'addq	$8, %rsp'

Hi Quentin,

If you want to have more examples, you can take a look at test/CodeGen/X86/x86-shrink-wrapping.ll and/or run the llvm test-suite with and without the shrink-wrapping option and look at the diffs.

I will take a further look at the referenced test case and run the llvm test-suite. I will let you know the results.

Thanks,
Carlos

Hi Quentin,

run the llvm test-suite with and without the shrink-wrapping option and look at the diffs.

I forced the '-enable-shring-wrap' to be true/false and run the 'check-llvm' and 'check-lldb' tests.

The output from 'check-lldb' are the same.

There are some differences in the output from 'check-llvm' and corresponds to failures due to test cases expecting a specific value for the flag.

I am setting up the llvm test-suite, so I can run it. I would let you know the results.

Thanks,
Carlos

Hi Quentin,

run the llvm test-suite with and without the shrink-wrapping option and look at the diffs.

I managed to setup the llvm test-suite and the results are the same. There is only 1 failure regardless of the option setting.

Failing Tests (1):
  test-suite :: SingleSource/UnitTests/ms_struct_pack_layout.test

The same failure is present without my changes.

Thanks,
Carlos

Ping.

Thanks,
Carlos

qcolombet resigned from this revision.Feb 8 2018, 9:28 AM

For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).

For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).

Hi Quentin,

Thanks very much for your help and feedback.

Carlos

Hi Adrian,

For the review itself, I leave this to @aprantl. I was just providing insights on the shrink-wrapping question :).

Do you have any additional concerns or comments in relation to the shrink-wrapping, that I have not covered with Quentin?

Thanks,
Carlos

aprantl accepted this revision.Feb 13 2018, 8:56 AM

Thanks for posting the examples and for doing due diligence on shrink wrapping! IIUC, enabling/disabling shrink-wrapping does not move the prologue_end, so this LGTM.

This revision is now accepted and ready to land.Feb 13 2018, 8:56 AM

IIUC, enabling/disabling shrink-wrapping does not move the prologue_end, so this LGTM.

If I understand the code correctly, in case of shrink-wrapping kicking in, prologue_end will be at the beginning of the function, while without shrink-wrapping, it will be at the end of the prologue. I believe that's correct, right?

$ cat foo.c
void foo(int *);

int main(int argc, char const *argv[])
{
  if (argc > 109) {
    int j = 30;
    foo(&j);
    return j;
  }
  return 0;
}

$ clang -O1 -S -g -o - foo.c
_main:                                  ## @main
        [...]
## %bb.0:                               ## %entry
	xorl	%eax, %eax
Ltmp0:
	.loc	1 5 12 prologue_end     ## foo.c:5:12
	cmpl	$110, %edi
	[...]
	jl	LBB0_2
## %bb.1:                               ## %if.then
        [...]
	pushq	%rbp
	[...]
	popq	%rbp
LBB0_2:                                 ## %return
	[...]
	retq

$ clang -O1 -S -g -o - foo.c -mllvm -enable-shrink-wrap=false
_main:                                  ## @main
	[...]
## %bb.0:                               ## %entry
	pushq	%rbp
        [.cfi...]
	movq	%rsp, %rbp
	subq	$16, %rsp
	[...]
	xorl	%eax, %eax
Ltmp0:
	.loc	1 5 12 prologue_end     ## foo.c:5:12
	cmpl	$110, %edi
	[...]
	jl	LBB0_2
Ltmp2:
## %bb.1:                               ## %if.then
	[...]
LBB0_2:                                 ## %return
	[...]
	popq	%rbp
	retq

IIUC, enabling/disabling shrink-wrapping does not move the prologue_end, so this LGTM.

If I understand the code correctly, in case of shrink-wrapping kicking in, prologue_end will be at the beginning of the function, while without shrink-wrapping, it will be at the end of the prologue. I believe that's correct, right?

Hi,

I tried your given test case with the same command line options and this is the assembler generated for both cases:

clang -O1 -S -g -o - foo.c

main:                           # @main
  [...]
# %bb.0:                        # %entry
  xorl	%eax, %eax
.Ltmp0:
  .loc	1 5 12 prologue_end     # foo.c:5:12
  cmpl	$110, %edi
.Ltmp1:
  .loc	1 5 7 is_stmt 0         # foo.c:5:7
  jl	.LBB0_2
.Ltmp2:
# %bb.1:                        # %if.then
  pushq	%rax
  .cfi_def_cfa_offset 16
.Ltmp3:
  .loc	1 6 9 is_stmt 1         # foo.c:6:9
  movl	$30, 4(%rsp)
  leaq	4(%rsp), %rdi
.Ltmp4:
  .loc	1 7 5                   # foo.c:7:5
  callq	foo
.Ltmp5:
  .loc	1 8 12                  # foo.c:8:12
  movl	4(%rsp), %eax
.Ltmp6:
  .loc	1 0 12 is_stmt 0        # foo.c:0:12
  addq	$8, %rsp
.Ltmp7:
.LBB0_2:                        # %return
  .loc	1 11 1 is_stmt 1        # foo.c:11:1
  retq

clang -O1 -S -g -o - foo.c -mllvm -enable-shrink-wrap=false

main:                           # @main
  [...]
# %bb.0:                        # %entry
  pushq	%rax
  .cfi_def_cfa_offset 16
  xorl	%eax, %eax
.Ltmp0:
  .loc	1 5 12 prologue_end     # foo.c:5:12
  cmpl	$110, %edi
.Ltmp1:
  .loc	1 5 7 is_stmt 0         # foo.c:5:7
  jl	.LBB0_2
.Ltmp2:
# %bb.1:                        # %if.then
  .loc	1 6 9 is_stmt 1         # foo.c:6:9
  movl	$30, 4(%rsp)
  leaq	4(%rsp), %rdi
.Ltmp3:
  .loc	1 7 5                   # foo.c:7:5
  callq	foo
.Ltmp4:
  .loc	1 8 12                  # foo.c:8:12
  movl	4(%rsp), %eax
.Ltmp5:
.LBB0_2:                        # %return
  .loc	1 11 1                  # foo.c:11:1
  popq	%rcx
  retq

I noticed that the code generation does not look similar to your posting specially for the case when -enable-shrink-wrap=false.

May be we are using different compilers. My version is revision 325102 plus the submitted changes for the review.

Thanks,
Carlos

Thanks for posting the examples and for doing due diligence on shrink wrapping! IIUC, enabling/disabling shrink-wrapping does not move the prologue_end, so this LGTM.

Hi Adrian,

You are correct. Enabling/disabling shrink-wrapping does not move the prologue_end debug records.

Thanks very much for your suggestions, feedback and review.

Carlos

IIUC, enabling/disabling shrink-wrapping does not move the prologue_end, so this LGTM.

If I understand the code correctly, in case of shrink-wrapping kicking in, prologue_end will be at the beginning of the function, while without shrink-wrapping, it will be at the end of the prologue. I believe that's correct, right?

Hi,

I tried your given test case with the same command line options and this is the assembler generated for both cases:

The difference here is the push %rax.

clang -O1 -S -g -o - foo.c

In this case, the push (the prologue) is in bb.1, after the cmp.

clang -O1 -S -g -o - foo.c -mllvm -enable-shrink-wrap=false

In this case, the push (the prologue) is in bb.0, before the cmp.

I noticed that the code generation does not look similar to your posting specially for the case when -enable-shrink-wrap=false.

Yes, sorry, you should be able to achieve the same result with -fno-omit-frame-pointer in both commands.

May be we are using different compilers. My version is revision 325102 plus the submitted changes for the review.

My bad, I should have provided the correct triple: -target x86_64-apple-darwin.

I noticed that the code generation does not look similar to your posting specially for the case when -enable-shrink-wrap=false.

Yes, sorry, you should be able to achieve the same result with -fno-omit-frame-pointer in both commands.
My bad, I should have provided the correct triple: -target x86_64-apple-darwin.

Now using those extra options, I am be able to achieve the same results.

Thanks

The difference here is the push %rax.

clang -O1 -S -g -o - foo.c

In this case, the push (the prologue) is in bb.1, after the cmp.

clang -O1 -S -g -o - foo.c -mllvm -enable-shrink-wrap=false

In this case, the push (the prologue) is in bb.0, before the cmp.

Hi,

I think the following links describe your findings.

https://reviews.llvm.org/D41762#990580
https://reviews.llvm.org/D41762#990589
https://reviews.llvm.org/D41762#990611

The assembler lines change, but the debug line records remain the same.

Thanks,
Carlos

Thanks @CarlosAlbertoEnciso, I finally understood the original concern about shrink-wrapping. LGTM.

This revision was automatically updated to reflect the committed changes.

Thanks @CarlosAlbertoEnciso, I finally understood the original concern about shrink-wrapping. LGTM.

Thanks @thegameg for your review.