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

[Higuoxing]

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.

[Higuoxing]

I'll add some tests later.

[MaskRay]

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

[Higuoxing]

In D84008#2160426, @MaskRay wrote:

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]

In D84008#2161243, @Higuoxing wrote:

In D84008#2160426, @MaskRay wrote:

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.

[Higuoxing]

In D84008#2161461, @labath wrote:

In D84008#2161243, @Higuoxing wrote:

In D84008#2160426, @MaskRay wrote:

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!

[jhenderson]

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

[Higuoxing]

Address comments & Rebase & Add one test.

[jhenderson]

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml

743

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

748–750

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]

Address comments.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml

743

Oops, sorry, I forget to modify this line.

[jhenderson]

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

[labath]

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]

Address comments. Thanks for reviewing!

llvm/lib/ObjectYAML/DWARFEmitter.cpp

206–215

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

[jhenderson]

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]

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.