Page MenuHomePhabricator

[DWARFFormValue] Don't consider DW_FORM_data4/8 to be section offsets.
ClosedPublic

Authored by JDevlieghere on Feb 26 2019, 2:15 PM.

Details

Summary

When dumping ToT clan's debug info with dwarfdump, we were seeing an error
saying that that the location list overflows the debug_loc section. After
reducing the testcase we figured out that we were interpreting the
DW_FORM_data4 as a section offset.

In DWARF3 DW_FORM_data4 and DW_FORM_data8 served also as a section offset.
Until now we didn't check check for the DWARF version, because some producers
(read old versions of clang) were still emitting this. The relevant
code/comment was added in 2013, and I believe it's now reasonable to start
checking the version.

The FormValue class is a little bit of a mess because it cashes the DWARF unit
and context when it extracted the value itself. Several methods of the class
rely on it being present, or return an Optional for the code path that needs
it. At the same time the FormValue class also used in places where there's no
DWARF unit.

For this patch I went with the least invasive change: checking the version from
the CU when it's available. Possible other (more correct?) alternatives are
storing the DWARF version in the FormValue class, or making it a required
argument to the isFormClass method. Neither are particularly attractive. Please
let me know what you think is best.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Feb 26 2019, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 26 2019, 2:15 PM
aprantl added inline comments.Feb 26 2019, 2:28 PM
llvm/test/tools/llvm-dwarfdump/X86/formclass2.s
14 ↗(On Diff #188456)

--name g ?

382 ↗(On Diff #188456)

Should we just drop the apple types section from the assembler input?

aprantl added inline comments.Feb 26 2019, 2:29 PM
llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
200 ↗(On Diff #188456)

"some producers" here means older versions of clang, but since older versions of clang also default to DWARF 2 (at least on Darwin) I think this is a safe change.

aprantl added inline comments.Feb 26 2019, 2:52 PM
llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
200 ↗(On Diff #188456)

To clarify: what we are dropping here is support for compilers that claim to emit DWARF 3+ but still is a FORM_data4 for a section offset. I'm fine with dropping support for these.

I'm not a fan of DWARFFormValue optionally having a DWARFUnit pointer. This is asking for trouble. If we could assert that we have on in this condition (or get rid of the optionality) I'd be much happier.

dblaikie added inline comments.Feb 26 2019, 2:55 PM
llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
200 ↗(On Diff #188456)

I mean... we could drop support for this convenience feature when dumping DWARFv3 and older entirely?

llvm/test/tools/llvm-dwarfdump/X86/formclass2.s
3–4 ↗(On Diff #188456)

I'd consider making these both chars, just to make it clear that their type isn't interesting, perhaps.

6–9 ↗(On Diff #188456)

I see this is necessary to produce a debug_loc section in the output. Perhaps that could be commented and/or maybe there's a more direct way to do it? (and/or maybe it'd be worth separating the need for a debug_loc section (& the code that tickles it sufficiently to occur) from the code that ensures 'e' is emitted in the DWARF)

Also - any idea why there's on an error emitted when debug_loc is present but too small? Should we emit an error even when it's not present? I mean, I can see the counterargument (could've stripped/objcopied this one section for test purposes, etc)

Also - 16384 seems rahter large. Any idea why the offset of 'g' must be /that/ large before the warning is emitted? oh... because it turns it into a data4, rather than a data2, etc. OK

aprantl added inline comments.Feb 26 2019, 3:01 PM
llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
200 ↗(On Diff #188456)

I mean... we could drop support for this convenience feature when dumping DWARFv3 and older entirely?

Probably not. This is generic libDebugInfo code so it's not a convenience feature, but necessary functionality. I ran across this bug in dsymutil, too, for example.

dblaikie added inline comments.Feb 26 2019, 3:05 PM
llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
200 ↗(On Diff #188456)

Ah, fair enough

JDevlieghere marked 8 inline comments as done.

Code review feedback.

ormris removed a subscriber: ormris.Mar 5 2019, 2:51 PM
aprantl accepted this revision.Mar 5 2019, 3:08 PM
This revision is now accepted and ready to land.Mar 5 2019, 3:08 PM

Is there a way to make the DWARF unit non-optional in this function?

This revision was automatically updated to reflect the committed changes.

Is there a way to make the DWARF unit non-optional in this function?

Sorry, I landed this before seeing your comment. No, unfortunately it's not. The way the DWARFForm class works is that you can either initialize it yourself with a value (in which case there is no context or unit) or you can read it from data (in which case both are set). The only way to guarantee that we have a DWARF version in both scenarios would be to make it mandatory to pass a version number, which would become possible as the result of my recent refactoring.