Page MenuHomePhabricator

DWARFDebugLoc(v4): Add an incremental parsing function
ClosedPublic

Authored by labath on Tue, Nov 5, 7:37 AM.

Details

Summary

This adds a visitLocationList function to the DWARF v4 location lists,
similar to what already exists for DWARF v5. It follows the approach
outlined in previous patches (D69672), where the parsed form is always
stored in the DWARF v5 format, which makes it easier for generic code to
be built on top of that. v4 location lists are "upgraded" during
parsing, and then this upgrade is undone while dumping.

Both "inline" and section-based dumping is rewritten to reuse the
existing "generic" location list dumper. This means that the output
format is consistent for all location lists (the only thing one needs to
implement is the function which prints the "raw" form of a location
list), and that debug_loc dumping correctly processes base address
selection entries, etc.

The previous existing debug_loc functionality (e.g.,
parseOneLocationList) is rewritten on top of the new API, but it is not
removed as there is still code which uses them. This will be done in
follow-up patches, after I build the API to access the "interpreted"
location lists in a generic way (as that is what those users really
want).

Diff Detail

Event Timeline

labath created this revision.Tue, Nov 5, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 5, 7:37 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
JDevlieghere accepted this revision.Tue, Nov 5, 9:42 AM
This revision is now accepted and ready to land.Tue, Nov 5, 9:42 AM
labath planned changes to this revision.Mon, Nov 11, 6:14 AM

I ran into a bit of a snag a couple of patches down this direction. I'm going to do a small course correction and then get back to this. I'll explain more in the other patches.

labath updated this revision to Diff 229107.Wed, Nov 13, 8:14 AM
  • rebase on master
  • reuse the new generic dumping mechanism
This revision is now accepted and ready to land.Wed, Nov 13, 8:14 AM
labath requested review of this revision.Wed, Nov 13, 8:22 AM
labath marked an inline comment as done.

requesting re-review, as the this patch has changed/expanded substantially

llvm/test/tools/dsymutil/X86/basic-lto-linking-x86.test
140–142

One thing I'm not sure of it's the syntax of these "raw" entries. The () syntax is sort of similar to the DW_LLE(x,y) syntax, but at same time, it's easy to miss that one is looking at the "raw" dump as the difference is only one character. OTOH, the v4 loclists don't have any fancy encodings, so the "raw" format is nearly the same as the "interpreted" format anyway...

Maybe we could use curly brackets here or something?

JDevlieghere accepted this revision.Thu, Nov 14, 12:55 PM

A few nits inline, but otherwise this LGTM.

llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h
107

Can we have a more meaningful name than F?

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
161

Nit: given that we have the comments in both branches, maybe it's worth having a named temporary variable?

184

Any reason you prefer this over while(true) with a break? Just asking out of curiosity.

This revision is now accepted and ready to land.Thu, Nov 14, 12:55 PM
labath marked 3 inline comments as done.Fri, Nov 15, 4:36 AM

Thanks for the review. I'll incorporate the comments before committing.

llvm/lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
184

I think I was influenced by the other loops in this patch which do some extra work after setting the "continue" flag. while(true) seems fine here.

This revision was automatically updated to reflect the committed changes.

Hi Pavel, a bug from debug_loc.dwo emission might has gone up here.
Now please correcting me if I'm wrong.
It's originating from the emission part of debug_loc.dwo. -- I tried pointing / correcting it in D69462.

Consider the below snippet at the end of emitDebugLocDWO() function
Asm->emitInt8(dwarf::DW_LLE_end_of_list); -- now this piece of code is causing to emit DW_LLE_end_of_list in a debug_loc.dwo which has no notion of this form. amidst it has it's own way of representing end of list.

I corrected this as --

Asm->OutStreamer->EmitIntValue(0, Size);  -- where Size = Asm->MAI->getCodePointerSize()
Asm->OutStreamer->EmitIntValue(0, Size);

@dblaikie also did something same for marking termination of list in function emitRangeList at the very end while emitting in non-split debug_loc section.

Now the undesirable behavior, after correcting this -- Since this patch already add visitLocationList and some sort of verbose printing of DW_LLE* for debug_loc.dwo, which I suppose llvm-dwardump is using.
For typical case of location list {loc.dwo} -- with latest master

llvm-dwarfdump --debug-loc test.dwo
.debug_loc.dwo contents:
0x00000000:
            DW_LLE_startx_length   (0x00000000, 0x00000004): DW_OP_reg5 RDI
            DW_LLE_startx_length   (0x00000003, 0x00000014): DW_OP_reg3 RBX
            DW_LLE_end_of_list     ()

