[PATCH] D84911: [DWARFYAML] Make the 'Length' field of the address range table optional.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:149
+
+    if (Range.Length)
+      Length = *Range.Length;
----------------
Higuoxing wrote:
> MaskRay wrote:
> > You can move Length assignment above to the else branch.
> 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.
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.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:147
+    Length += PaddedHeaderLength - HeaderLength;
+    Length += AddrSize * 2 * (Range.Descriptors.size() + 1);
+
----------------
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.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:514-515
+
+## k) Test that yaml2obj is able to determine the correct length for the address range table
+## if the 'Length' field isn't specified.
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84911/new/

https://reviews.llvm.org/D84911