Page MenuHomePhabricator

Change DWARF parser to use enumerations for DWARF tags, attributes and forms.
ClosedPublic

Authored by clayborg on Oct 26 2016, 2:54 PM.

Details

Summary

To improve our ability to debug DWARF code I propose that we switch all variables for tags, attributes and forms over to use the llvm::dwarf enumerations instead of using uint16_t values. This allows easier debugging as users can see the values of the enumerations in the variables view that will show the enumeration string instead of just a number.

Diff Detail

Event Timeline

clayborg updated this revision to Diff 75952.Oct 26 2016, 2:54 PM
clayborg retitled this revision from to Change DWARF parser to use enumerations for DWARF tags, attributes and forms..
clayborg updated this object.
clayborg set the repository for this revision to rL LLVM.
aprantl added inline comments.Oct 26 2016, 3:08 PM
lib/DebugInfo/DWARF/DWARFFormValue.cpp
126

Here's an idea: How about adding a

template<typename EnumType>
llvm::Optional<EnumType>
getULEB128AsEnum() {
  // Decode ULEB.
  uint64_t Val = ...; // what about larger values?
  if (Val >= EnumType::getMinValue() && Val <= EnumType::getMaxValue())
    return static_cast<EnumType>(Val);
  return None;
}

?

This way we could guarantee that on of the enums if it is passed as an enum is always well-formed. Then we would never need a default case in a switch over a dwarf enum and could benefit from the not-all-enum-cases-covered-by-switch warning.

Is that feasible?

dblaikie added inline comments.Oct 26 2016, 3:09 PM
include/llvm/DebugInfo/DWARF/DWARFAbbreviationDeclaration.h
46–47
lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp
42–45

What happened to these error paths?

44–52

Drop the const from local values - we don't generally do that (& can be confused with "const T&" - might be read as though a reference was intended & accidentally omitted)

Also, if you like, you can use "auto" for the type when the RHS has the type prominently in the static_cast type parameter anyway.

(as you did later with AtomForm)

lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp
211

Drop the const

237

I'd probably drop the const here, even though it was there in the original.

(similarly in a few cases below)

lib/DebugInfo/DWARF/DWARFFormValue.cpp
126–127

What's this added/here for?

275–276

Perhaps it'd work to have enums defined for the beginning of the user-defined extension space - and just never add support for those to these switches, then they wouldn't be fully covered switches and the warning wouldn't fire on the default?

I will make the changes dblaikie suggested.

lib/DebugInfo/DWARF/DWARFFormValue.cpp
125–128

This was added because the switch statement was on a uint16_t before and now it is on a dwarf::Form enumeration. It will warn that not all cases were handled. The cases don't include all cases because most case are handled above the current switch statement. So this was added to avoid a "not all enum cases were handled.

275–276

That is a good idea. I will do that.

Added clarification on the "where did this go" in inlined comments.

lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp
41–45

They are combined into the "else" clause on line 51 on the right. No need to check that nothing was parsed by comparing the offset. If there is no data, zero will be returned. So the changes on the right check the success case first:

if (A && F)

then check if both are zero, which is the success case in the middle:

} else if (A == 0 && F == 0) {

And then check of either A or F was zero which is the error case.

clayborg updated this revision to Diff 75971.Oct 26 2016, 5:39 PM
clayborg removed rL LLVM as the repository for this revision.

Fixed all requests.

clayborg marked 5 inline comments as done.Oct 26 2016, 5:41 PM

Updated diffs with all requested changes

dblaikie accepted this revision.Oct 26 2016, 9:55 PM
dblaikie edited edge metadata.
dblaikie added inline comments.
lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp
49

Maybe "Finished" rather than "Successfully" (or "Successfully finished") - or perhaps "Reached the end-of-abbreviation marker" or something.

52

Maybe there's a better way to phrase this comment (I get what you're trying to say, but it parses a bit oddly for my brain at least - "both which is an error" almost sounds like it's referring to the previous case, which isn't an error but the end-of-input marker).

This revision is now accepted and ready to land.Oct 26 2016, 9:55 PM
aprantl added inline comments.Oct 27 2016, 8:56 AM
lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp
52

FYI, the LLVM coding standard prefers full sentences in comments with a trailing '.' (http://llvm.org/docs/CodingStandards.html#commenting).

clayborg updated this revision to Diff 76056.Oct 27 2016, 9:34 AM
clayborg edited edge metadata.

Improved comments in the abbreviation declaration.

clayborg marked 3 inline comments as done.Oct 27 2016, 9:34 AM

Fixed comments and marked them as done.

clayborg closed this revision.Oct 27 2016, 9:42 AM

SVN revision 285309.