This is an archive of the discontinued LLVM Phabricator instance.

[DWARFYAML] Make the 'Length' field of the address range table optional.
ClosedPublic

Authored by Higuoxing on Jul 29 2020, 10:31 PM.

Details

Summary

This patch makes the 'Length' field of the address range table optional.

Diff Detail

Event Timeline

Higuoxing created this revision.Jul 29 2020, 10:31 PM
Higuoxing requested review of this revision.Jul 29 2020, 10:31 PM
MaskRay added inline comments.Jul 29 2020, 10:42 PM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
145

const uint64_t PaddedHeaderLength

149

You can move Length assignment above to the else branch.

Higuoxing marked an inline comment as done.

Address review comments.

Thanks for reviewing!

llvm/lib/ObjectYAML/DWARFEmitter.cpp
145

Done!

149

It's not easy to do that since the PaddedHeaderLength and HeaderLength is used below. If we move the length assignment above to the else branch, we might need to duplicate some logic.

jhenderson added inline comments.Jul 30 2020, 12:54 AM
llvm/lib/ObjectYAML/DWARFEmitter.cpp
147

Shouldn't there be a SegmentSelectorSize calculation here too? .debug_aranges defines how to actually use the property, so we should probably do the right calculation here.

149

I think @MaskRay is suggesting:

if (Range.Length) {
  Length = *Range.Length;
} else {
  Length += PaddedHeaderLength - HeaderLength;
  Length += AddrSize * 2 * (Range.Descriptors.size() + 1);
}

Since this if overrides Length, and there are no side effects to the += calculations before, there's no reason to do them at all in the if case.

llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
514–515

You probably need a variation of this test with a different non-default address size (e.g. 8 for a 32-bit object), to show that the length accounts for the right size. Same applies for the segment selector size.

Higuoxing added inline comments.Jul 30 2020, 1:00 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
514–515

Currently, yaml2obj doesn't support emitting the segment selector for the address range table. Shall I implement it before this patch?

jhenderson added inline comments.Jul 30 2020, 1:02 AM
llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml
514–515

No need to do it before unless you want to (this patch is more useful). You can add it afterwards - just make sure to fix this code path when you do!

Higuoxing updated this revision to Diff 281829.Jul 30 2020, 1:40 AM
Higuoxing marked 2 inline comments as done.

Address comments.

Thanks for reviewing!

This revision is now accepted and ready to land.Jul 30 2020, 1:44 AM