This is an archive of the discontinued LLVM Phabricator instance.

[DWARFv5] Parse new line-table header format.
ClosedPublic

Authored by probinson on May 1 2017, 11:57 AM.

Details

Summary

The directory and file tables now have form-based content descriptors.
Parse these and extract the per-directory/file records based on the
descriptors. For now we support only DW_FORM_string (inline) for the
path names; follow-up work will add support for indirect forms (i.e.,
DW_FORM_strp, strx<N>, and line_strp).

Diff Detail

Event Timeline

probinson created this revision.May 1 2017, 11:57 AM
aprantl edited edge metadata.May 1 2017, 12:48 PM

seems generally fine, couple of stylistic nits inline.

include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
46

These should probably all be ///. Perhaps in a separate commit first? (no need to go through phabricator)

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
30

Would a struct with named members be more readable?

84

If this is accurate, I would prefer calling this parseV2DirFileTables

85

As a general comment, none of the variable/parameter names follow the LLVM coding style (capitalization).

89

not super important, but LLVM style would be

if (!s || !s[0])
      break;
IncludeDirectories.push_back(s);
98

same here

133

extra newline

dblaikie added inline comments.May 1 2017, 1:18 PM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
90–93

Hmm - getCStr has to find the null terminator anyway (to move the offset forward) - perhaps it'd be good to have a version that returns StringRef so the length doesn't need to be recomputed? & then use that here:

if (s.empty())
  break;
IncludeDirectories.push_back(s);

(reducing indentation, etc)

97–98

Same thing here - and much more code to benefit from unindenting

134–136

Would it be reasonable to have parseV5EntryFormat return the vector instead of taking an out parameter? (& have empty represent failure - or if necessary, ErrorOr<SmallVector>)

159–160

Looks like this only happens if Descriptors doesn't contain DW_LNCT_path - worth checking for that separately/up-front to be more explicit?

171

Might be better to use a different variable name here to make it clear that these are separate things with separate sizes, etc. (maybe even bundling up the file extraction and directory extracton into separate helper functions)

172

Generally != EntryCount rather than < EntryCount.

& the loop may be better optimized with int rather than unsigned, unfortunately :)

177

Any chance Descriptor.second could be typed as dwarf::Form to begin with to avoid the need for a cast here?

200

When would this condition occur? Looks like the only way through the loop that doesn't add an element to FileNames is the "return false" for extractValue failure (& the other return false for an earlier error) so the loop necessarily adds an element for every iteration of EntryCount times, etc.

test/DebugInfo/Inputs/dwarfdump-header.s
205

indent?

231

indent?

probinson marked 12 inline comments as done.May 1 2017, 6:00 PM

I sent DWARFDebugLine to the stylist in rL301883. Review for StringRef-ized paths in D32728. Other requested changes in-plan where indicated.
Likely more as well but it's time to go home.

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
159–160

I can have parseV5EntryFormat check that DW_LNCT_path is present, then the only failure mode is if extractValue or skipValue returns false, and this check becomes unnecessary.

test/DebugInfo/Inputs/dwarfdump-header.s
205

I think Emacs snuck a tab in there on me.

231

Tab.

probinson marked 8 inline comments as done.May 2 2017, 12:44 PM

Review for StringRef-ized paths in D32728.

rL301940.

So, with the mechanical stuff out of the way, back to the real change.

probinson updated this revision to Diff 97487.May 2 2017, 12:46 PM

Refresh to catch up with NFC changes, address review comments.

Thanks! I'll leave this to David to mark as accepted.

dblaikie accepted this revision.May 2 2017, 2:33 PM
dblaikie added inline comments.
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
137

Use the ContentDescriptors typedef here? (I'd probably skip the typedef/not use it at all - but if you're going to use it, probably makes sense to use it here?)

This revision is now accepted and ready to land.May 2 2017, 2:33 PM
probinson marked an inline comment as done.May 2 2017, 2:52 PM
probinson added inline comments.
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
137

Doh! Meant to use it here, and also for FileDescriptors. With the type being used in 4 places it seemed like a typedef would be reasonable.

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.