[PATCH] D83853: [DWARFYAML] Implement the .debug_str_offsets section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:434
+    for (uint64_t Offset : Table.Offsets) {
+      cantFail(writeVariableSizedInteger(Offset,
+                                         Table.Format == dwarf::DWARF64 ? 8 : 4,
----------------
A thought I've just had: what do you think should happen if a user specifies a 64-bit-using value for DWARF32? This may apply in one or two other places too.

I don't have any particular opinions here. Just wondering if you've considered it at all.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:46-47
+    - Offsets:
+        - 0x1234
+        - 0x5678
+    - Format: DWARF64
----------------
I'd explicitly use the full 4 bytes here (and 8 bytes in the DWARF64 version).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:98
+      Padding: 0x12
+      Offsets: []
+
----------------
Perhaps omit `Offsets` entirely here.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-str-offsets.yaml:120
+  debug_str_offsets:
+    - Offsets: []
+
----------------
Perhaps it would be worth a test case for there being no tables at all, i.e:
```
DWARF:
  debug_str_offsets: []
```

I'd expect an empty section to be emitted in this case.

This might also be a missing test case in some (most?) of the other sections.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83853