0x00000013:
            DW_LLE_startx_length   (0x00000001, 0x00000006): DW_OP_reg5 RDI
            DW_LLE_end_of_list     ()

0x0000001d:
            DW_LLE_startx_length   (0x00000001, 0x0000000b): DW_OP_reg4 RSI
            DW_LLE_end_of_list     ()

Now if, I correct it --

llvm-dwarfdump --debug-loc test.dwo
debug_loc.dwo contents:
0x00000000:
            DW_LLE_startx_length   (0x00000000, 0x00000004): DW_OP_reg5 RDI
            DW_LLE_startx_length   (0x00000003, 0x00000014): DW_OP_reg3 RBX
            DW_LLE_end_of_list     ()

0x00000013:
            DW_LLE_end_of_list     ()
..... + a lot of DW_LLE_end_of_list     ()

0x00000022:
            DW_LLE_startx_length   (0x00000001, 0x00000006): DW_OP_reg5 RDI
            DW_LLE_end_of_list     ()
..... + a lot of DW_LLE_end_of_list     ()

0x0000003b:
            DW_LLE_startx_length   (0x00000001, 0x0000000b): DW_OP_reg4 RSI
            DW_LLE_end_of_list     ()
..... + a lot of DW_LLE_end_of_list     ()

I'm suspecting, the changed representation of termination list might not be appropriately handled{Anyway, you were not aware of this, at that point} here in DWARFDebugLoc::visitLocationList.

Why this bothering me ?? I think, I also started out a discussion in D70081 that why we are adding this verbosity and stuff to debug_loc.dwo
you answered.

You're absolutely right, and I am planning to do that. I already have a patch which does just that -- I just need to clean it up before submission (I'll do that today or tomorrow).

Untill now loc printing is mostly ranges, so we don't worry about form DW_LLE*, but I'm suspecting this handling of forms, specially termination of list, you might run into un-necessary trouble.

I think, it's fine if we just dump ranges for pre-dwarf5 stuff.
Please share your thoughts on this, As @dblaikie in D69462 already asked to put a separate patch for this, which I suspect invites rework on DWARFDebugLoc::visitLocationList.
My Apologies, if this is too big / self explanatory.

I'm sorry, but I'm confused. Are you saying that the .debug_loc.dwo (i.e. non-standardized dwarf4 fission) location lists should be terminated with a classical .debug_loc double-zero entry ? That doesn't seem right -- the .debug_loc.dwo lists are in reality very different from .debug_loc (they use DW_LLE_*** constants and everything), and are really more similar to .debug_loclists(.dwo) than .debug_loc (though there are some subtle differences). It also doesn't seem to be what gcc is doing now -- this is a typical location list I get from gcc-9 -gsplit-dwarf (comments added by me):

        .section        .debug_loc.dwo,"e",@progbits
.Ldebug_loc0:
.LVUS0: # gcc "view" stuff -- ignore that
        .uleb128 .LVU2
        .uleb128 0
.LLST0: # DW_AT_location points here
        .byte   0x3                  # DW_LLE_startx_length
        .uleb128 0x1                 # index
        .long   .LFE0-.LVL0          # length
        .value  0x5                  # expression length
        .byte   0x75
        .sleb128 0
        .byte   0x31
        .byte   0x24
        .byte   0x9f
        .byte   0                # DW_LLE_end_of_list
.LVUS1: # next "view" starts here

In practice, changing the .byte 0 to two word-sized zeroes probably won't change anything, as the first byte of that will terminate the location list anyway. However, I don't think that's the right thing to do, and I think llvm-dwarfdump is right to display a bunch of end_of_list entries for something like that.

I'm sorry, but I'm confused. Are you saying that the .debug_loc.dwo (i.e. non-standardized dwarf4 fission) location lists should be terminated with a classical .debug_loc double-zero entry ? That doesn't seem right -- the .debug_loc.dwo lists are in reality very different from .debug_loc (they use DW_LLE_*** constants and everything), and are really more similar to .debug_loclists(.dwo) than .debug_loc (though there are some subtle differences). It also doesn't seem to be what gcc is doing now -- this is a typical location list I get from gcc-9 -gsplit-dwarf (comments added by me):

Thanks for sharing, this I'm also sceptical here, as to what approach we should follow. You're comments seems reasonable here, But If you just try dumping this{GCC, compiled binary} -- again using llvm-dwarfdump you'll notice lots of DW_LLE_end_of_list. I'm not pointing you're wrong -- it's just the dump that the end user will see -- will not make sense{atleast confuses me} to him.

        .section        .debug_loc.dwo,"e",@progbits
