This is an archive of the discontinued LLVM Phabricator instance.

Clean up DWARFFormValue by reducing duplicated code and removing DWARFFormValue::getFixedFormSizes()
ClosedPublic

Authored by clayborg on Nov 10 2016, 2:31 PM.

Details

Summary

In preparation for a follow on patch that improves DWARF parsing speed, clean up DWARFFormValue so that we have can get the fixed byte size of a form value given a DWARFUnit or given the version, address byte size and dwarf32/64.

This patch cleans up code so that everyone is using one of the new DWARFFormValue functions:

static Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form Form, const DWARFUnit *U = nullptr);
static Optional<uint8_t> DWARFFormValue::getFixedByteSize(dwarf::Form Form, uint16_t Version, uint8_t AddrSize, bool Dwarf32);

This patch changes DWARFFormValue::skipValue() to rely on the output of DWARFFormValue::getFixedByteSize(...) instead of duplicating the code in each function. This will reduce the number of changes we need to make to DWARF to fewer places in DWARFFormValue when we add support for new form.

This patch also starts to support DWARF64 so that we can get correct byte sizes for forms that vary according the DWARF 32/64.

To reduce the code duplication a new FormSizeHelper pure virtual class was created that can be created as a FormSizeHelperDWARFUnit when you have a DWARFUnit, or FormSizeHelperManual where you manually specify the DWARF version, address byte size and DWARF32/DWARF64. There is now a single implementation of a function that gets the fixed byte size (instead of two where one took a DWARFUnit and one took the DWARF version, address byte size and DWARFFormat enum) and one function to skip the form values.

Diff Detail

Event Timeline

clayborg updated this revision to Diff 77555.Nov 10 2016, 2:31 PM
clayborg retitled this revision from to Clean up DWARFFormValue by reducing duplicated code and removing DWARFFormValue::getFixedFormSizes().
clayborg updated this object.
dblaikie added inline comments.Nov 10 2016, 2:43 PM
lib/DebugInfo/DWARF/DWARFFormValue.cpp
64

Maybe '= default' rather than {}, though I don't think it makes a difference.

Also probably best to = delete the copy/move ops (the copy ops are the only ones that are a real concern, the move ones are already implicitly not provided due to the presence of this user declared dtor) to avoid any risk of slicing.

78

Remove this - it's implied/the default.

106

This is the implicit default, you can omit it entirely.

120

should probably be llvm_unreachable (LLVM might flag this code as unreachable, since the above switch is fully covering all the enum values - and we'll have a warning/error if a new enum value is added that's not covered in the switch)

124

LLVM style is to mark file-local functions as 'static' rather than placing them in an anonymous namespace (but classes, like those above, go in the anonymous namespace as they are).

191

skip the return after unreachable - it should be flagged as unreachable/dead by some compiler warnings

293

Oh, I see - this type doesn't actually need a virtual dtor (it's never destroyed polymorphically - it could just have a protected dtor in the base class, and make derived classes final (so there's no possibility of accidental polymorphic ownership/destruction).

Also - perhaps we could just make getFixedByteSize a template (templated on the FSH type) & avoid the virtual calls? (remove the base class, and just have FormSizeHelper* have non-virtual functions but otherwise the same as they are today)

But maybe this polymorphism (static or dynamic) isn't really necessary - maybe getFixedByteSize needs a struct with the 3 values (FormSizeHelperManual, basically) & just a way to construct FormSizeHelperManual from a DWARFUnit? Is it worth delaying querying the DWARFUnit until the implementation of getFixedByteSize compared to just calling it at the call-site to getFixedByteSize?

clayborg updated this revision to Diff 77561.Nov 10 2016, 3:13 PM

Fixed David Blaikie's issues.

clayborg marked 6 inline comments as done.Nov 10 2016, 3:16 PM

Marked things as done. Let me know if this is enough or if you have objections to the FormSizeHelper class. I think it is better than extracting 3 values that you are almost 90% of the time never use. The construction of FormSizeHelperDWARFUnit is very cheap and it will be 2 pointers in size so it shouldn't be too bad.

lib/DebugInfo/DWARF/DWARFFormValue.cpp
120

Yeah, that is why I left it out as I figured the warning for a missing value would show up, but I can add an llvm_unreachable here.

292–293

Yeah I thought about these issues. I didn't want to always extract 3 values for each form we are going to skip since a large percent of the forms will never touch or access the data in the first place. So I kind of like the laziness of the current implementation because most forms don't need to access the DWARFUnit or the Version/AddrSize/Format.

I can remove the virtual destructor and declare the two classes final though. You ok with that?

dblaikie edited edge metadata.Nov 10 2016, 3:23 PM

I'm not sure I understand the motivation for laziness here - the three things are direct accessors, right? (not virtual, don't do any non-trivial work, should basically be inlined away (one of them has a conditional operator))

I would be a bit surprised if the difference in laziness ever showed up in a profile, but perhaps I'm misunderstanding something about the characteristics of this code/usage/etc.

aprantl accepted this revision.Nov 10 2016, 3:33 PM
aprantl edited edge metadata.

Thanks, I think this is looking good now!
(with outstanding inline comments addressed)

include/llvm/Support/Dwarf.h
436

///

lib/DebugInfo/DWARF/DWARFFormValue.cpp
61

///

97

///

196

Form

197

OffsetPointer or OffsetPtr

198

Indirect

202

.

253

S ... .

tools/llvm-dwp/llvm-dwp.cpp
136

I think this is file might be using the llvm namespace already?

unittests/DebugInfo/DWARF/DWARFFormValueTest.cpp
27

.

34

.

This revision is now accepted and ready to land.Nov 10 2016, 3:33 PM

In order to skip a form to access the 10th attribute, we need to skip 9 forms. Each one of those will individually extract 3 piece of data for which 2 surely never be used and 1 might be used 10% of the time. So it seems it would be worth being lazy if we can especially since it is so easy to do.

clayborg updated this revision to Diff 77566.Nov 10 2016, 4:05 PM
clayborg edited edge metadata.

Ok, I think this will keep David happy. I templatized the static functions:

template <class T> static Optional<uint8_t> getFixedByteSize(dwarf::Form Form, const T *U);
template <class T> static bool skipFormValue(dwarf::Form Form, const DataExtractor &DebugInfoData, uint32_t *OffsetPtr, const T *U);

This means they can be called directly with a "const DWARFUnit *" or we can make a local FormSizeHelper instance and pass the address of that. The template assumes that the following functions can be call on T:

class T {
public:
  uint8_t getAddressByteSize() const;
  uint8_t getRefAddrByteSize() const;
  uint8_t getDwarfOffsetByteSize() const;
};

FormSizeHelper doesn't have a vtable anymore and the template functions will ensure I get the laziness that I was trying for.

I also fixed all of the things Adrian asked for.

clayborg closed this revision.Nov 22 2016, 9:28 AM

Committed with SVN revision 286597.