Page MenuHomePhabricator

[DebugInfo]: Added support for DWARFv5 Info section header parsing in llvm-dwp utility.
ClosedPublic

Authored by SouraVX on Feb 11 2020, 10:47 AM.

Details

Summary

This patch teaches llvm-dwp to parse DWARFv5 info section header. Tested this using asm test case caontaining DWARFv5 info. Assemling it to DWO object, checking corresponding content using llvm-dwarfdump. Then finally, packaging it to DWP using llvm-dwp and checking corresponding content using llvm-dwarfdump.

This patch is splitted from D74312, based on suggestions by @dblaikie

Diff Detail

Event Timeline

SouraVX created this revision.Feb 11 2020, 10:47 AM
Herald added a project: Restricted Project. · View Herald Transcript
dblaikie added inline comments.Feb 11 2020, 11:07 AM
llvm/test/tools/llvm-dwp/X86/info-v5.s
26

Remove all the non-dwo sections (so the text section, and the debug sections that aren't dwo sections, etc).

70

Correct indentation. (elsewhere in this too - line up the assembly directives and the comments)

llvm/tools/llvm-dwp/llvm-dwp.cpp
156

Might be worth specifying the unit type that was found? At least even if it's just the integer value "unexpected unit type 0x3 found in debug_info".

This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)

161–164

DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)

SouraVX updated this revision to Diff 243956.Feb 11 2020, 12:28 PM

Addressed comments by @dblaikie

SouraVX marked an inline comment as done.Feb 11 2020, 12:34 PM
SouraVX added inline comments.
llvm/tools/llvm-dwp/llvm-dwp.cpp
156

Wrapped this as string, since DWPError expects a string.

This should also be tested. (the existing test case could have a second CU in it that just has an incorrect unit type in its header)

I've tested it locally, error production works fine. Should I also add negative test case.?
If I add this in existing test case, then DWP packaging will detect/produce error and we'll loose primary verification for positive/Everthing fine case/ working llvm-dwp.

dblaikie added inline comments.Feb 11 2020, 12:38 PM
llvm/test/tools/llvm-dwp/X86/info-v5.s
25

Some indentation still needs fixing in this file - perhaps there's a tab/space issue, I'm not sure? But if you examine it here on the web you can see several are misaligned (DWARF version number comment, produce, language, name, dwo_name comments, external attribute, etc... )

llvm/tools/llvm-dwp/llvm-dwp.cpp
156

Yes, please add a test case. If adding it to the same file invalidates the existing test coverage there, then please add it as a separate test case.

161–164

