Page MenuHomePhabricator

[ dwarfdump ] Add symbolic dump of known DWARF attribute values.
ClosedPublic

Authored by friss on Sep 4 2014, 4:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

friss updated this revision to Diff 13248.Sep 4 2014, 4:59 AM
friss retitled this revision from to [ dwarfdump ] Add symbolic dump of known DWARF attribute values..
friss updated this object.
friss edited the test plan for this revision. (Show Details)
friss added a reviewer: friss.
friss edited reviewers, added: dblaikie, echristo, aprantl; removed: friss.Sep 4 2014, 5:04 AM
friss added a subscriber: Unknown Object (MLST).
dblaikie added inline comments.Sep 4 2014, 8:36 AM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
90 ↗(On Diff #13248)

Roll the "Val" variable into the if condition to reduce its scope:

if (Optional<uint64_t> Val = formValue.getAsUnsignedConstant())

Name = AttributeValueString(attr, Val.getValue());

(personally I prefer using the pointer-like interface of Optional (so I would write "*Val" rather than "Val.getValue()") but opinions differ and there's no solid convention in the codebase so far - so do whichever you prefer, I just mention it in case you aren't aware of it)

96 ↗(On Diff #13248)

Mistaken indentation here? (looks like a 4 space instead of a 2, but it might be the review software being weird)

lib/Support/Dwarf.cpp
799 ↗(On Diff #13248)

As a follow-up, if you like, all these functions should /probably/ be modified to return StringRef, that way we don't have to compute their length later on (or do null-test walks) in printing functions, etc.

799 ↗(On Diff #13248)

Should the type of Attr be something more type-safe? (the actual enum of DWARF attributes)

829 ↗(On Diff #13248)

This line is unreachable (so far as I can see?) and should be deleted.

(alternatively you could leave this one and remove the default case from the switch)

friss added inline comments.Sep 4 2014, 9:53 AM
lib/DebugInfo/DWARFDebugInfoEntry.cpp
90 ↗(On Diff #13248)

Will do. I wasn't aware of Optional::operator*. I'll use it.

96 ↗(On Diff #13248)

No the indentation is really off.

lib/Support/Dwarf.cpp
799 ↗(On Diff #13248)

I first wrote it like that, but I was finding the cast at the call site awkward. I can add it back, but we have no guarantee that the data we've read from the DWARF file will actually be a known Attribute, thus I find it more accurate to allow all values.

829 ↗(On Diff #13248)

I actually wondered while writing the code if all supported compilers could correctly do the control flow analysis that proves that the second return is unnecessary. I added the default case to prevent warnings about missed cases when I was using a typed enum Attr, but now the default case can just go away.

dblaikie accepted this revision.Sep 4 2014, 10:02 AM
dblaikie edited edge metadata.

Looks good with your responses (& implied/pending updates) to the last round of comments, please commit once you've made those changes.

lib/Support/Dwarf.cpp
799 ↗(On Diff #13248)

Fair enough.

This revision is now accepted and ready to land.Sep 4 2014, 10:02 AM
friss closed this revision.Sep 4 2014, 12:49 PM
friss updated this revision to Diff 13280.

Closed by commit rL217186 (authored by @friss).