Page MenuHomePhabricator

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

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



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

Here's an idea: How about adding a

template<typename 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

What happened to these error paths?


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)


Drop the const


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

(similarly in a few cases below)


What's this added/here for?


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.


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.


That is a good idea. I will do that.

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


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.

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


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

FYI, the LLVM coding standard prefers full sentences in comments with a trailing '.' (

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.