This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML] Make the unit_length and header_length fields optional.
ClosedPublic

Authored by Higuoxing on Aug 25 2020, 11:57 PM.

Details

Summary

This patch makes the unit_length and header_length fields of line tables
optional. yaml2obj is able to infer them for us.

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 25 2020, 11:57 PM
Higuoxing requested review of this revision.Aug 25 2020, 11:57 PM

The added test case is verified using llvm-dwarfdump.

Fix failed test.

jhenderson added inline comments.Aug 26 2020, 12:43 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
508

Could you add a TODO somewhere in this function saying that this doesn't support DWARF v5 yet, please?

510
517–518

I'd get rid of the auto here and below in the for loops. I think it's quite important we know what type they are because of how we write things.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml
483

It may be worth having a DWARF64 version of this case too (i.e. where the format is explicitly specified as such). You could add it as a separate subsequent table to this same case, I think (which also then shows that the lengths are calculated independently of one another).

489

The unit_length field looks incorrect. It is currently apparently 0x39, yet the end of the things you check is at offset 0x28, so I'd expect a value of 0x24. The header_length looks right though.

489

Never mind you fixed it whilst I was looking.

518

Nit: delete the extra DWARF:

528

I would suggest a non-standard number of lengths, to show that the header length isn't hard-coded.

529

Similarly, I'd add an include directory here, to show that the length properly includes this field.

Higuoxing updated this revision to Diff 287864.Aug 26 2020, 1:36 AM
Higuoxing marked 9 inline comments as done.

Address review comments.

jhenderson accepted this revision.Aug 26 2020, 2:07 AM

LGTM, with nit.

llvm/lib/ObjectYAML/DWARFEmitter.cpp
526

No need for the cast, since OpcodeLength is uint8_t. Does that apply to any of the other writeInteger calls elsewhere in this function too?

This revision is now accepted and ready to land.Aug 26 2020, 2:07 AM
Higuoxing updated this revision to Diff 287893.Aug 26 2020, 3:05 AM
Higuoxing marked an inline comment as done.

Address comments.

Thanks for reviewing!

jhenderson accepted this revision.Aug 26 2020, 3:06 AM