[PATCH] D85085: Fix debug_loc offset difference with basic block sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1732-1735
+    // Entries in different basic block sections can still be merged.  Here, we
+    // check if the previous entry's end label and the next entry's begin label
+    // are in adjacent basic block sections and pointing to the end of the
+    // first section and the beginning of the following section respectively.
----------------
tmsriram wrote:
> dblaikie wrote:
> > 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?
> 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.
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();
  } 
} 
```


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

https://reviews.llvm.org/D85085