This is an archive of the discontinued LLVM Phabricator instance.

llvm-symbolizer: support DW_FORM_loclistx locations.
ClosedPublic

Authored by eugenis on Nov 26 2019, 5:38 PM.

Details

Summary

With -gdwarf-5 local variable locations are emitted as DW_FORM_loclistx
form instead of the regular DW_FORM_sec_offset. Teach
DWARFDie::getLocations to understand the new format and use if in
llvm-symbolizer "FRAME" command.

Diff Detail

Event Timeline

eugenis created this revision.Nov 26 2019, 5:38 PM
ostannard added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
1096

I'm seeing a crash in test tools/llvm-symbolizer/frame-noname.s, I think Expected requires it's error to be consumed when getLocations fails.

jdoerfert resigned from this revision.Nov 29 2019, 10:10 AM
eugenis updated this revision to Diff 232017.Dec 3 2019, 4:51 PM

handle error condition when missing AT_location

eugenis marked an inline comment as done.Dec 3 2019, 4:51 PM
ostannard added inline comments.Dec 10 2019, 2:51 AM
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
503

It looks like this patch conflicts with D71006, which adds the loc section base inside DWARFUnit::getLoclistOffset, so it doesn't need adding here.

jhenderson added inline comments.Dec 10 2019, 3:07 AM
llvm/test/tools/llvm-symbolizer/frame-loclistx.s
1 ↗(On Diff #232017)

I think this test would make more sense in the DebugInfo test directory, as this isn't really testing llvm-symbolizer's specific features, and is more testing the DebugInfo library code.

A couple of other points:

  • I've noticed that a number of text editors don't work well with C++ style comments for .s files. I think you should use '#' (and '##' for comments to improve clarity of comments versus lit/FileCheck directives).
  • The assembly is very complex. Can you reduce it down significantly to just what you need for your test?
eugenis updated this revision to Diff 233160.Dec 10 2019, 11:27 AM
eugenis marked 2 inline comments as done.

address review comments

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
503

fixed

llvm/test/tools/llvm-symbolizer/frame-loclistx.s
1 ↗(On Diff #232017)

Moved the test to DebugInfo.

C++ styles comments are what clang generates in the assembly output. Anyway, I've replaced them with "#".

The assembly is very complex. Can you reduce it down significantly to just what you need for your test?

I'm not really comfortable hacking on the dwarf assembly. I'm worried it's very easy to create a malformed test input this way, and I'm not familiar with the format enough to recognize it.

C++ styles comments are what clang generates in the assembly output. Anyway, I've replaced them with "#".

They aren't for me ('#' is what clang is generating for me), but I default to X86_64 targets. It looks like it depends on the target triple (aarch64 appears to use '//' as you've observed).

I'm not really comfortable hacking on the dwarf assembly. I'm worried it's very easy to create a malformed test input this way, and I'm not familiar with the format enough to recognize it.

I can sympathise, but it isn't actually that hard to get rid of a lot of junk. You also now have a working test, so you should be able to try deleting things and seeing if the test still works. Just remember to verify that the test failed beforehand. I've commented inline on some bits that are obviosuly not needed.

I'm not really appropriate to review the loclists stuff, but I've added a few perople more familiar with DWARF as reviewers who may be better placed to do so.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
1106

If there's an error, we shouldn't just throw away that error ideally. However, I see that this is a void function so propagating the Error up the stack may be non-trivial. If that is the case, please at least comment this with a TODO to say that we should handle the Error more cleanly.

llvm/test/DebugInfo/AArch64/frame-loclistx.s
72–73

You can delete this string and its reference in the .debug_str_offsets table, if you trim the .debug_info as I suggest.

107

You'll be able to delete big chunks of the .debug_abbrev data, that correspond to the stuff I recommend you delete in the .debug_info below.

242–245

I believe this is all irrelevant to the test in question.

261

I believe you can delete this attribute, as the type of the variabls is irrelevant for the test.

262–269

If I'm reading the DWARF right, these have no relevance to the test in question.

281–282

These two lines don't add anything useful for this test, I believe (they add a STT_FILE elf symbol and an irrelevant section).

eugenis updated this revision to Diff 234625.Dec 18 2019, 2:46 PM
eugenis marked an inline comment as done.

reverted comments back to //

I'm sorry but I have better use for my time than editing valid dwarf files by hand.

llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
1106

The error is handled by not setting Local.FrameOffset, which would result in llvm-symbolizer printing "??" instead of the frame offset value. AFAIK, it is valid for AT_location to not be present, and I've definitely seen such files in the wild.

As for the test case: Yeah, I'd include the source (as minimal as possible) in a comment (along with whatever compiler command is used to generate the assembly from the source) & leave the assembly relatively unmodified from whatever LLVM produces - rather than trying to hand-craft it too much.

This is some of the shortest code I have that produces a loclist:

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

But maybe that doesn't produce the kind of location list you need? (I guess you need the variable to reside in memory/in the stack frame? in which case escaping the int as a poniter parameter (which, glancing through the DWARF in the test case, looks like what's going on)). You might be able to turn off the call site stuff (I'm not sure if there's a flag for it - I think there is) to reduce the DWARF a bit more.

