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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.
Herald added a subscriber: danielkiss.


================
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.
----------------
dblaikie wrote:
> 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();
>   } 
> } 
> ```
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")
```


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

https://reviews.llvm.org/D85085