Page MenuHomePhabricator

[ObjectYAML] NFC. Refactor DWARFYAML CompileUnit dump code

Authored by beanz on Feb 24 2017, 3:18 PM.



This patch refactors the DWARFYAML code for dumping compile units to use a visitor pattern. Using this design will, in the future, enable the DWARF YAML code to perform analysis and mutations of the DWARF DIEs. An example of such mutations would be calculating the length of a compile unit and updating the CU's Length field before writing the DIE. This support will make it easier to craft or modify DWARF tests by hand.

Diff Detail


Event Timeline

beanz created this revision.Feb 24 2017, 3:18 PM

It would be good to split this into an NFC patch that just moves stuff around and then a small commit+testcase for the new functionality. This makes reviewing easier & faster.

117 ↗(On Diff #89732)

Doxygenify all comments?

beanz updated this revision to Diff 90249.Mar 1 2017, 3:41 PM
  • Reducing the patch to just the NFC changes
beanz retitled this revision from [ObjectYAML] Add ability for DWARFYAML to calculate DIE lengths to [ObjectYAML] NFC. Refactor DWARFYAML CompileUnit dump code.Mar 1 2017, 3:45 PM
beanz edited the summary of this revision. (Show Details)
aprantl accepted this revision.Mar 2 2017, 11:13 AM

Sorry for the delay. Few comments inline, LGTM with these addressed.

19 ↗(On Diff #90249)


43 ↗(On Diff #90249)

Personally, I'd rather use a while loop here, but I'll leave that up to you.

48 ↗(On Diff #90249)

this is redundant

59 ↗(On Diff #90249)

This would be easier to read if written like this:

uint64_t?? writeSize;
if (!DebugInfo.DebugLines.empty())
  writeSize =  DebugInfo.DebugLines[0].TotalLength == UINT32_MAX ? 8 : 4;
else if (Unit.Version == 2)
  writeSize = Unit.AddrSize
  writeSize = 4;
138 ↗(On Diff #90249)

Can we factor this out into a tiny static helper function?

33 ↗(On Diff #90249)

A thing we do in other headers is:

/// Visitor Functions.
/// @{
  virtual void OnStartCompileUnit(Unit &CU) {}
  virtual void OnEndCompileUnit(Unit &CU) {}
  virtual void OnStartDIE(Unit &CU, Entry &DIE) {}
  virtual void OnEndDIE(Unit &CU, Entry &DIE) {}
  virtual void OnForm(AttributeAbbrev &AttAbbrev, FormValue &Value) {}
/// @}
This revision is now accepted and ready to land.Mar 2 2017, 11:13 AM
beanz added inline comments.Mar 2 2017, 1:04 PM
59 ↗(On Diff #90249)

There is actually some slightly incorrect logic here. The correct logic should be more like:

size_t writeSize = 4;
if (Unit.Version == 2)
  writeSize = Unit.AddrSize;
else if (!DebugInfo.DebugLines.empty())
  writeSize = DebugInfo.DebugLines[0].TotalLength == UINT32_MAX ? 8 : 4;

The idea being that size 4 as a default is sane. If version == 2, then you use AddrSize, otherwise it is based on whether it is DWARF32 or DWARF64 which requires the Line table.

138 ↗(On Diff #90249)

Yes, will do.

beanz updated this revision to Diff 90383.Mar 2 2017, 1:25 PM

Updates based on feedback from Adrian.

Few more inline comments, feel free to commit whenever ready.

38 ↗(On Diff #90383)

we probably want a target-independent type like uint64_t here?

why not make this static uint64_t getRefSize(DebugInfo, Unit) instead?

29 ↗(On Diff #90383)

We should also add a Doxygen comment here to explaining what this is.

aprantl added inline comments.Mar 2 2017, 2:09 PM
35 ↗(On Diff #90383)

Side note: Not sure how much work this is, but technically these should all start with lower case:

beanz added inline comments.Mar 2 2017, 2:18 PM
38 ↗(On Diff #90383)

I think using size_type here is arguably more correct, because it is a size value.

That said, since the value will never be larger than the max value of AddrSize (which is uint8_t), using uint64_t would be overkill, and has performance issues on some 32-bit hosts.

Do you have strong feelings on this?

probinson added inline comments.
59 ↗(On Diff #90249)

Why do you need the line table? Every unit has a length that lets you determine its 32/64 format.

aprantl added inline comments.Mar 2 2017, 3:43 PM
38 ↗(On Diff #90383)

If my memory serves correctly C doesn't guarantee that size_t is larger than unsigned.
I don't feel strongly about it and in this particular case it shouldn't matter. Personally I would probably use unsigned here.

beanz added inline comments.Mar 2 2017, 3:44 PM
59 ↗(On Diff #90249)

Reading the DWARF 4 spec (7.2.2 Initial Length Values) I believe you are correct that the unit's should start with a length that lets you determine the 32/64 format, however I don't believe that is how our parser is written:

It looks to me like we only ever read a 32-bit value for the length from the unit.

Am I misunderstanding something here?

beanz added inline comments.Mar 2 2017, 4:37 PM
59 ↗(On Diff #90249)

Answered this one for myself... not misunderstanding. LLVM's DWARF parser is handling DWARF64 wrong. That will need to get fixed.

beanz added inline comments.Mar 2 2017, 5:35 PM
59 ↗(On Diff #90249)

Also looks like we don't emit DWARF64 Units in LLVM. That seems like it could be bad:

probinson added inline comments.Mar 2 2017, 11:15 PM
59 ↗(On Diff #90249)

What do you mean by "could be bad"? We can consider it a bug that the code you cite does not guard against a unit too big for DWARF32 format, but failing to support DWARF64 seems like a missing feature rather than something inherently bad.

beanz added inline comments.Mar 3 2017, 8:47 AM
59 ↗(On Diff #90249)

I was perhaps being a bit glib there, and I apologize.

When I said it could be bad I was referring to what you more clearly stated; that we could be overflowing the size calculation on Units. In practice I'm sure that is highly unlikely.

beanz updated this revision to Diff 90502.Mar 3 2017, 9:48 AM

Updated based on feedback from Adrian.

I'm going to leave this patch briefly while I go off and fix up a bunch of DWARF32/DWARF64 issues in the DWARFYAML code. That will allow me to better accomodate Paul's feedback.

beanz updated this revision to Diff 90529.Mar 3 2017, 1:32 PM

Updating to address Paul's feedback about DWARF32/DWARF64.

My LGTM still stands, there's nothing left here that couldn't be fixed in a post-commit review. I found one more opportunity to simplify the code a bit inline.

93 ↗(On Diff #90529)

This chunk also looks like it could be factored out into a templated function.

This revision was automatically updated to reflect the committed changes.