[PATCH] D83624: [DWARFYAML] Implement the .debug_rnglists section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/ObjectYAML/DWARFYAML.h:189
+
+struct RnglistsEntry {
+  dwarf::DwarfFormat Format;
----------------
Maybe call this `RnglistTable` to reduce the potential for confusion.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:502
+    else
+      AddrSize = DI.Is64bit ? 8 : 4;
+
----------------
I keep finding the `Is64Bit` variable name confusing with the DWARF format. Could you rename it in a separate change to something like `Is64BitAddrSize` or something similar, please?


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:507-508
+    // length and then serialize the buffer content to the actual output stream.
+    std::string Buffer;
+    raw_string_ostream BufferOS(Buffer);
+
----------------
Maybe these could be named to indicate these contain just the lists. Perhaps `ListBuffer` and `ListBufferOS`?


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:515
+
+    for (const DWARFYAML::Rnglist &Rnglist : RnglistsEntry.Lists) {
+      Offsets.push_back(BufferOS.tell());
----------------
Perhaps use `List` for the variable name to avoid confusion with the type.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:517
+      Offsets.push_back(BufferOS.tell());
+      for (const DWARFYAML::RnglistEntry &RnglistEntry : Rnglist.Entries) {
+        Expected<uint64_t> EntrySize = writeRnglistEntry(
----------------
Similarly, consider `Entry` for the name.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:524-525
+      }
+      // Write DW_RLE_end_of_list to terminate this entry.
+      BufferOS.write_zeros(1);
+      Length += 1;
----------------
It would probably be better to be more explicit about writing DW_RLE_end_of_list, e.g. `writeInteger(DW_RLE_end_of_list, ...)`.

It may make more sense to get users to explicitly specify the end-of-list marker. This would be in keeping with how `DT_NULL` works for SHT_DYNAMIC ELF sections. This means that users can deliberately write lists without trailing terminators.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:530-534
+    if (RnglistsEntry.OffsetEntryCount)
+      OffsetEntryCount = *RnglistsEntry.OffsetEntryCount;
+    else
+      OffsetEntryCount = RnglistsEntry.Offsets ? RnglistsEntry.Offsets->size()
+                                               : Offsets.size();
----------------
A comment explaining this order of precedence might help make things clearer.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:539-540
+
+    // If the length is specified in the YAML description, we use it as the
+    // actual length.
+    if (RnglistsEntry.Length)
----------------
Maybe "... we use it instead of the actual length" might be clearer/more correct.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:550
+
+    auto EmitOffsets = [&](const std::vector<uint64_t> &Offsets,
+                           uint64_t OffsetsSize) {
----------------
`ArrayRef<uint64_t>` here.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:242
+  IO.mapOptional("Length", RnglistsEntry.Length);
+  IO.mapRequired("Version", RnglistsEntry.Version);
+  IO.mapOptional("AddressSize", RnglistsEntry.AddrSize);
----------------
`Version` should be optional, and have a value of 5 by default, since it's the only supported version of the format currently.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:1
+## Test that yaml2obj emits .debug_rnglists section.
+
----------------
I'm out of time to review these now, but I'll try to look at them tomorrow, or later on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83624