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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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.
lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
117 ↗ | (On Diff #89732) | Doxygenify all comments? |
Sorry for the delay. Few comments inline, LGTM with these addressed.
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
19 ↗ | (On Diff #90249) | switch(Size) |
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 else writeSize = 4; |
138 ↗ | (On Diff #90249) | Can we factor this out into a tiny static helper function? |
lib/ObjectYAML/DWARFVisitor.h | ||
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) {} /// @} |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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. |
Few more inline comments, feel free to commit whenever ready.
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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? |
lib/ObjectYAML/DWARFVisitor.h | ||
29 ↗ | (On Diff #90383) | We should also add a Doxygen comment here to explaining what this is. |
lib/ObjectYAML/DWARFVisitor.h | ||
---|---|---|
35 ↗ | (On Diff #90383) | Side note: Not sure how much work this is, but technically these should all start with lower case: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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? |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
59 ↗ | (On Diff #90249) | Why do you need the line table? Every unit has a length that lets you determine its 32/64 format. |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
38 ↗ | (On Diff #90383) | If my memory serves correctly C doesn't guarantee that size_t is larger than unsigned. |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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: https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFUnit.cpp#L91 It looks to me like we only ever read a 32-bit value for the length from the unit. Am I misunderstanding something here? |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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. |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
59 ↗ | (On Diff #90249) | Also looks like we don't emit DWARF64 Units in LLVM. That seems like it could be bad: https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfUnit.cpp#L1549 |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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. |
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
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. |
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.
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.
lib/ObjectYAML/DWARFVisitor.cpp | ||
---|---|---|
93 ↗ | (On Diff #90529) | This chunk also looks like it could be factored out into a templated function. |