.Ldebug_loc0:
.LVUS0: # gcc "view" stuff -- ignore that
        .uleb128 .LVU2
        .uleb128 0
.LLST0: # DW_AT_location points here
        .byte   0x3                  # DW_LLE_startx_length
        .uleb128 0x1                 # index
        .long   .LFE0-.LVL0          # length
        .value  0x5                  # expression length
        .byte   0x75
        .sleb128 0
        .byte   0x31
        .byte   0x24
        .byte   0x9f
        .byte   0                # DW_LLE_end_of_list
.LVUS1: # next "view" starts here

In practice, changing the .byte 0 to two word-sized zeroes probably won't change anything, as the first byte of that will terminate the location list anyway. However, I don't think that's the right thing to do, and I think llvm-dwarfdump is right to display a bunch of end_of_list entries for something like that.

I'm sorry, but I'm confused. Are you saying that the .debug_loc.dwo (i.e. non-standardized dwarf4 fission) location lists should be terminated with a classical .debug_loc double-zero entry ? That doesn't seem right -- the .debug_loc.dwo lists are in reality very different from .debug_loc (they use DW_LLE_*** constants and everything), and are really more similar to .debug_loclists(.dwo) than .debug_loc (though there are some subtle differences). It also doesn't seem to be what gcc is doing now -- this is a typical location list I get from gcc-9 -gsplit-dwarf (comments added by me):

Thanks for sharing, this I'm also sceptical here, as to what approach we should follow. You're comments seems reasonable here, But If you just try dumping this{GCC, compiled binary} -- again using llvm-dwarfdump you'll notice lots of DW_LLE_end_of_list. I'm not pointing you're wrong -- it's just the dump that the end user will see -- will not make sense{atleast confuses me} to him.

Could you share this binary or some instructions how to produce it. I know there are some issues with dumping gcc-produced range/location lists, but I don't think this has to do with how they are terminated. The for instance, if I tried to do a section-based dump ("inline dumps" work fine, but they don't show end-of-list entries) of the location list from https://reviews.llvm.org/D69847#1753190, then I'd get an error like "error: LLE of kind 2 not supported". However this is due to the "view" blurbs that gcc inserts and not about location lists per se. (Which kind of ties in to the discussion of how should we actually perform section dumps -- there's an opinion that just scanning through the section and printing location lists (i.e. what llvm-dwarfdump -debug-loc is doing now) is not correct/legal, and that one should always start from the debug_info section and follow references from there. Gcc apparently exploits this fact to insert some custom data into the debug_loc/range sections)

Could you share this binary or some instructions how to produce it. I know there are some issues with dumping gcc-produced range/location lists, but I don't think this has to do with how they are terminated. The for instance, if I tried to do a section-based dump ("inline dumps" work fine, but they don't show end-of-list entries) of the location list from https://reviews.llvm.org/D69847#1753190, then I'd get an error like "error: LLE of kind 2 not supported". However this is due to the "view" blurbs that gcc inserts and not about location lists per se. (Which kind of ties in to the discussion of how should we actually perform section dumps -- there's an opinion that just scanning through the section and printing location lists (i.e. what llvm-dwarfdump -debug-loc is doing now) is not correct/legal, and that one should always start from the debug_info section and follow references from there. Gcc apparently exploits this fact to insert some custom data into the debug_loc/range sections)

Ah, Did I communicated intentions here, this problem of multiple spurious DW_LLE* will arise if, I placed or fix it that way,
Asm->OutStreamer->EmitIntValue(0, Size) -- not needed, I suppose after this discussion. thank you for clarifying this.

But remain with GCC compiled binaries--
test case--
compiled with
gcc -O2 -gdwarf-4 -gsplit-dwarf test.c GCC version 9.2.0

int func(int* ptr){
        printf("%d\n",*ptr);     return *ptr + 5;
}
int main(){
        int a = 4;     int* ptr_a = &a;
        int b = func(ptr_a);
        return 0;
}

llvm-dwarfdump --debug-loc test.dwo
.debug_loc.dwo contents:
0x00000000:
            DW_LLE_end_of_list     ()
+... DW_LLE_end_of_list

0x00000004:
            DW_LLE_startx_length   (0x00000000, 0x0000000e): DW_OP_reg5 RDI
            DW_LLE_startx_length   (0x00000002, 0x0000000e): DW_OP_GNU_entry_value(DW_OP_reg5 RDI), DW_OP_stack_value
            DW_LLE_end_of_list     ()
+... DW_LLE_end_of_list
... like wise

