Page MenuHomePhabricator

[DebugInfo]: Added preliminary support for DWARFv5 in llvm-dwp utility.
AbandonedPublic

Authored by SouraVX on Feb 10 2020, 5:49 AM.

Details

Summary

Summary: This patch adds preliminary support in llvm-dwp to package dwarfv5 DWO objects. Their are numerous updates in section contents in dwarfv5. This patch teaches llvm-dwp to parse revised debug_info and debug_str_offsets section based on the version information present in CU header.

Diff Detail

Event Timeline

SouraVX created this revision.Feb 10 2020, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 5:49 AM
aprantl added inline comments.Feb 10 2020, 1:26 PM
llvm/tools/llvm-dwp/llvm-dwp.cpp
94

nit: this should be

// Emit the version.
Out.EmitIntValue(5, 2); 
// Emit two bytes padding.
Out.EmitIntValue(0, 2);
591

//

592

on *the* DWARF version

SouraVX updated this revision to Diff 243756.Feb 11 2020, 1:01 AM

Thank You @aprantl
Incorporated some nits comments by @aprantl

SouraVX updated this revision to Diff 243763.Feb 11 2020, 1:30 AM

Please split this into smaller/more incremental patches - it should be possible to implement the new support for parsing DWARFv5 unit headers, signature, etc, without the new string header support. (you'd have to modify/hand-craft the dwo file assembly though, since LLVM's generated DWARFv5 will always use FORM_strx - but if you change the dwo to use FORM_str (direct strings, no indirection) it should mean you can make a small but valid test case without the need for strx support)

llvm/test/tools/llvm-dwp/X86/simple-v5.test
1

While previous testing has used checked in binary object files (& I wrote those tests... ) - it seems a lot of testing even of tools like this has moved ot checking in assembly files and assuming/relying on llvm-mc as an assembler to make tests more legible.

Could you do that in this case? Use/add assembly rather than binary files, and have the test assemble those files then run them through llvm-dwp?

llvm/tools/llvm-dwp/llvm-dwp.cpp
593–600

This is a rather strange construct - a named local lambda that's called only once. It doesn't seem appropriate.

If you especially want to reduce the scope of the extra variables, some options might include:

auto DwarfVersion;
{
  DataExtractor InfoData(InfoSection, true, 0);
  uint64_t Offset = 4;
  DwarfVersion = InfoData.getU16(&Offset);
}

This is usually only done if there's repeated use of similarly named variables, a large scope where these variables might get confused with others, etc. Which doesn't seem to be the case here.

or

auto DwarfVersion = [] {
  DataExtractor InfoData(InfoSection, true, 0);
  uint64_t Offset = 4;
  return InfoData.getU16(&Offset);
}();

The latter is usually only for when the variable being initialized is 'const'.

I think it's probably best to just write it more directly:

DataExtractor InfoData(InfoSection, true, 0);
uint64_t InfoVersionOffset = 4;
writeStringsAndOffsets(..., InfoData.getU16(&InfoVersionOffset));

Please split this into smaller/more incremental patches - it should be possible to implement the new support for parsing DWARFv5 unit headers, signature, etc, without the new string header support. (you'd have to modify/hand-craft the dwo file assembly though, since LLVM's generated DWARFv5 will always use FORM_strx - but if you change the dwo to use FORM_str (direct strings, no indirection) it should mean you can make a small but valid test case without the need for strx support)

Hi Thanks for review!
Sounds good to me, will segregate this in 2 patches.

  1. With DWARFv5 Info section parsing support.
  2. With DWARFv String Offset parsing support.

Regarding, testing I'm considering adding the test cases to be written and tested with llvm-mc in this directory llvm/test/tools/llvm-dwp/ . Didn't notice any debug-info related test case in llvm/test/MC/X86 directory. Do you have any other suitable place in mind, Please share.

Please split this into smaller/more incremental patches - it should be possible to implement the new support for parsing DWARFv5 unit headers, signature, etc, without the new string header support. (you'd have to modify/hand-craft the dwo file assembly though, since LLVM's generated DWARFv5 will always use FORM_strx - but if you change the dwo to use FORM_str (direct strings, no indirection) it should mean you can make a small but valid test case without the need for strx support)

Hi Thanks for review!
Sounds good to me, will segregate this in 2 patches.

  1. With DWARFv5 Info section parsing support.
  2. With DWARFv String Offset parsing support.

Regarding, testing I'm considering adding the test cases to be written and tested with llvm-mc in this directory llvm/test/tools/llvm-dwp/ . Didn't notice any debug-info related test case in llvm/test/MC/X86 directory. Do you have any other suitable place in mind, Please share.

Yes, llvm/test/tools/llvm-dwp would be the right place for the test (see similar tests using llvm-mc in llvm/test/tools/dsymutil or llvm/test/tools/llvm-dwarfdump etc)

SouraVX abandoned this revision.Mar 7 2020, 10:37 AM