[PATCH] D86590: [DWARFYAML] Make the unit_length and header_length fields optional.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:508
+
 Error DWARFYAML::emitDebugLine(raw_ostream &OS, const DWARFYAML::Data &DI) {
   for (const auto &LineTable : DI.DebugLines) {
----------------
Could you add a TODO somewhere in this function saying that this doesn't support DWARF v5 yet, please?


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:511
+    // Buffer holds the bytes following the header_length (or prologue_length in
+    // DWARFv2) field to the end of line number program itself.
+    std::string Buffer;
----------------



================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:524
 
     for (auto OpcodeLength : LineTable.StandardOpcodeLengths)
+      writeInteger((uint8_t)OpcodeLength, BufferOS, DI.IsLittleEndian);
----------------
I'd get rid of the `auto` here and below in the for loops. I think it's quite important we know what type they are because of how we write things.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml:483
+
+## j) Test that yaml2obj is able to infer the length and header_length fields.
+
----------------
It may be worth having a DWARF64 version of this case too (i.e. where the format is explicitly specified as such). You could add it as a separate subsequent table to this same case, I think (which also then shows that the lengths are calculated independently of one another).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml:489
+#      INFER-LENGTH: Hex dump of section '.debug_line':
+# INFER-LENGTH-NEXT: 0x00000000 39000000 04001b00 00000101 01fb0e0d 9...............
+##                              ^-------                            unit_length (4-byte) 0x39
----------------
The unit_length field looks incorrect. It is currently apparently 0x39, yet the end of the things you check is at offset 0x28, so I'd expect a value of 0x24. The header_length looks right though.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml:518
+  Type:  ET_EXEC
+DWARF:
+DWARF:
----------------
Nit: delete the extra `DWARF:`


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml:528
+      OpcodeBase:            13
+      StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
+      IncludeDirs:           []
----------------
I would suggest a non-standard number of lengths, to show that the header length isn't hard-coded.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-line.yaml:529
+      StandardOpcodeLengths: [ 0, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0, 1 ]
+      IncludeDirs:           []
+      Files:
----------------
Similarly, I'd add an include directory here, to show that the length properly includes this field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86590