(& yeah, I'm confused by the choice of comment character - all the assembly test cases I've seen/dealt with use '#' as the comment character)

As for the error handling - yeah, the absence of a DW_AT_location isn't an error case, but mangled DWARF that makes it impossible to resolve the location list (eg: a missing debug_addr section if address pool entry references are used) might be worthy of an error message. I don't think you need to handle this here and now, but I think it's possible that the "getLocations" API could be changed to return an empty non-error when the attribute is not present - in the same way as it could return an empty non-error if the attribute was present but referred to an empty location list (no DWARF producer should ever produce that) - it's just the degenerate case. Then the error cases would be really erroneous, I think, and might be worthy of reporting to a user.

Unit tests: pass. 61013 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

eugenis added a comment.EditedDec 18 2019, 3:39 PM

I've added the source code and compiler flags to the test case.
As for the comment syntax, if you search for assignments to MCAsmInfo::CommentString, you'll discover that the options are:

//
#
##
;
!
@

Unit tests: pass. 61013 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

dblaikie accepted this revision.Dec 18 2019, 4:31 PM

Looks good

This revision is now accepted and ready to land.Dec 18 2019, 4:31 PM

I've got no more comments, but I'm not going to LGTM it because I disagree with the principle of not minimising test cases (I have better things to do with my time than try to understand what is actually important in a test case that has started failing/needs updating etc etc).

I've got no more comments, but I'm not going to LGTM it because I disagree with the principle of not minimising test cases (I have better things to do with my time than try to understand what is actually important in a test case that has started failing/needs updating etc etc).

I understand your concern. But by minimizing the test case, I (not an expert in dwarf by any measure) can make it invalid in a way that is not detected by the current parser code. How do you feel about fixing a bunch of tests, all broken in different ways, when LLVM becomes smarter about it? This test case may be a bit long, but I'm reasonably sure that it is correct.

I've got no more comments, but I'm not going to LGTM it because I disagree with the principle of not minimising test cases (I have better things to do with my time than try to understand what is actually important in a test case that has started failing/needs updating etc etc).

I understand your concern. But by minimizing the test case, I (not an expert in dwarf by any measure) can make it invalid in a way that is not detected by the current parser code. How do you feel about fixing a bunch of tests, all broken in different ways, when LLVM becomes smarter about it? This test case may be a bit long, but I'm reasonably sure that it is correct.

Yeah, while I wouldn't generally consider not being an expert as sufficient for this - I'd push back a bit if it were only that. But hand writing DWARF assembly isn't exactly trivial (DIE offsets being a simple example of something that would need to be manually adjusted currently - though we've talked about modifying LLVM's output to be more symbolic & thus easier to edit) but also changing abbreviations in lock-step with the DIEs themselves, etc.

At some point we might end up with yaml2obj or something like it growing higher level descriptions to make it easy to write varying degrees of valid DWARF - so it's easy to write to the level of correctness/incorrectness that's desired for a test without a lot of redundancy/verbosity in the written form. (eg: something that autoselects correct forms, requires certain attributes, etc, automatically generates abbreviations - but then lets you drop down below that if you want a quirky attribute added/removed, etc)

(I wouldn't mind if we had some way to turn off the call site entry DIEs, as they do add a fair bit of noise to simple test cases like this - but doesn't look like there's a flag for it & I don't think it's worth hand-editing the DWARF to remove them)

Sorry, I think my previous comment came out wrong. I'm happy with this going in without my approval. I just don't want to be marked as approving something that I don't really agree with (I do see your point of view to be clear - this isn't me stubbornly digging in my heels and saying you're all wrong).

I do agree that a clearer way of generating DWARF would be nice. Gtest unit tests may be the way forward in many situations, since you can easily write some simple framework to do the common stuff and build on that (see the existing DWARFDebugLine ones for an example of what I mean). I'm not suggesting it's always appropriate mind you.

I did stumble across a dwarf2yaml version of obj2yaml. I haven't looked into it beyond that it's currently used by the mach-o version of obj2yaml, and I have no idea if yaml2obj works with it or not at this point, but it might provide a starting point for the future.

This revision was automatically updated to reflect the committed changes.