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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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.
In principle, this approach looks fine, but please do as @labath suggests to reduce the amount of changes in one go.
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. |
Address comments.
llvm/test/tools/yaml2obj/ELF/DWARF/debug-info.yaml | ||
---|---|---|
743 ↗ | (On Diff #279756) | Oops, sorry, I forget to modify this line. |
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. |
Address comments. Thanks for reviewing!
llvm/lib/ObjectYAML/DWARFEmitter.cpp | ||
---|---|---|
206–215 | Thanks! I'll do it in a separate patch. |
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. |
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. |
clang-tidy: error: 'lldb/Expression/DWARFExpression.h' file not found [clang-diagnostic-error]
not useful