This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Versioning for DWARF constants; verify FORMs
ClosedPublic

Authored by probinson on Mar 9 2017, 10:18 AM.

Details

Summary

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).

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Mar 9 2017, 10:18 AM

+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).

dblaikie added inline comments.Mar 9 2017, 10:35 AM
lib/CodeGen/AsmPrinter/DIE.cpp
82–89 ↗(On Diff #91184)

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?

probinson added inline comments.Mar 9 2017, 10:49 AM
lib/CodeGen/AsmPrinter/DIE.cpp
82–89 ↗(On Diff #91184)

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.
But, it's true if we detect a problem here, that indicates an internal inconsistency rather than a problem with our external input, so on that basis an assertion would be more the thing.

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.
I would be okay with emitting the v4 attribute in earlier versions, but not the v4 FORM. We'd have to pick something other than FORM_ref_sig8 for that case.

probinson updated this revision to Diff 91223.Mar 9 2017, 2:46 PM

Change report_fatal_error to an assertion.
Always emit the type signature but select a version-appropriate FORM.

probinson marked 2 inline comments as done.Mar 9 2017, 2:54 PM

This doesn't necessarily solve the signature-reference problem, but using a version-appropriate FORM makes it "syntactically" valid.

clayborg edited edge metadata.Mar 10 2017, 4:28 PM

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.

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?

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.

probinson updated this revision to Diff 91764.Mar 14 2017, 1:04 PM

Use a separate vendor identifier instead of overloading version.

Much better, I am good with this. I will let Dave Blaikie give the final OK.

dblaikie added inline comments.Mar 17 2017, 9:00 AM
include/llvm/Support/Dwarf.h
420 ↗(On Diff #91764)

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 ↗(On Diff #91764)

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 ↗(On Diff #91764)

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?

probinson added inline comments.Mar 17 2017, 12:24 PM
include/llvm/Support/Dwarf.h
420 ↗(On Diff #91764)

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.
This means I think there is no concise contract to specify; it would have to be something along these lines:

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 ↗(On Diff #91764)

I will look into that. I put it here because that's where the check for implicit_const had been.

89 ↗(On Diff #91764)

I knew that didn't look right...

probinson added inline comments.Apr 10 2017, 4:41 PM
lib/CodeGen/AsmPrinter/DIE.cpp
85–90 ↗(On Diff #91764)

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.

probinson updated this revision to Diff 94905.Apr 11 2017, 4:22 PM

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.

probinson updated this revision to Diff 95306.Apr 14 2017, 9:17 AM

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.

probinson marked an inline comment as done.Apr 14 2017, 9:18 AM

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.

aprantl edited edge metadata.Apr 20 2017, 9:49 AM

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
82–89 ↗(On Diff #91184)

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?

This revision is now accepted and ready to land.Apr 20 2017, 10:21 AM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Support/Dwarf.cpp