This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML] Refactor emitDebugInfo() to make the length be inferred.
ClosedPublic

Authored by Higuoxing on Jul 17 2020, 2:34 AM.

Details

Summary

This patch refactors emitDebugInfo() to make the length field be
inferred from its content. Besides, the Visitor class is removed in
this patch. The original Visitor class helps us determine an
appropriate length and emit the .debug_info section. These two
processes can be merged into one process. Besides, the length field
should be inferred when it's missing rather than when it's zero.

Diff Detail

Event Timeline

Higuoxing created this revision.Jul 17 2020, 2:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 17 2020, 2:34 AM

I'll add some tests later.

Higuoxing retitled this revision from [DWARFYAML] Refactor emitDebugInfo() to make the length be inferred. to [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred..Jul 17 2020, 2:36 AM

The number of changed tests is large. Is it worth moving the IO.mapOptional("Length", Unit.Length); change to a separate patch to make the refactoring more focused? Thanks

The number of changed tests is large. Is it worth moving the IO.mapOptional("Length", Unit.Length); change to a separate patch to make the refactoring more focused? Thanks

This patch is intended to make the length field be inferred when emitting the .debug_info section. If we move the IO.mapOption("Length", Unit.Length); change to a separate change, we might not be able to know when to infer the length? There are two visitors, DumpVisitor which is used to emit the .debug_info section and DIEFixupVisitor which is used to calculate the length field for us. Do you mean that we keep the DIEFixupVisitor class and remove the DumpVisitor class in this patch?

labath added a subscriber: labath.Jul 20 2020, 2:17 AM

The number of changed tests is large. Is it worth moving the IO.mapOptional("Length", Unit.Length); change to a separate patch to make the refactoring more focused? Thanks

This patch is intended to make the length field be inferred when emitting the .debug_info section. If we move the IO.mapOption("Length", Unit.Length); change to a separate change, we might not be able to know when to infer the length? There are two visitors, DumpVisitor which is used to emit the .debug_info section and DIEFixupVisitor which is used to calculate the length field for us. Do you mean that we keep the DIEFixupVisitor class and remove the DumpVisitor class in this patch?

I think that should work if you make it so that this other patch comes before the functional change in this patch. That other patch could change the encoding to hex (uint64_t Length; -> yaml::Hex64 Length;) and make it default to zero (IO.mapRequired("Length", Unit.Length); -> IO.mapOptional("Length", Unit.Length, 0);). That should have no functional change (I think), but allow you to make the changes in all the yaml files. The visitation stuff could come after that.

The number of changed tests is large. Is it worth moving the IO.mapOptional("Length", Unit.Length); change to a separate patch to make the refactoring more focused? Thanks

This patch is intended to make the length field be inferred when emitting the .debug_info section. If we move the IO.mapOption("Length", Unit.Length); change to a separate change, we might not be able to know when to infer the length? There are two visitors, DumpVisitor which is used to emit the .debug_info section and DIEFixupVisitor which is used to calculate the length field for us. Do you mean that we keep the DIEFixupVisitor class and remove the DumpVisitor class in this patch?

I think that should work if you make it so that this other patch comes before the functional change in this patch. That other patch could change the encoding to hex (uint64_t Length; -> yaml::Hex64 Length;) and make it default to zero (IO.mapRequired("Length", Unit.Length); -> IO.mapOptional("Length", Unit.Length, 0);). That should have no functional change (I think), but allow you to make the changes in all the yaml files. The visitation stuff could come after that.

Thank you @labath, It sounds good to me!

In principle, this approach looks fine, but please do as @labath suggests to reduce the amount of changes in one go.

Higuoxing updated this revision to Diff 279756.Jul 22 2020, 3:42 AM

Address comments & Rebase & Add one test.

Higuoxing retitled this revision from [DWARFYAML][WIP] Refactor emitDebugInfo() to make the length be inferred. to [DWARFYAML] Refactor emitDebugInfo() to make the length be inferred..Jul 22 2020, 3:43 AM
Higuoxing added a reviewer: labath.
jhenderson added inline comments.Jul 22 2020, 5:41 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
743 ↗(On Diff #279756)

Should this be INFER-LENGTH? (The test is currently failing...)

748–750 ↗(On Diff #279756)

I don't think these comments are particularly helpful, since they can be inferred from the byte-count/offset at the start of the line. I think if you want comments, you should say what the bytes represent, like you do elsewhere.

Higuoxing updated this revision to Diff 280020.Jul 22 2020, 8:28 PM
Higuoxing marked 3 inline comments as done.

Address comments.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml
743 ↗(On Diff #279756)

Oops, sorry, I forget to modify this line.

jhenderson accepted this revision.Jul 23 2020, 1:48 AM

LGTM, thanks, but might be best to get a second opinion.

This revision is now accepted and ready to land.Jul 23 2020, 1:48 AM
labath accepted this revision.Jul 23 2020, 2:18 AM

Yeah, looks good. Some small suggestions inline, though some of them may be better off as separate patches...

llvm/lib/ObjectYAML/DWARFEmitter.cpp
206–215

Maybe this is for a separate patch, since you're just moving these, but this code already exists in BinaryFormat/Dwarf.h. It would be nice if DWARFYaml::Unit had a dwarf::FormParams field or a getter, and then these things could be retrieved by querying that object...

348–352

Since the comments repeat the entire expression, it would be less repetitive to just structure the code in a way that comments can be attached to the actual expressions. Maybe something like:

Lenght = 3; // version + address_size
Length += (Unit.Version >= 5 ? 1 : 0); // unit_type
Length += Unit.getFormParams().getDwarfOffsetByteSize(); // abbrev_offset
379–381

a writeDwarfOffset function might be handy.

Higuoxing updated this revision to Diff 280116.Jul 23 2020, 7:42 AM
Higuoxing marked 3 inline comments as done.

Address comments. Thanks for reviewing!

llvm/lib/ObjectYAML/DWARFEmitter.cpp
206–215

Thanks! I'll do it in a separate patch.

jhenderson added inline comments.Jul 23 2020, 7:45 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
379–381

I'm guessing the new function can be used elsewhere too (I expect in places like .debug_aranges for example). In another patch, please consider looking at that.

Higuoxing marked an inline comment as done.Jul 23 2020, 7:49 AM
Higuoxing added inline comments.
llvm/lib/ObjectYAML/DWARFEmitter.cpp
379–381

Yeah, it can be used in the .debug_str_offsets/debug_rnglists/debug_loclists/debug_aranges sections, etc. I've applied it in this patch and will apply it to other sections in a follow-up patch.

This revision was automatically updated to reflect the committed changes.
llvm/lib/ObjectYAML/DWARFEmitter.cpp