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

Repository
rL LLVM

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 ↗(On Diff #97323)

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

lib/DebugInfo/DWARF/DWARFDebugLine.cpp
30 ↗(On Diff #97323)

Would a struct with named members be more readable?

84 ↗(On Diff #97323)

If this is accurate, I would prefer calling this parseV2DirFileTables

85 ↗(On Diff #97323)

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

89 ↗(On Diff #97323)

not super important, but LLVM style would be

if (!s || !s[0])
      break;
IncludeDirectories.push_back(s);
98 ↗(On Diff #97323)

same here

133 ↗(On Diff #97323)

extra newline

dblaikie added inline comments.May 1 2017, 1:18 PM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
90–93 ↗(On Diff #97323)

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 ↗(On Diff #97323)

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

134–136 ↗(On Diff #97323)

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 ↗(On Diff #97323)

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 ↗(On Diff #97323)

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 ↗(On Diff #97323)

Generally != EntryCount rather than < EntryCount.

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

177 ↗(On Diff #97323)

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

200 ↗(On Diff #97323)

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 ↗(On Diff #97323)

indent?

231 ↗(On Diff #97323)

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 ↗(On Diff #97323)

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 ↗(On Diff #97323)

I think Emacs snuck a tab in there on me.

231 ↗(On Diff #97323)

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
143 ↗(On Diff #97487)

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
143 ↗(On Diff #97487)

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.