This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Introduce -brief command line option to llvm-dwarfdump
ClosedPublic

Authored by sgravani on Jun 3 2017, 10:14 AM.

Details

Summary

This patch introduces a new command line option, called brief, to llvm-dwarfdump.
When -brief is used, the attribute forms for the .debug_info section will not be emitted to output.

Diff Detail

Repository
rL LLVM

Event Timeline

sgravani created this revision.Jun 3 2017, 10:14 AM
aprantl requested changes to this revision.Jun 3 2017, 10:39 AM

Thanks, I made a couple of comments inline.

include/llvm/DebugInfo/DWARF/DWARFDie.h
123 ↗(On Diff #101323)

This line appears to be > 80-columns. Did you run the patch through clang-format?

lib/DebugInfo/DWARF/DWARFDie.cpp
82 ↗(On Diff #101323)

clang-format again :-)

133 ↗(On Diff #101323)

extra whitespace

363 ↗(On Diff #101323)

Looks like this was accidentally left in?

tools/llvm-dwarfdump/llvm-dwarfdump.cpp
86 ↗(On Diff #101323)

I assume that eventually we will want to hide more than just DW_FORMs (e.g., similar to Darwin dwarfdump's output) so it might be better to just generally say something like "Print fewer low-level details".

Personally, I would like this mode to become the default, and then introduce a --verbose option to get to the original output, but we can have that discussion another time.

This revision now requires changes to proceed.Jun 3 2017, 10:39 AM
aprantl added inline comments.Jun 3 2017, 10:40 AM
test/tools/llvm-dwarfdump/brief.s
4 ↗(On Diff #101323)

Maybe add a comment here explaining what the test is checking:
# Verify that --brief hides DW_FORMs.

dblaikie added inline comments.Jun 3 2017, 10:55 AM
test/tools/llvm-dwarfdump/brief.s
4 ↗(On Diff #101323)

Might be worth checking a whole attribute line to demonstrate the output format more clearly. (arguably this test could pass if no attributes were dumped at all, for example)

probinson added inline comments.Jun 5 2017, 9:58 AM
lib/DebugInfo/DWARF/DWARFDie.cpp
331 ↗(On Diff #101323)

Indentation? (run clang-format ...)

test/tools/llvm-dwarfdump/lit.local.cfg
1 ↗(On Diff #101323)

All llvm-dwarfdump tests are in target-specific subdirectories; please move this file and your new test to a new X86 subdirectory.

tools/llvm-dwarfdump/llvm-dwarfdump.cpp
86 ↗(On Diff #101323)

I agree with Adrian. In fact the comment is already incorrect, the option hides abbrev codes as well as forms.

probinson edited edge metadata.Jun 5 2017, 10:02 AM
probinson added a subscriber: llvm-commits.

Subscribe llvm-commits

sgravani updated this revision to Diff 101488.Jun 5 2017, 5:42 PM
sgravani edited edge metadata.

Addressed the reviewers' comments:

  1. Used clang-format on patch
  2. Updated description of --brief
  3. Moved test case to X86 subdirectory in llvm-dwarfdump test directory
  4. Added an integer declaration in the test case

It looks like you accidentally created a diff between your last patch and the new version. When uploading a revised patch, please always upload the diff from svn trunk.

test/tools/llvm-dwarfdump/X86/brief.s
7 ↗(On Diff #101488)

What David meant here was that
CHECK-NOT: DW_FORM
is also true for empty output.
If we instead check for

CHECK: DW_TAG_compile_unit
CHECK-NOT: DW_FORM
CHECK: DW_AT

or (probably better)

CHECK: DW_TAG_compile_unit {{$}}

We match an entire line of output and the test case is more robust.

sgravani updated this revision to Diff 101589.Jun 6 2017, 11:03 AM

Updated to the diff to be from the svn trunk.
Modified test case to be more robust.

aprantl accepted this revision.Jun 6 2017, 2:50 PM

Thanks, this LGTM now.

This revision is now accepted and ready to land.Jun 6 2017, 2:50 PM
This revision was automatically updated to reflect the committed changes.