I'm sorry, but I'm confused. Are you saying that the .debug_loc.dwo (i.e. non-standardized dwarf4 fission) location lists should be terminated with a classical .debug_loc double-zero entry ? That doesn't seem right -- the .debug_loc.dwo lists are in reality very different from .debug_loc (they use DW_LLE_*** constants and everything), and are really more similar to .debug_loclists(.dwo) than .debug_loc (though there are some subtle differences). It also doesn't seem to be what gcc is doing now -- this is a typical location list I get from gcc-9 -gsplit-dwarf (comments added by me):

        .section        .debug_loc.dwo,"e",@progbits
.Ldebug_loc0:
.LVUS0: # gcc "view" stuff -- ignore that
        .uleb128 .LVU2
        .uleb128 0
.LLST0: # DW_AT_location points here
        .byte   0x3                  # DW_LLE_startx_length
        .uleb128 0x1                 # index
        .long   .LFE0-.LVL0          # length
        .value  0x5                  # expression length
        .byte   0x75
        .sleb128 0
        .byte   0x31
        .byte   0x24
        .byte   0x9f
        .byte   0                # DW_LLE_end_of_list
.LVUS1: # next "view" starts here

In practice, changing the .byte 0 to two word-sized zeroes probably won't change anything, as the first byte of that will terminate the location list anyway. However, I don't think that's the right thing to do, and I think llvm-dwarfdump is right to display a bunch of end_of_list entries for something like that.

Hmm, I'm not seeing these extra ulebs in GCC's output in this example: https://godbolt.org/z/x5RvMY - am I missing something in the reproduction or not reading it accurately?

llvm/test/tools/dsymutil/X86/basic-lto-linking-x86.test
140–142

Few thoughts:

  • I think I mentioned in a related review that we could use the same thing as the range dumping - that includes the section name/number (though that's only done in verbose mode? And wouldn't help with pure offset-pair/base-relative dumping in the absence of a base address anyway - because there's no way to know the section...)
  • {} sound OK to me (maybe we could change the DW_LLE to that too, since the (), espceially with the whitespace between the LL and the '(' are a bit ambiguous too, I think? But I'm not sure/don't have a strong leaning either way there.
  • Could write something like "base + " before the pair in cases where the base is unknown - but I guess that means most traditional location lists (ones that don't use any base addresses/constant zero base address) would print with "base + " which isn't super great

So... "{}" I guess? It'd be good to look at v4 range dumping too & come up with a decision we're OK with for both cases, I think.

Hmm, I am sort of coming back to maybe printing these as [x, y) isn't /so/ bad. In the unrelocated object file we would have the section info (because we'd have relocations) maybe we could print that like the verbose range dumping but even in non-verbose mode when section dumping & not having the base address? & we could ignore the base address part... that's how we print v4 range lists currently? Either in objects where the CU has a relocated base address, or in linked executables where the range list no longer contains relocations (but also doesn't contain apparently overlapping ranges all starting at zero - because now they're all relocated). I guess we could print "base + " as a separate line at the start of the list to make it clear that they're all relative to any potential CU base...

Eh, would love to hear other people's ideas/preferences here. How much shuold the dumping output be unambiguous/? How much is skipping things like "base+" useful to us folks who already work on DWARF and understand that the code we're looking at doesn't start at address zero, so of course ranges starting at zero must be either unresolved relocations (well, in that case it's easy to print something nice about the section) or relative to a base address?

Hmm, I'm not seeing these extra ulebs in GCC's output in this example: https://godbolt.org/z/x5RvMY - am I missing something in the reproduction or not reading it accurately?

It looks like you need to add -gvariable-location-views to the command line. I'm not sure what's the difference, but my build of gcc seems to have it on by default, and godbolt's doesn't.

I'm going to reply to the other comment tomorrow.

Hmm, I'm not seeing these extra ulebs in GCC's output in this example: https://godbolt.org/z/x5RvMY - am I missing something in the reproduction or not reading it accurately?

It looks like you need to add -gvariable-location-views to the command line. I'm not sure what's the difference, but my build of gcc seems to have it on by default, and godbolt's doesn't.

Ah, that is a curious switch indeed. And, yeah, the existence of that feature sounds like a very solid justification that we should consider moving in the direction of not trying to parse arbitrary parts of non-debug_info sections (only those referenced from debug_info itself) except /maybe/ in a very exploratory debugging mode, and/or phrased differently in terms of "attempted to parse hole in debug_loc [offset1:offset2), but failed: <reason>"

I'm going to reply to the other comment tomorrow.

Ah, sure - that was a comment I didn't realize I'd written a while back and had failed to hit "send" on, feel free to ignore it if it doesn't make any more sense today or the like.