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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:73
 
+static void writeInitialLength(const dwarf::DwarfFormat Format,
+                               const uint64_t Length, raw_ostream &OS,
----------------
I'd replace `Format` with `Is64Bit` and put it after `IsLittleEndian`. 


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:81
+    writeInteger((uint32_t)Length, OS, IsLittleEndian);
+}
+
----------------
It can be

```
if (Is64Bit)
  writeInteger((uint32_t)UINT32_MAX, OS, IsLittleEndian);
writeVariableSizedInteger(Length, Is64Bit ? 8 : 4, OS, IsLittleEndian);
```


================
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 =
----------------
Please do not use `auto` when the type us not obvious.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:355
+                              /*2-byte (version) + 1-byte (address_size) +
+                                 1-byte (segment_selector_size) = 4-byte*/
+            4 + (AddrTableEntry.AddrSize + AddrTableEntry.SegSelectorSize) *
----------------
I'd simplify:

```
(uint64_t)*AddrTableEntry.Length : /*2 (version) + 1 (address size) + 1 (segment selector size) =*/ 4 + 
```


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:364
+
+    for (auto Pair : AddrTableEntry.SegAddrPairs) {
+      if (AddrTableEntry.SegSelectorSize != 0)
----------------
no `auto` please


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:205
+    IO &IO, DWARFYAML::AddrTableEntry &AddrTable) {
+  IO.mapOptional("Format", AddrTable.Format, dwarf::DWARF32);
+  IO.mapOptional("Length", AddrTable.Length);
----------------
Hmmm. I guess `DWARF64` would be more common?


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:207
+  IO.mapOptional("Length", AddrTable.Length);
+  IO.mapOptional("Version", AddrTable.Version, 5);
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
----------------
Should it be required? I guess we will be unable to change this value in the future here, because otherwise
our tests that do not specify it explicitly might start to fail. So I am not sure that we want a default value of 5 (or any default value).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81541