Fix debug_loc offset difference with basic block sections

[tmsriram]

Summary

debug_loc for params not used in entry block need to be computed with the section start as the base label.

The attached test will not assemble without the patch as the assembler will complain that we are trying to compute a label difference from 2 different sections.

If the basic block containing the first mention of the param is not in the same section as the entry basic block, we cannot use the CurrentFnBegin label. To fix it, we use the Section begin label instead. Using the Section Label will result in loss of debug loc fidelity as we do not capture the basic blocks before the start of this section.

Right now, LabelsBeforeInsn just stores one label for each insn. We can fix the fidelity problem by storing all the basic block section labels that appear before the insn and generating a difference for each of them. Hence, I have made this as a FIXME.

Is this alright? I am not very familiar with debug_loc, other better suggestions are much appreciated here.

[dblaikie]

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

[tmsriram]

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

Yes, you are right. The debug_loc section is not generated without basic block sections, I will dig more.

[tmsriram]

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

I see why debug_loc is not produced in the default case. This is what happens:

+ For the default case (no bbsections), DebugLoc in https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1744 , it has exactly one value that contains the whole function scope as the start label is lfunc_begin and the end label is the end of the function.
+ Such location lists are handled specially here when the scope is the whole function : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841 and the debug_loc is not generated. Diffing the asm by forcing a debug_loc entry, what happens here is that a DW_AT_location is replaced with a DW_AT_const_value. I don't understand this too well but this seems like an optimization to me.

Does this make sense?

[dblaikie]

In D85085#2192340, @tmsriram wrote:

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

I see why debug_loc is not produced in the default case. This is what happens:

+ For the default case (no bbsections), DebugLoc in https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1744 , it has exactly one value that contains the whole function scope as the start label is lfunc_begin and the end label is the end of the function.
+ Such location lists are handled specially here when the scope is the whole function : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841 and the debug_loc is not generated.

OK ^ and with bb sections do we see the same behavior? If not, why not?

Diffing the asm by forcing a debug_loc entry,

Not sure what you mean by this ^ could you explain further?

what happens here is that a DW_AT_location is replaced with a DW_AT_const_value. I don't understand this too well but this seems like an optimization to me.

Does this make sense?

I'm still a bit confused. I think if non-bb-sections doesn't need a location list, then bb-sections shouldn't either. Perhaps that issue should be fixed first, if it's an issue?

In any case/any order - perhaps this patch is still reflective of a bug that exists even in this ^ issue is fixed, but maybe it's that the test case isn't sufficient to demonstrate the remaining bug - perhaps the test case could be complicated a bit further to ensure it's not testing a behavior that shouldn't be happening for other reasons anyway?

[tmsriram]

In D85085#2194130, @dblaikie wrote:

In D85085#2192340, @tmsriram wrote:

In D85085#2191679, @dblaikie wrote:

Trying to reproduce this (by taking the .ll test case and compiling it with clang -c) I don't get any debug_loc section or DW_AT_locations. Is there something I'm missing about how to reproduce this?

