Associate the version-when -defined with definitions of DWARF constants.
Use this to verify FORMs in .debug_abbrev are defined in the version for the associated unit.
Fixed a bug where we emitted FORM_ref_sig8 for older DWARF versions.
Removed a test that specified DWARF v1 (which essentially does not exist).
Details
Diff Detail
Event Timeline
+llvm-commits. Repeating the summary for benefit of the email:
Associate the version-when -defined with definitions of DWARF constants.
Use this to verify FORMs in .debug_abbrev are defined in the version for the associated unit.
Fixed a bug where we emitted FORM_ref_sig8 for older DWARF versions.
Removed a test that specified DWARF v1 (which essentially does not exist).
lib/CodeGen/AsmPrinter/DIE.cpp | ||
---|---|---|
84–91 | Should this be an assertion instead? | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
918–919 ↗ | (On Diff #91184) | This is probably incorrect (short of the broader discussion I started about how references to type units should be implemented - I should ping that thread). If the user requests type units, either it should fail if asking for a DWARF version that doesn't support type units - or ignore the conflict and produce the signature anyway? |
lib/CodeGen/AsmPrinter/DIE.cpp | ||
---|---|---|
84–91 | I dithered on that. An assertion would tell you "something bad happened somewhere" with no details, because there's no way I know of to report a runtime value in an assertion. So you don't know what FORM is causing the problem. Doing it this way was more valuable when figuring out the test failures that I had to fix. | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
918–919 ↗ | (On Diff #91184) | The signature attribute was coming out even without type units. This was added by Adrian for "external" types, something to do with modules. |
Change report_fatal_error to an assertion.
Always emit the type signature but select a version-appropriate FORM.
This doesn't necessarily solve the signature-reference problem, but using a version-appropriate FORM makes it "syntactically" valid.
I wonder since we are adding new macro parameters to some macros if we shouldn't add two params: version and vendor for anything that uses the special versions. Currently you are overloading version with vendor into a single integer, why not add both version and vendor? Then vendor could be
DWARF_VENDOR_DWARF = 0, // DWARF as ratified by the latest DWARF standard DWARF_VENDOR_APPLE = 1, DWARF_VENDOR_BORDLAND = 2, ...
This would still allow vendors to specify a version value for their user enums.
Other than that it looks nice. It will really help with "llvm-dwarfdump --verify" in the future.
As Blaikie said in email, extensions are unlikely to be versioned like standard values, and it seemed to me there was no useful value to put as the version for an extension. Also it adds yet another set of APIs to retrieve the values. But as the admittedly minor code bloat is about the only downside, I'll go add that all in on Monday.
include/llvm/Support/Dwarf.h | ||
---|---|---|
420 | Not sure this comment quite makes sense (which is it, most or all? & it doesn't describe the contract). I'd be OK with "most" or "legacy" or some similar language? | |
lib/CodeGen/AsmPrinter/DIE.cpp | ||
85–90 | Might it be more helpful to fail at the point where the DIEAbbrev is created, rather than emitted? (more debuggable - the smoking gun, as it were) - would also save/avoid the need to plumb the dwarf version down through some of the layers added in this change. | |
89 | llvm_unreachable is preferred over assert(0) | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
297–298 ↗ | (On Diff #91764) | Still have uncertainty about this change - type units are only supported in DWARF4 and above. I think you were saying this was used by a particular prototype of modular debug info that ended up being a dead-end and Adrian removed. Is this still needed? |
include/llvm/Support/Dwarf.h | ||
---|---|---|
420 | The policy for different constants will vary widely. For example, DW_AT_MIPS_linkage_name really should be used only *before* the equivalent standard attribute existed. The GNU forms for split DWARF should be used only in v4 with experimental split-DWARF. The GNU TLS opcode should be used only with GDB tuning. For constants defined in the DWARF standard, returns the version when the constant was first defined. For vendor extensions, returns a number that might be useful in policy decisions about when the constant can be used. | |
lib/CodeGen/AsmPrinter/DIE.cpp | ||
85–90 | I will look into that. I put it here because that's where the check for implicit_const had been. | |
89 | I knew that didn't look right... |
lib/CodeGen/AsmPrinter/DIE.cpp | ||
---|---|---|
85–90 | The form/value information is created by DIE::addValue() (inherited from DIEValueList::addValue()). These places do not have direct access to the DWARF version, we'd have to follow the DIE parent chain up to the unit. Is that cheap enough to do every time we add any attribute to any DIE? Alternatively, caching the version in the DIE makes every DIE a word bigger. Or leave the check here. (There's currently no "plumbing" of the version down through layers in this patch, except for dsymutil which for correctness has to remember the max version number it has seen anyway. Moving the check to DIEValueList would introduce some plumbing.) | |
lib/CodeGen/AsmPrinter/DwarfUnit.cpp | ||
297–298 ↗ | (On Diff #91764) | Now that Adrian has removed the other usage, yes this is no longer necessary. |
Move the version check to DIE::addValue(). This required making the base-class method virtual, because the base class doesn't have a way to find the DWARF version; only a DIE can do that.
Other review comments addressed as well.
Move the form-version check back to DIEAbbrev::Emit. It's cheaper to check there, and with debugging output we should have enough breadcrumbs to figure out where the problem is coming from.
Ping. @dblaikie are you okay with putting the check back where it was? It's clearly cheaper and my experience is that it's not super hard to find the problem given the information available at that point.
Besides my above comment, I'm very much looking forward to this commit. It will also help a lot with implementing dwarfdump -verify functionality.
lib/CodeGen/AsmPrinter/DIE.cpp | ||
---|---|---|
84–91 | This is something I'm still getting confused about. Doesn't llvm_unreachable() give the compiler the license to optimize away everything in the path leading up to the unreachable? Would report_fatal_error() be more appropriate here? |
Not sure this comment quite makes sense (which is it, most or all? & it doesn't describe the contract). I'd be OK with "most" or "legacy" or some similar language?