This is an archive of the discontinued LLVM Phabricator instance.

Add macro support to llvm-dwarfdump tool
ClosedPublic

Authored by aaboud on Nov 3 2015, 10:25 AM.

Details

Summary

Added "macro" option to "-debug-dump" flag, which trigger parsing & dumping of the ".debug_macinfo" section.

Note: Once DWARF 5.0 is published this same option can be used to parse & dump ".debug_macro" section as well.

Diff Detail

Repository
rL LLVM

Event Timeline

aaboud updated this revision to Diff 39085.Nov 3 2015, 10:25 AM
aaboud retitled this revision from to Add macro support to llvm-dwarfdump tool.
aaboud updated this object.
aaboud added reviewers: echristo, dblaikie.
aaboud set the repository for this revision to rL LLVM.
aaboud added a subscriber: llvm-commits.
samsonov added inline comments.
include/llvm/DebugInfo/DWARF/DWARFContext.h
167 ↗(On Diff #39085)

Remove DWARFContext::

include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
10 ↗(On Diff #39085)

Please fix the header guard (LLVM_DEBUGINFO_DWARF_DWARFDEBUGMACRO_H)

30 ↗(On Diff #39085)

Looks like all entries contain either a string, or an integer (file number). Maybe, we can use a union of them?

33 ↗(On Diff #39085)

Why will uint16_t be enough?

lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
58 ↗(On Diff #39085)

You can just call data.getU8() and then switch on that, breaking from the loop if parsed value was 0.

67 ↗(On Diff #39085)

This wouldn't work: if the .debug_macinfo section is malformed, and record type is invalid (or it will be some value from new DWARF standard), this code will have undefined behavior. Generally this is the reason we don't use enums from Dwarf.h to hold types in parser library - we can't be sure that the values we parse will correspond to defined enum values.

71 ↗(On Diff #39085)

see below - probably you just need to return that parsing was unsuccessful. Probably (if you're abandoning parsing), you should clear all entries parsed so far.

88 ↗(On Diff #39085)

This is wrong: DW_MACINFO_vendor_ext is well described: it's a constant followed by a string - you should consume it, and ignore it.

92 ↗(On Diff #39085)

You don't really need std::move here: let the compiler add it if necessary.

95 ↗(On Diff #39085)

It's not nice when library prints some stuff to stderr(). You can either avoid diagnosing this error, or return some value from parse() indicating that parsing failed.

test/DebugInfo/debugmacinfo.test
28 ↗(On Diff #39085)

Please fix (here and in another places)

aaboud marked 5 inline comments as done.Nov 5 2015, 3:48 AM

Hi Alexey,
Thanks for your comments.
I addressed most of them, however, there are couple of them that I am not sure how to resolve.

Please, see my answers below.

include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
10 ↗(On Diff #39085)

I fixed this in this file.
However, all the headers files in this directory have the wrong guard name.

30 ↗(On Diff #39085)

I tried to do that before, but it does not work when one of the fields is a class like StringRef.
This is the error I an getting:

'llvm::DWARFDebugMacro::Entry::MacroStr' : illegal union member; type 'llvm::StringRef' has a user-defined constructor or non-trivial default constructor.

Should I use char* and length instead of StringRef? Or just keep the code like this?

33 ↗(On Diff #39085)

I really do not know.
I just copied that from "DWARFDebugLine.h", it is using uint16_t for File ID.

An unsigned integer indicating the identity of the source file

// corresponding to a machine instruction.
uint16_t File;

Should I use a bigger integer type?

lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
58 ↗(On Diff #39085)

I am not sure about that.
If I call data.getU8(), it will read and remove the first byte.
If that byte is not '\0', I cannot use it to calculate the Type, as the Type is a ULEB128.

Am I missing something?

67 ↗(On Diff #39085)

Once again, I was following other section implementation: "DWARFDebugLocDWO::parse()", it is casting to a dwarf enumeration.

Anyway, I just replaces the dwarf macro type with uint32_t.

71 ↗(On Diff #39085)

Yes, I missed the "return;" statement here.
But I do not think we need to clean the parsed entries, let the user get an indication on at what point the object file get corrupted.

Notice that "DWARFDebugFrame.cpp" is using llvm_unreachable for corrupted code!

92 ↗(On Diff #39085)

I will fix that in this file.
However, I copied it from "DWARFDebugLoc.cpp", maybe it should be fixed there as well.

95 ↗(On Diff #39085)

Totally agree.

I will skip this error check for this patch.
Because, as this llvm-dwarfdump tool /LLVMDebugInfoDwarf library does not support error result communication, I do not think that we should do it in this patch.

I see that there is a place to fix this code (the one added in this patch and the existing code for other sections), however, I believe it is out of the scope of this patch.

Do you think otherwise?

aaboud updated this revision to Diff 39339.Nov 5 2015, 3:50 AM
aaboud marked 2 inline comments as done.

Applied Alexey comments.

Please, let me know if there is anything else I should change.

samsonov added inline comments.Nov 6 2015, 5:04 PM
include/llvm/DebugInfo/DWARF/DWARFDebugMacro.h
11 ↗(On Diff #39339)

Right, thanks. The header guards in another files were probably not fixed properly when the files were moved.

31 ↗(On Diff #39339)

Let's use const char * here instead.

34 ↗(On Diff #39339)

It's unlikely that there will be more than 65536 files, but let's not introduce this restriction here: if we use union we can as well use uint64_t to keep the same size of Entry.

lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
31 ↗(On Diff #39339)

Will it work correctly with nullptr string if E.Type is unknown? We might just output DW_MACINFO_unknown (0x...) instead of dying here.

59 ↗(On Diff #39339)

Ah, sorry for confusion. ULEB128 encoding of zero is a single zero byte, so you can call

E.Type = data.getULEB128(&Offset);

and break the look if it's zero.

68 ↗(On Diff #39339)

Thanks! I agree, that this practice is not followed everywhere in DWARF parser at the moment, and hopefully we will fix it one day. It would be nice to have at least the new code follow this.

72 ↗(On Diff #39339)

I'd vote for eventually removing all llvm_unreachable from parsers... I agree that proper error reporting is out of the scope of this patch, but let's at least not print to stderr. You can just skip parsing the rest of the section, or clear all the entries parsed so far - I'm fine with either of this, as long as you document this in a method declaration in header.

89 ↗(On Diff #39339)

Hm, if .debug_macinfo in my file contained DW_MACINFO_vendor_ext, I would expect to see it in llvm-dwarfdump output.

96 ↗(On Diff #39339)

See above.

aaboud updated this revision to Diff 39677.Nov 9 2015, 3:48 AM
aaboud marked 7 inline comments as done.

Applied more changes according to Alexey comments.
The change Includes introducing a new DWARF Enumerator "DW_MACINFO_invalid".

Please, let me know if there is anything else I should change.

samsonov edited edge metadata.Nov 9 2015, 10:04 AM

Looks mostly good. A few remaining comments below.

lib/DebugInfo/DWARF/DWARFDebugMacro.cpp
24 ↗(On Diff #39677)

Do this only if IndLevel is positive

36 ↗(On Diff #39677)

you don't use dwarf:: namespace specified in another places, remove from here?

78 ↗(On Diff #39677)

Why do you cast it to unsigned? Line is uint64_t.

84 ↗(On Diff #39677)

see above

86 ↗(On Diff #39677)

see above - why uint16_t?

aaboud updated this revision to Diff 39896.Nov 11 2015, 3:42 AM
aaboud edited edge metadata.
aaboud marked 4 inline comments as done.

Thanks Alexey for catching these leftovers that I forgot to modify to fit the recent changes.

I hope that this patch is good for commit.
Do I have an approval to commit?

Thanks,
Amjad

samsonov accepted this revision.Nov 11 2015, 6:50 AM
samsonov edited edge metadata.

LGTM, thanks!

This revision is now accepted and ready to land.Nov 11 2015, 6:50 AM
This revision was automatically updated to reflect the committed changes.