DWARFv5 reorders the abbreviation offset and the address size - so this code is /probably/ wrong/needs support for this part of DWARFv5. But that could wait for another patch (might be worth checking how the failure to parse that correctly is manifesting - if there's a good spot to demonstrate it in the test case by adding an extra CHECK line and a FIXME saying "hey, this is broken here/fix it soon" would be good)

Any thoughts on this ^ ?

SouraVX marked an inline comment as done.Feb 11 2020, 12:41 PM
SouraVX added inline comments.
llvm/tools/llvm-dwp/llvm-dwp.cpp
161–164

Yes, I noticed this thing in Spec couple of times. But since llvm-dwarfdump is also not detecting while parsing, I took it as an expected/planned behavior.

SouraVX marked an inline comment as done.Feb 11 2020, 12:43 PM
SouraVX added inline comments.
llvm/test/tools/llvm-dwp/X86/info-v5.s
26

Strange, locally it's showing fine :)
Meanwhile, I'll add a negative test case, see if it works out second time.

SouraVX updated this revision to Diff 243985.Feb 11 2020, 1:36 PM

Increased test coverage by adding negative test case.

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

Any chance there are tabs rather than spaces, or the wrong amount of tabs, or something? (I guess LLVM's assembly does have leading tabs and tabs between assembly directive and argument - but usually is pure spaces after that to the comment - so maybe you've got some tabs in unintended places)

Indentation is still a bit looking odd here, but it's fine on my machine and VI editor.

Any chance there are tabs rather than spaces, or the wrong amount of tabs, or something? (I guess LLVM's assembly does have leading tabs and tabs between assembly directive and argument - but usually is pure spaces after that to the comment - so maybe you've got some tabs in unintended places)

I'll look again closely. BTW, are you Okay with overall content of this patch ?

SouraVX updated this revision to Diff 243997.Feb 11 2020, 2:10 PM

Indentation done, finally!

dblaikie added inline comments.Feb 11 2020, 2:22 PM
llvm/test/tools/llvm-dwp/X86/info-v5.s
5

Skip this - the DWARFv5 dwo files/dumping I think is already pretty well tested, I don't think there's much need to retest it here. (handy to test personally to validate some things, but probably not worth carrying as a checked in test)

10

Not sure what this check for debug_abbrev is? Check that it's present?

It might be more valuable to check the cu_index to check the contribution sizes of the various sections & demonstrate that the CU DWO ID was correctly parsed and inserted into the cu_index table. Please do that instead.

llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s
10

Rather than removing the unit type (& causing the address size to be read as the unit type) - please include an explicit unit type that is incorrect. I think that'll make the test easier to read - so it's obvious what value the error is referring to, rather than the comment misleadingly suggesting this is the address size field when it's really the unit type field.

llvm/tools/llvm-dwp/llvm-dwp.cpp
161–164

I don't think that's correct - it looks like this code is buggy but just not failing in a way that's been tested/examined yet.

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

The code should not be left as-is for very long because it is erroneously parsing the content & while the values may not currently be used (partly sub-optimal/possibly buggy, partly because they aren't needed) it's still a trap set to trip over anyone working with this code when that's not actually the order of fields in DWARFv5.

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

SouraVX updated this revision to Diff 244016.Feb 11 2020, 3:01 PM

Modified test cases.

SouraVX marked an inline comment as done.Feb 11 2020, 3:07 PM
SouraVX added inline comments.
llvm/tools/llvm-dwp/llvm-dwp.cpp
161–164

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.

SouraVX marked 3 inline comments as done.Feb 11 2020, 3:09 PM
dblaikie added inline comments.Feb 18 2020, 1:58 PM
llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s
5

missing space between the unexpected unit type and the word 'found'.

(do other error messages use quotation marks around integer constants like this? I'd guess not & would recommend following that convention here - but could you check/verify that?)

llvm/tools/llvm-dwp/llvm-dwp.cpp
161–164

Ah, I see - looks like this is ignoring the abbrev offset, assuming it will be zero which is sort of OK, but sort of not. Then it's reading the address size which it's not using (surprising there's no unused variable warning)

It;s getting used while skipping the DIE. Line:201. + I think, that has to be read anyways, to advance the offset/cursor by that amount for reading further data.

Then it seems important that this gets parsed correctly in v4 and v5 when the fields change location? Otherwise the address size that will be used would actually be the DWARF version number instead?

Please add a FIXME around the AddrSize and AbbrevOffset parsing logic to fix it to parse these correctly & possibly respect the abbrev offset, or at least error-check that it's always zero.

Can you please, hint/describe a scenario, when AbbrevOffset different than zero in a DWO object.

The DWARF spec is generally fairly permissive - so "if it doesn't disallow it, it's probably/kind-of allowed". In this case there's no reason a DWARF producer couldn't hide an extra payload in debug_abbrev before the actual debug abbrev contribution, for instance.

At some point we might eventually want to figure out what/if it looks like to support LTO and Split DWARF which might involve multiple CUs in a single .dwo file, in which case then it could involve multiple debug_abbrev contributions and non-zero abbrev offsets.

SouraVX updated this revision to Diff 245281.Feb 18 2020, 2:50 PM

nit: test case revised.

SouraVX marked 2 inline comments as done.Feb 18 2020, 2:57 PM
SouraVX added inline comments.
llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s
5

Their is as such precedent, although removed Single Quotes. One thing I noticed considering other error handling in DWP, We don't emit forms, simple fire an error

Line:170
eturn make_error<DWPError>("top level DIE is not a compile unit");

dblaikie accepted this revision.Feb 18 2020, 2:59 PM
dblaikie added inline comments.
llvm/test/tools/llvm-dwp/X86/wrong-unit-type-info-v5.s
5

Yeah, I think that's an example of an error message that could be improved but hasn't been as yet.

Please remove the exclamation mark from the error message.

(hmm, it looks like the test has been updated but this test would currently fail because it looks like the implementation still has the same output format/bugs)

This revision is now accepted and ready to land.Feb 18 2020, 2:59 PM
This revision was automatically updated to reflect the committed changes.
SouraVX marked an inline comment as done.
fhahn added a subscriber: fhahn.Feb 19 2020, 1:47 AM

It looks like the tests added in this patch are failing on macOS, because they require ELF object files. I've pushed rGc41a1f63b3c0 which adds a Linux triple to the tests. That should fix the failures on macOS