[PATCH] D81541: [ObjectYAML][DWARF] Implement the .debug_addr section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:66-71
 static void writeInitialLength(const DWARFYAML::InitialLength &Length,
                                raw_ostream &OS, bool IsLittleEndian) {
   writeInteger((uint32_t)Length.TotalLength, OS, IsLittleEndian);
   if (Length.isDWARF64())
     writeInteger((uint64_t)Length.TotalLength64, OS, IsLittleEndian);
 }
----------------
If this function isn't dead already, we should probably aim to delete it with other changes.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:77
+  if (Format == dwarf::DWARF64) {
+    writeInteger((uint32_t)UINT32_MAX, OS, IsLittleEndian);
+    writeInteger((uint64_t)Length, OS, IsLittleEndian);
----------------
`(uint32_t)UINT32_MAX` -> `std::numeric_limits<uint32_t>::max()`


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:351
+Error DWARFYAML::emitDebugAddr(raw_ostream &OS, const Data &DI) {
+  for (auto AddrTableEntry : DI.DebugAddr) {
+    uint64_t Length =
----------------
I'd avoid this `auto`. It's not obvious what the type of `DI.DebugAddr` is. Same below.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:363
+                 DI.IsLittleEndian);
+
+    for (auto Pair : AddrTableEntry.SegAddrPairs) {
----------------
`writeVariableSizedInteger` emits an assertion if you are writing values of size other than 1/2/4/8. You'll need to add an error check here for other sizes, or add some way of allowing writing of other sized integers. If you add an error check, you probably only want to trigger the check when there are one or more entries in the table, so that you can still have an invalid size without error if the table is empty.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:209
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
+  IO.mapRequired("SegmentSelectorSize", AddrTable.SegSelectorSize);
+  IO.mapOptional("Entries", AddrTable.SegAddrPairs);
----------------
I would make `SegmentSelectorSize` optional, with a default case of 0. That is by far and away the most common case.

You could probably make `AddrSize` optional too, by referring to the ELF class (i.e. 64/32) if it is unspecified.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:1
+## Test that yaml2obj emits .debug_addr section.
+
----------------
I'm out of time today, so will do a proper review of the tests either later or tomorrow.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:155
+  debug_addr:
+    ## a1) Only address (8-byte) exists.
+    - Format:              [[FORMAT]]
----------------
You can probably get rid of the `a1)` etc bits. from these comments. If you want them for cross-referencing to the output check above, I'd just make them 1), 2) etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81541