(trying to see what the non-bb-sections location description looks like - if it depends on basic block ordering, it's probably incorrect anyway, and might be worth fixing so it doesn't depend on that non-guaranteed property & then there'll be less divergence between bb-sections and not)

I see why debug_loc is not produced in the default case. This is what happens:

+ For the default case (no bbsections), DebugLoc in https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1744 , it has exactly one value that contains the whole function scope as the start label is lfunc_begin and the end label is the end of the function.
+ Such location lists are handled specially here when the scope is the whole function : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841 and the debug_loc is not generated.

OK ^ and with bb sections do we see the same behavior? If not, why not?

Please bear with my longish response here:

First, let me explain what happens with the test by default. There is no .debug_loc section by default and here is why:

Without bbsections, the ASM looks like this:

	.text
	.file	"locdspnm.cpp"
...
	.type	_ZL4ncatPcjz,@function
_ZL4ncatPcjz:                           # @_ZL4ncatPcjz
.Lfunc_begin0:
	.file	3 "/proc/self/cwd/third_party/icu/source/common/locdspnm.cpp"
	.loc	3 37 0                          # third_party/icu/source/common/locdspnm.cpp:37:0
	.cfi_startproc
# %bb.0:                                # %.critedge3
	subq	$56, %rsp
...
	je	.LBB0_2
# %bb.1:                                # %.critedge3
...
	movaps	%xmm7, 32(%rsp)
.LBB0_2:                                # %.critedge3
...
	leaq	-128(%rsp), %rax
	#DEBUG_VALUE: ncat:buflen <- 157
.Ltmp0:
	.loc	3 47 3 prologue_end             # third_party/icu/source/common/locdspnm.cpp:47:3
...
	movl	$16, (%rax)
.Ltmp1:
	.p2align	4, 0x90
.LBB0_3:                                # =>This Inner Loop Header: Depth=1
	#DEBUG_VALUE: ncat:buflen <- 157
	.loc	3 0 3 is_stmt 0                 # third_party/icu/source/common/locdspnm.cpp:0:3
	jmp	.LBB0_3
.Lfunc_end0:
	.size	_ZL4ncatPcjz, .Lfunc_end0-_ZL4ncatPcjz
	.cfi_endproc

The function buildLocationList : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1638 is called and it adds two entries to DebugLoc with StartLabel and Label pairs as follows:

1. .Lfunc_begin0, .Ltmp1
2. .Ltmp1, .Lfunc_end0

The size of DebugLoc is 2 but these two ranges are merged into one range with MergeRanges here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1733

There is now one entry in DebugLoc with full function scope .Lfunc_begin0, .Lfunc_end0

whose StartLabel is the start of the function and the EndLabel is the function end.

There is exactly one entry in the Location List DebugLoc in function buildLocationList. Before returning, there is a check to see if this one entry is valid throughout the scope of the function: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1744 This returns true in this case.

This leads to an optimization to use DW_AT_const_value instead of DW_AT_location and the dwarf standard here seems to suggest this : http://dwarfstd.org/ShowIssue.php?issue=161109.2

This optimization is triggered right here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841

At this point isValidSingleLocation is the return value of buildLocationList and when true does not create a .debug_loc section but instead creates a DW_AT_const_value.

Now, I tried to force a .debug_loc for this by simply making the value of isValidSingleLocation false always and the .debug_loc list shows up as expected. Diffing the asm without and with .debug_loc, the asm differs exactly like this :

Without debug_loc	With debug_loc
	.section .debug_loc
	...
...	...
.byte 28 # DW_AT_const_value	.byte 2 # DW_AT_location
.byte 15 # DW_FORM_udata	.byte 23 # DW_FORM_sec_offset

To summarize, it seems like DW_AT_const_value can be used instead of DW_AT_location under certain conditions and that's what seems to be happening here. Please correct me if this is a wrong assumption.

This is for the default case.

Now, let's look at what happens with bbsections, first without this patch. With bb sections, buildLocationList is called again. Here is the ASM with basic block sections:

	.text
	.file	"locdspnm.cpp"
	.type	_ZL4ncatPcjz,@function
_ZL4ncatPcjz:                           # @_ZL4ncatPcjz
.Lfunc_begin0:
	.loc	3 37 0                          # third_party/icu/source/common/locdspnm.cpp:37:0
	.cfi_startproc
# %bb.0:                                # %.critedge3
...
	je	_ZL4ncatPcjz.2
	jmp	_ZL4ncatPcjz.1
	.cfi_endproc
	.section	.text,"ax",@progbits,unique,1
_ZL4ncatPcjz.1:                         # %.critedge3
	.cfi_startproc
	.cfi_def_cfa %rsp, 64
...
	jmp	_ZL4ncatPcjz.2
.LBB_END0_1:
	.size	_ZL4ncatPcjz.1, .LBB_END0_1-_ZL4ncatPcjz.1
	.cfi_endproc
	.section	.text,"ax",@progbits,unique,2
_ZL4ncatPcjz.2:                         # %.critedge3
	.cfi_startproc
	.cfi_def_cfa %rsp, 64
...
	leaq	-128(%rsp), %rax
	#DEBUG_VALUE: ncat:buflen <- 157
.Ltmp0:
	.loc	3 47 3 prologue_end             # third_party/icu/source/common/locdspnm.cpp:47:3
...
	jmp	_ZL4ncatPcjz.3
.Ltmp1:
.LBB_END0_2:
	.size	_ZL4ncatPcjz.2, .LBB_END0_2-_ZL4ncatPcjz.2
	.cfi_endproc
	.p2align	4, 0x90
	.section	.text,"ax",@progbits,unique,3
_ZL4ncatPcjz.3:                         # =>This Inner Loop Header: Depth=1
	.cfi_startproc
	.cfi_def_cfa %rsp, 64
	#DEBUG_VALUE: ncat:buflen <- 157
	.loc	3 0 3 is_stmt 0                 # third_party/icu/source/common/locdspnm.cpp:0:3
	jmp	_ZL4ncatPcjz.3
.LBB_END0_3:
	.size	_ZL4ncatPcjz.3, .LBB_END0_3-_ZL4ncatPcjz.3
	.cfi_endproc
	.text
.Lfunc_end0:
	.size	_ZL4ncatPcjz, .Lfunc_end0-_ZL4ncatPcjz

Again, buildLocationList at https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1638 adds two entries but their start and end label pairs are now:

1. .Lfunc_begin0, .Ltmp1 // This is the same as the default case.
2. _ZL4ncatPcjz.3, .LBB_END0_3 // Note that the start label is the start of the next section and the end label is the end of the next section

Now, merge ranges will not merge these two into a single range as .Ltmp1 is not equal to _ZL4ncatPcjz3.

So, buildLocationList returns false at : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1741

That is, the optimization to not make a .debug_loc is not applicable in this case as that only holds if there is a valid single location here : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L1841

So, a loc list is constructed but without this patch this triggers a problem at : https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/AsmPrinter/DebugLocStream.h#L99

This is because the startSym and the endSym have to be in the same section. .Ltmp1 and .Lfunc_begin0 are in different sections. The assembler will complain if we try to assemble this.

So, what triggered this problem? LabelsBeforeInsn[insn] cannot be loosely assigned to be the start of the function with basic block sections if the instruction and the start of the function are in different sections.

Now, this patch fixes it by making LabelsBeforeInsn to be the start of the section. So, the two entries in DebugLoc would become:

1. _ZL4ncatPcjz.2, .Ltmp1
2. _ZL4ncatPcjz.3, .Lfunc_end0

This is good, but we are missing the entries corresponding to (.Lfunc_begin0 ... _ZL4ncatPcjz.2) which denote the 2 basic blocks before. We could handle this if we extend LabelsBeforeInsn to have multiple values in the case of basic block sections.

The way this patch exists, it loses the debug_loc fidelity as it does not capture the other blocks.

Thoughts?

Apologies for the very long reply!

Diffing the asm by forcing a debug_loc entry,

Not sure what you mean by this ^ could you explain further?

what happens here is that a DW_AT_location is replaced with a DW_AT_const_value. I don't understand this too well but this seems like an optimization to me.

Does this make sense?

I'm still a bit confused. I think if non-bb-sections doesn't need a location list, then bb-sections shouldn't either. Perhaps that issue should be fixed first, if it's an issue?

In any case/any order - perhaps this patch is still reflective of a bug that exists even in this ^ issue is fixed, but maybe it's that the test case isn't sufficient to demonstrate the remaining bug - perhaps the test case could be complicated a bit further to ensure it's not testing a behavior that shouldn't be happening for other reasons anyway?

[tmsriram]

@amharc

When basic block sections are on and the debug_loc entries span multiple sections, emit entries for each section. This keeps the fidelity and there is no loss of debug information.

To summarize, by default there is only one entry which is collapsed into a DW_AT_const_value.

WIth basic block sections, this one entry spans multiple sections and must be split into an entry for each section.

[dblaikie]

In D85085#2225420, @tmsriram wrote:

@amharc

When basic block sections are on and the debug_loc entries span multiple sections, emit entries for each section. This keeps the fidelity and there is no loss of debug information.

To summarize, by default there is only one entry which is collapsed into a DW_AT_const_value.

WIth basic block sections, this one entry spans multiple sections and must be split into an entry for each section.

if the location is valid for the entire range of the enclosing scope, it could still be described with a single DW_AT_const_value, I think?

DWARFv5 spec: "[single (non-loclist) location descriptions] are sufficient for describing the location of any object as long as its lifetime is either static or the same as the lexical block that owns it, and it does not move during its lifetime"

[tmsriram]

In D85085#2225425, @dblaikie wrote:

In D85085#2225420, @tmsriram wrote:

@amharc

When basic block sections are on and the debug_loc entries span multiple sections, emit entries for each section. This keeps the fidelity and there is no loss of debug information.

To summarize, by default there is only one entry which is collapsed into a DW_AT_const_value.

WIth basic block sections, this one entry spans multiple sections and must be split into an entry for each section.

if the location is valid for the entire range of the enclosing scope, it could still be described with a single DW_AT_const_value, I think?

DWARFv5 spec: "[single (non-loclist) location descriptions] are sufficient for describing the location of any object as long as its lifetime is either static or the same as the lexical block that owns it, and it does not move during its lifetime"

I am not sure I fully understood what you meant here, could you please elaborate? With sections, since the individual sections could be separated and can become disjoint you need one per section right? Maybe you meant something else.

[dblaikie]

In D85085#2225426, @tmsriram wrote:

In D85085#2225425, @dblaikie wrote:

In D85085#2225420, @tmsriram wrote:

@amharc

When basic block sections are on and the debug_loc entries span multiple sections, emit entries for each section. This keeps the fidelity and there is no loss of debug information.

To summarize, by default there is only one entry which is collapsed into a DW_AT_const_value.

WIth basic block sections, this one entry spans multiple sections and must be split into an entry for each section.

if the location is valid for the entire range of the enclosing scope, it could still be described with a single DW_AT_const_value, I think?

DWARFv5 spec: "[single (non-loclist) location descriptions] are sufficient for describing the location of any object as long as its lifetime is either static or the same as the lexical block that owns it, and it does not move during its lifetime"

I am not sure I fully understood what you meant here, could you please elaborate? With sections, since the individual sections could be separated and can become disjoint you need one per section right? Maybe you meant something else.

Ah, sure, sorry - if a variable only has one location, and that location is valid for the entire range of its enclosing scope (even if that enclosing scope is fragmented/uses ranges (rather than high/low) to describe its valid set of addresses) then the variable can use a simple/single location description.

Take for example this code:

void f1();
void f2() {
  {
    int i = 7;
    f1();
    f1();
  }
  f1();
}

compiled to LLVM IR with optimizations, then manually modified to reorder the 2nd and 3rd calls to f1 (to cause the valid set of addresses for the explicit lexical scope that contains i to become fragmented, and need a DW_AT_ranges to describe its valid addresses (the first f1 call, then the interleaved (originally 3rd, now second) f1 call which isn't part of tnhe scope, then more scope with the last call to f1)), this produces the following DWARF:

DW_TAG_subprogram
  DW_AT_low_pc    (0x0000000000000000)
  DW_AT_high_pc   (0x0000000000000011)
  DW_AT_name      ("f2")
  ...

  DW_TAG_lexical_block
    DW_AT_ranges  (0x00000000
      [0x0000000000000001, 0x0000000000000006)
      [0x000000000000000b, 0x0000000000000011))

    DW_TAG_variable
      DW_AT_const_value   (7)
      DW_AT_name  ("i")
      ...
    NULL

Despite 'i' having a disjoint valid range (it's only valid in the [1, 6)+[b, 11) instructions) - it doesn't need to use a loclist to describe its location, because its location is the same for the entire life of its enclosing scope, which is the implicit range that the location is valid over if no location list is used.

[tmsriram]

Redo the patch according to @dblaikie 's observation that loc list need not be used even with sections.

This change checks if debug loc entries can be merged to form a larger range even if they are split across sections. The test checks that a single range is collapsed into DW_AT_const_value both with and without sections.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1732–1735 ↗

(On Diff #287715)

This description sounds like it might merge across BB sections even if the location doesn't span the whole outer range.

eg: given a scope like [x, x+5) [y, y+3) and a location range of [x+3, x+5) [y, y+2) it sounds like this code might produce a single [x+3, y+2) range? That would be incorrect - since there's no ordering between the x and y ranges - so there's no way to cross the gap like that.

Could you include some test cases with interesting ranges like this to check that the DWARF is correct there?

[tmsriram]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1732–1735 ↗

(On Diff #287715)

Should the location range span the entire function? Could it be fully covering a subset of sections? Test cases for this seem very hard, even the one attached is reduced from a larger failure. Any pointers here? Thanks.

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1732–1735 ↗

(On Diff #287715)

llvm-dwarfdump some examples with non-trivial locations (I'll provide some suggestions on how to create non-trivial locations shortly) - if the location doesn't use a location list (ie: it uses a single location that spans the entire enclosing scope) then it should continue to use a single location with BB sections - because the implied range is "whatever addresses the enclosing scope says" - which will use ranges or high/low depending on whether it's disjoint or crosses BB-sections boundaries, etc)

If the location isn't a single location without BB-sections, then it can't be a single location with BB-sections. And any ranges that that location uses should be fragmented in the same way that DW_AT_ranges/high/low would be fragmented.

Would it be possible to use the same logic that handles ranges to handle these location ranges too? Presumably there's some code that transforms a begin/end pair into fragments when that pair crosses BB-section boundaries? So perhaps all the merging can be done here, then it can be checked to see if it's a valid single location (there's got to be some code somewhere that checks whether the final ranges cover all the enclosing scope ranges and uses the same location -> go to single location) and then fragmented if it crosses BB-section boundaries?

Some examples to consider:

void f1();
int f2();
extern bool b;
extern int x;
void test() {
  {
    // constant value with a single location description
    // Shouldn't change with BB-sections
    int i = 7;
    f1();
  } 
  int tmp = f2();
  { 
    // non-constant value with a single location description
    // Shouldn't change with BB-sections
    int i = tmp;
    f1();
    x = i;
  }
  { 
    // constant value through partial scope, but crossing a BB boundary
    // Should change with BB sections, I think - the address range for the
    // constant value will be fragmented.
    int i = 7;
    f1();
    if (b)
      f1();
    i = f2();
    f1();
  } 
  { 
    // constant value through partial scope, no BB boundary
    // Shouldn't change with BB-sections
    int i = 7;
    f1();
    i = f2();
    f1();
  } 
  { 
    // constant value through whole scope, including BB crossing - but LLVM puts
    // this in a location list unnecessarily.
    // You could fix the underlying LLVM bug (would be much appreciated) & make
    // this use a single location description - then with BB sections it should
    // still use a single location description.
    // If this bug remains, then the BB-sections code would need to fragment the
    // location list. (this'll make BB-sections DWARF size overhead higher than
    // it would otherwise be if the bug was fixed).
    int i = 7;
    f1();
    if (b)
      f1();
  } 
}

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1732–1735 ↗

(On Diff #287715)

Handily enough, this last case got fixed to use a constant location rather than a loclist recently, so this:

{
  int i = 7;
  f1();
  if (b)
    f1();
}

produces this DWARF (without bb sections - and should continue to produce the same DWARF with bb-sections)

DW_TAG_lexical_block
  DW_AT_low_pc  (0x0000000000000044)
  DW_AT_high_pc (0x0000000000000058)

  DW_TAG_variable
    DW_AT_const_value   (7)
    DW_AT_name  ("i")
    DW_AT_decl_file     ("/usr/local/google/home/blaikie/dev/scratch/loc.cpp")
    DW_AT_decl_line     (48)
    DW_AT_type  (0x00000192 "int")

[MaskRay]

llvm/test/DebugInfo/X86/basic-block-sections-debug-loc.ll

33

The attributes can be cleaned up a bit. For example "use-soft-float" defaults to false so it is not needed.

[tmsriram]

Address reviewer comments on adding various test cases to make sure loc lists are generated the same way with and without basic block sections except when the ranges cross section boundaries.

Disabled the temporary fix applied in D87787.

I have mainly added two things in the patch:

• Explicitly check if the ranges can be merged into one with sections and this is similar to how MergeRanges works without sections.
• Split the ranges across sections when a range spans sections. This happens specifically when an argument is used in a non-entry basic block, added a test for it.

Added individual tests for all the different cases pointed out by dblaikie@

[dblaikie]

Thanks for this - tests generally look great. Code mostly makes sense to me (that's on me, not you) - couple of minor quibbles with comment wording, but nothing major I think.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1742–1744 ↗

(On Diff #336595)

Presumably this happens also when StartLabel isn't the function begin symbol - but could be inside but not at the start of the first BB, or in some other BB - but where the end is in some other BB?
(the test cases are all simple enough/happen to have the interesting location range start in the first BB, but it's not a requirement)

1787–1788 ↗

(On Diff #336595)

Drop the else after return, ie:

if (x)
  return
if (hasBBSections()) {
  ...

Might also be worth inverting a bunch of these tests to reduce indentation/early return, eg:

if (!isSafeForSingleLocation)
  return false;
if (!validThroughout)
  return false;
if (size == 1)
  return true;
if (hasBBSections())
  return false;
// ...
MachineBasicBlock *RangeMBB = nullptr;
...

1789 ↗

(On Diff #336595)

presumably this isn't only about const_values - but any location (including non-const values) that's valid across the whole scope?

llvm/test/DebugInfo/X86/basic-block-sections-debug-loc-split-range.ll

17–18 ↗

(On Diff #336595)

might be worth a few more words, maybe the original source/description of the original test case - so this one can be read more independently (still useful to reference where it came from, though) - otherwise it's hard to know why that test + the transformation should have the location this test is testing for

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-2.ll

13 ↗

(On Diff #336595)

probably SECTIONS-NEXT

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll

12–16 ↗

(On Diff #336595)

I /think/ this test might be able to be made a bit more resilient to future unrelated DWARF changes (such as adding/removing/renaming unrelated attributes) with something like:

CHECK-NEXT: ... reg3
CHECK-NEXT: DW_AT
CHECK-NOT: DW_TAG
CHECK: _name ("i")

I'm not 100% sure that CHECK will match both in the case when there's a DW_AT_name immediately after the DW_TAG_location, and when there's some other attributes between location and name.

We don't always go to this much bother - but at least you can probably remove the decl file/line/type - the name seems enough to verify that the test hasn't accidentally started matching the location of some other variable like 'tmp'

(consider this sort of change/design aspect for the other test cases too)

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-5.ll

13 ↗

(On Diff #336595)

Guess this should probably be SECTIONS-NEXT to continue matching this location list/not accidentally skip over some other location

I take it there are 3 chunks because there are 3 BBs this constant value is live over (the initial BB, the if, and then a fragment of the last/returning BB (during the execution of the f2 call, but before the result has been assigned that will overwrite the constant value)

[tmsriram]

Address reviewer comments:

• Code refactoring.
• Test case cleanup.

[tmsriram]

In D85085#2708898, @dblaikie wrote:

Thanks for this - tests generally look great. Code mostly makes sense to me (that's on me, not you) - couple of minor quibbles with comment wording, but nothing major I think.

Thanks for reviewing this and providing the test cases. Took me quite a while to understand how to fix this!

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1742–1744 ↗

(On Diff #336595)

I may have misunderstood your question but I am assuming you are asking if StartLabel and Instr can be in different sections when StartLabel is not function begin?

This is a special case, mentioned in DebugHandlerBase.cpp in function "beginFunction". Quoting : "The first mention of a function argument gets the CurrentFnBegin label, so arguments are visible when breaking at function entry."

This means the Debug Instr and function begin could be in different sections which was the original problem to begin with. We make sure that we update the label each time we cross a section boundary otherwise, so an instr and a label being in different sections apart from this case is not possible. In function "endBasicBlock" PrevLabel is explicitly set to nullptr. This will force an emission of a temp label (or reuse the section's begin label) in the same section as the instruction for which a LabelsBeforeInsn is desired.

1787–1788 ↗

(On Diff #336595)

Apologies for not refactoring.

1789 ↗

(On Diff #336595)

Right, reworded to say it is about merging ranges.

llvm/test/DebugInfo/X86/basic-block-sections-debug-loc-split-range.ll

17–18 ↗

(On Diff #336595)

Understood, I added some more text describing what this test does. The source is not available because the IR was obtained using a reducer from a larger failing compile.

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll

12–16 ↗

(On Diff #336595)

I removed file, line and type. Seems like the other stuff is useful but if you think I should prune more, please lmk.

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-5.ll

13 ↗

(On Diff #336595)

Fixed and you are right about the chunks. The extra chunks for sections because there are the 2 additional basic blocks in different sections which cannot be collapsed.

[dblaikie]

[commented on the wrong review - sorry]

[dblaikie]

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1750 ↗

(On Diff #341002)

Hmm - using "I" for this sort of implies it's an iterator, but it isn't (if it was, we could increment it - but if we try to increment this pointer presumably we end up with garbage/random things that are in memory, because it's a linked list) - maybe if this was Asm->MF->begin() instead of front?

(also the name's still being flagged by the style checker - so would be good to rename it to something that matches the LLVM naming conventions)

It might be possible this could/should be a range-based-for loop?

for (MachineBasicBlock &MBB : *Asm->MF) {
}

And the checks like MBB_I != &Asm->MF->front() could be tested with &MBB != &Asm->MF->front()?

1742–1744 ↗

(On Diff #336595)

Stared at this a few times & still a bit confused, sorry.

Hmm, so there's some code somewhere else that handles the normal case of fragmenting basic blocks? Where is that code? (maybe there's a chance some of the logic here could be shared with that code) - what if that code elsewhere was removed, and this code were simplified (removing the "StartLabel == function begin" check) and became the only place location ranges were fragmented for bb sections? Would that be simpler?

This code here is specifically to patch up some strangeness in DebugHandlerBase.cpp - that strangeness is that certain parameter locations have their starting location drawn back up to the start of the function?

OK, so a few things - I'm guessing most of the tests I wrote don't cover this particular case? They do cover the general case of multi-basic block scope locations being fragmented by BB sections (& also the case of multi-basic block but not full-scope locations being broken up by bb sections). Do any of the tests pass/fail with this particular change applied or not?

Then, maybe it's easier for me to understand by looking at a particular test case that this code addresses an issue in - which test case would that be?

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll

12–16 ↗

(On Diff #336595)

Looks good - if you like, the test can be made more robust (in case new unrelated attributes are added in the future) by using CHECK-NOT: DW_TAG between attributes rather than using CHECK-NEXT (that way you'd still be checking that the attributes are all associated with the same tag - but allowing the possibility of other attributes).

It doesn't make the test resilient to changes in the order of attributes (you might be able to do that with something like:

CHECK: DW_TAG_variable
CHECK-DAG: DW_AT_location
CHECK-DAG: DW_AT_name
CHECK: DW_TAG

But if you do that, I'm not sure if the CHECK-NEXT needed for the DW_AT_location would work (not sure how it interacts with CHECK-DAG))

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-5.ll

13 ↗

(On Diff #336595)

Might be worth a comment in the test explaining this?

Maybe updating/changing/or moving the comment from the C source below and putting it up somewhere to say "When a location list must be used (because the location isn't valid through the entire enclosing scope - the assignment to 'i' towards the end truncates the 7 constant value location) - without basic block sections a single entry location list would be used. With basic block sections that one location list needs to be broken up to describe the separate address ranges in each section" or the like.

[tmsriram]

Address reviewer comments:

• Convert while(true) to for (...)
• Fix tests
• Provide more info on how loc list ranges are formed.

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

1750 ↗

(On Diff #341002)

Nice catch!

1742–1744 ↗

(On Diff #336595)

Stared at this a few times & still a bit confused, sorry.

Hmm, so there's some code somewhere else that handles the normal case of fragmenting basic blocks? Where is that code? (maybe there's a chance some of the logic here could be shared with that code) - what if that code elsewhere was removed, and this code were simplified (removing the "StartLabel == function begin" check) and became the only place location ranges were fragmented for bb sections? Would that be simpler?

https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp#L382

Please look at beginInstruction and endInstruction in DebugHandlerBase.cpp. LabelsBeforeInsn and LabelsAfterInsn are start and end label for each location list entry which will be either merged or kept unsplit in buildLocationList. LabelsBeforeInsn and LabelsAfterInsn are set here and these get a new label everytime we cross a basic block boundary via a jmp insn etc. This is true with or without sections.

Every jmp instruction and call insn for example will split the range. This would later be coalesced if possible in buildLocationList. endInstruction() in DebugHandlerBase.cpp is where the fragmenting happens in general, with or without basic block sections.

There is no code elsewhere that does anything specific for basic block sections with regards to location list. The real issue is with the coalescing of ranges which happens here for both sections and no-sections. With sections, this one assumption about the use starting from an intermediate basic block needs special handling.

This code here is specifically to patch up some strangeness in DebugHandlerBase.cpp - that strangeness is that certain parameter locations have their starting location drawn back up to the start of the function?

Correct, that one use case where the parameter is used at a basic block other than the entry but is defined from the start. An alternate way is to repeat the entry for this parameter multiple times pretending it was used in the entry too but that is a lot of code to do it. It also unnecessarily complicates the case for compiling without basic block sections. Please note that if we do this, we would split up the range first and then merge it when building without sections.

OK, so a few things - I'm guessing most of the tests I wrote don't cover this particular case? They do cover the general case of multi-basic block scope locations being fragmented by BB sections (& also the case of multi-basic block but not full-scope locations being broken up by bb sections). Do any of the tests pass/fail with this particular change applied or not?

I added a specific test for this, basic-block-setions-debug-loc-split-range.ll. That test will exercise this code to split the range as the first use is not in the entry basic block. A location list will be generated for this as the two uses of the value "buflen" in this test are different, 157 amd 159.

The original test which prompted the whole cleanup, basic-block-sections-debug-loc.ll will also exercise this code. Here, it will be merged into a const_value 157 as both uses of the value of buflen are the same. This means the range will first be split with basic block sections and then merged again later.

[tmsriram]

llvm/test/DebugInfo/X86/basic-block-sections-debug-loclist-3.ll

12–16 ↗

(On Diff #336595)

Ack, My different incantations seem to be less stable than this one. If you feel strongly about this, I can give this another pass.

[dblaikie]

Thanks for all your explanations/help understanding this. I think it's good to go.

[thakis]

This seems to break tests on mac and win, eg http://45.33.8.238/macm1/10314/step_11.txt

Please take a look and revert for now if it takes a while to fix.

[ahatanak]

Tests are failing here too:
http://lab.llvm.org:8080/green/job/clang-stage1-RA/21182/consoleFull#-706149783d489585b-5106-414a-ac11-3ff90657619c

[thakis]

reverted in 192b4141f0d74dd08a4eacf2184a6881906993ed for now

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

for (const auto &I : DbgValues) {
     // ideally not change the labels for constant debug values either, since
     // doing that violates the ranges that are calculated in the history map.
     // However, we currently do not emit debug values for constant arguments
     // directly at the start of the function, so this code is still useful.
     const DILocalVariable *DIVar =
         Entries.front().getInstr()->getDebugVariable();
     if (DIVar->isParameter() &&
         getDISubprogram(DIVar->getScope())->describes(&MF->getFunction())) {
+      // FIXME: If the basic block containing the first mention is not in the
+      // same section as the entry basic block, we cannot use the
+      // CurrentFnBegin label and we use the Section begin label instead.
+      // Using the Section Label will result in loss of debug loc fidelity as
+      // we do not capture the basic blocks before the start of this section.
+      auto FindLabelBeforeInsn = [this](const MachineInstr *MI) {
+        const auto *MBB = MI->getParent();
+        if (MBB->sameSection(&Asm->MF->front()))
+          return Asm->getFunctionBegin();
+        else
+          return Asm->MBBSectionRanges[MBB->getSectionIDNum()].BeginLabel;
+      };
       if (!IsDescribedByReg(Entries.front().getInstr()))
-        LabelsBeforeInsn[Entries.front().getInstr()] = Asm->getFunctionBegin();
+        LabelsBeforeInsn[Entries.front().getInstr()] =
+            FindLabelBeforeInsn(Entries.front().getInstr());
+
       if (Entries.front().getInstr()->getDebugExpression()->isFragment()) {
         // Mark all non-overlapping initial fragments.
         for (auto I = Entries.begin(); I != Entries.end(); ++I) {
           if (!I->isDbgValue())
             continue;
           const DIExpression *Fragment = I->getInstr()->getDebugExpression();
           if (std::any_of(Entries.begin(), I,
                           [&](DbgValueHistoryMap::Entry Pred) {
                             return Pred.isDbgValue() &&
                                    Fragment->fragmentsOverlap(
                                        Pred.getInstr()->getDebugExpression());
                           }))
             break;
           // The code that generates location lists for DWARF assumes that the
           // entries' start labels are monotonically increasing, and since we
           // don't change the label for fragments that are described by
           // registers, we must bail out when encountering such a fragment.
           if (IsDescribedByReg(I->getInstr()))
             break;
-          LabelsBeforeInsn[I->getInstr()] = Asm->getFunctionBegin();
+          LabelsBeforeInsn[I->getInstr()] = FindLabelBeforeInsn(I->getInstr());
         }
       }
     }
 
     for (const auto &Entry : Entries) {
       if (Entry.isDbgValue())
         requestLabelBeforeInsn(Entry.getInstr());
       else

llvm/test/DebugInfo/X86/basic-block-sections-debug-loc.ll

+;; When params loaded on the stack like va_args are used first in a non-entry
+;; basic block, the debug_loc entry is computed as the difference between the
+;; non-entry basic block and function begin address. When these two basic
+;; blocks are in different sections, it is an error.  With basic block
+;; sections, compute the difference from the start of the section.
+; RUN: llc %s --dwarf-version=4 --basic-block-sections=all -filetype=asm -o - | FileCheck %s
+
+; CHECK:      .section .debug_loc
+; CHECK-NEXT: .Ldebug_loc{{[0-9]+}}:
+; CHECK-NEXT: .quad
+; CHECK-NEXT: .quad  _ZL4ncatPcjz.2 # base address
+; CHECK-NEXT: .quad  .Ltmp{{[0-9]+}}-_ZL4ncatPcjz.2
+; CHECK-NEXT: .quad  .Ltmp{{[0-9]+}}-_ZL4ncatPcjz.2
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+define dso_local void @_ZL4ncatPcjz(i8* %0, i32 %1, ...) unnamed_addr #0 align 32 !dbg !22 {
+.critedge3:
+  call void @llvm.dbg.value(metadata i32 157, metadata !27, metadata !DIExpression()), !dbg !46
+  call void @llvm.va_start(i8* nonnull undef), !dbg !47
+  br label %2
+
+2:                                                ; preds = %2, %.critedge3
+  br label %2
+}
+
+; Function Attrs: nounwind
+declare void @llvm.va_start(i8*) #1
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+attributes #0 = { "use-soft-float"="false" }
+attributes #1 = { nounwind }
+attributes #2 = { nounwind readnone speculatable willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!20, !21}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version google3-trunk (9d2da6759b4d05d834371bcaaa8fc3d9b3385b18)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, retainedTypes: !2)
+!1 = !DIFile(filename: "bug.cpp", directory: "/proc/self/cwd")
+!2 = !{!3, !6, !9, !10, !11, !13, !16, !17, !15}
+!3 = !DIDerivedType(tag: DW_TAG_typedef, name: "int32_t", file: !4, line: 38, baseType: !5)
+!4 = !DIFile(filename: "stdint.h", directory: "/proc/self/cwd")
+!5 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!6 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !7, size: 64)
+!7 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !8)
+!8 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_unsigned_char)
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !8, size: 64)
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!11 = !DIDerivedType(tag: DW_TAG_typedef, name: "uint32_t", file: !4, line: 51, baseType: !12)
+!12 = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+!13 = !DIDerivedType(tag: DW_TAG_typedef, name: "UChar", file: !14, line: 372, baseType: !15)
+!14 = !DIFile(filename: "umachine.h", directory: "/proc/self/cwd")
+!15 = !DIBasicType(name: "char16_t", size: 16, encoding: DW_ATE_UTF)
+!16 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !3, size: 64)
+!17 = !DIDerivedType(tag: DW_TAG_typedef, name: "UBool", file: !14, line: 261, baseType: !18)
+!18 = !DIDerivedType(tag: DW_TAG_typedef, name: "int8_t", file: !4, line: 36, baseType: !19)
+!19 = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+!20 = !{i32 2, !"Debug Info Version", i32 3}
+!21 = !{i32 7, !"PIC Level", i32 2}
+!22 = distinct !DISubprogram(name: "ncat", linkageName: "_ZL4ncatPcjz", scope: !1, file: !1, line: 37, type: !23, scopeLine: 37, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !25)
+!23 = !DISubroutineType(types: !24)
+!24 = !{!3, !9, !11, null}
+!25 = !{!26, !27, !28, !41, !42, !43, !44}
+!26 = !DILocalVariable(name: "buffer", arg: 1, scope: !22, file: !1, line: 37, type: !9)
+!27 = !DILocalVariable(name: "buflen", arg: 2, scope: !22, file: !1, line: 37, type: !11)
+!28 = !DILocalVariable(name: "args", scope: !22, file: !1, line: 38, type: !29)
+!29 = !DIDerivedType(tag: DW_TAG_typedef, name: "va_list", file: !30, line: 14, baseType: !31)
+!30 = !DIFile(filename: "stdarg.h", directory: "/proc/self/cwd")
+!31 = !DIDerivedType(tag: DW_TAG_typedef, name: "__builtin_va_list", file: !1, line: 38, baseType: !32)
+!32 = !DICompositeType(tag: DW_TAG_array_type, baseType: !33, size: 192, elements: !39)
+!33 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "__va_list_tag", file: !1, line: 38, size: 192, flags: DIFlagTypePassByValue, elements: !34, identifier: "_ZTS13__va_list_tag")
+!34 = !{!35, !36, !37, !38}
+!35 = !DIDerivedType(tag: DW_TAG_member, name: "gp_offset", scope: !33, file: !1, line: 38, baseType: !12, size: 32)
+!36 = !DIDerivedType(tag: DW_TAG_member, name: "fp_offset", scope: !33, file: !1, line: 38, baseType: !12, size: 32, offset: 32)
+!37 = !DIDerivedType(tag: DW_TAG_member, name: "overflow_arg_area", scope: !33, file: !1, line: 38, baseType: !10, size: 64, offset: 64)
+!38 = !DIDerivedType(tag: DW_TAG_member, name: "reg_save_area", scope: !33, file: !1, line: 38, baseType: !10, size: 64, offset: 128)
+!39 = !{!40}
+!40 = !DISubrange(count: 1)
+!41 = !DILocalVariable(name: "str", scope: !22, file: !1, line: 39, type: !9)
+!42 = !DILocalVariable(name: "p", scope: !22, file: !1, line: 40, type: !9)
+!43 = !DILocalVariable(name: "e", scope: !22, file: !1, line: 41, type: !6)
+!44 = !DILocalVariable(name: "c", scope: !45, file: !1, line: 49, type: !8)
+!45 = distinct !DILexicalBlock(scope: !22, file: !1, line: 48, column: 45)
+!46 = !DILocation(line: 0, scope: !22)
+!47 = !DILocation(line: 47, column: 3, scope: !22)