[PATCH] D80972: [ObjectYAML][DWARF] Support emitting the .debug_aranges section in ELFYAML.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:99-100
     writeInteger((uint16_t)Range.Version, OS, DI.IsLittleEndian);
-    writeInteger((uint32_t)Range.CuOffset, OS, DI.IsLittleEndian);
+    if (Range.Length.isDWARF64())
+      writeInteger((uint64_t)Range.CuOffset, OS, DI.IsLittleEndian);
+    else
----------------
Perhaps you should move this fix into a follow-up change, and wait to add DWARF64 testing to that same change. It looks good, but I think we want to avoid confusing the two issues.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:108
+    auto FirstDescriptor =
+        alignTo(HeaderSize, Range.SegSize + Range.AddrSize * 2);
     ZeroFillBytes(OS, FirstDescriptor - HeaderSize);
----------------
Similar to above, you should probably delay segment size support for a follow-up change.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:1657-1659
+  if (Object.DWARF)
+    Object.DWARF->IsLittleEndian =
+        Object.Header.Data == ELFYAML::ELF_ELFDATA(ELF::ELFDATA2LSB);
----------------
This seems somewhat unrelated too? Perhaps defer to another patch, rather than trying to adopt it all at once.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:5
+
+## Generate and verify the little endian object file.
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
----------------
the -> a


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:7
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
+# RUN: llvm-readobj -S --section-data %t1.o | \
+# RUN:   FileCheck %s -DADDRALIGN="1" --check-prefixes=DWARF-LE-DEFAULT,DWARF-LE-CONTENT
----------------
Nit: -S -> --sections (since you are using a long option already).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:8
+# RUN: llvm-readobj -S --section-data %t1.o | \
+# RUN:   FileCheck %s -DADDRALIGN="1" --check-prefixes=DWARF-LE-DEFAULT,DWARF-LE-CONTENT
+
----------------
Can you get away with just one check-prefix here? It looks like they're both used together always.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:75
+      CuOffset: 0
+      AddrSize: 0x08
+      SegSize:  0x00
----------------
I'd make one of these 4 byte size and the other 8 (perhaps deliberately do 8 bytes for the DWARF32 case, to deliberately show the address size and DWARF32/64 bits are unrelated). Since I'm suggesting you wait for DWARF64 support to a separate change, make them both DWARF32 temporarily, and update the test case when you add DWARF64 support.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:81
+    - Length:
+        TotalLength:   0xffffffff
+        TotalLength64: 0x34
----------------
Not part of this change, but I think this should be changed in the future to be an explicit `Format: DWARF64` field, and just leave `TotalLength` to be the only length field (i.e. don't have a separate `Length` and `TotalLength` fields).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:91
+
+## Generate and verify the big endian object file.
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
----------------
the -> and


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:92
+## Generate and verify the big endian object file.
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: llvm-readobj -S --section-data %t2.o | FileCheck %s --check-prefix=DWARF-BE-DEFAULT
----------------
In your follow-up change to add support for big endian, you can avoid the need for two separate YAML blobs, by using yaml2obj's -D option. It works similary to FileCheck's:

```
# RUN: yaml2obj %s -D ENDIAN=ELFDATA2LSB -o %tle.o
...
# RUN: yaml2obj %s -D ENDIAN=ELFDATA2MSB -o %tbe.o
...

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    [[ENDIAN]]
...
```


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:178
+
+## b) Generate the .debug_aranges section from the raw section content.
+
----------------
Delete "the" from "the raw section"


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-aranges.yaml:216
+## d) Test that yaml2obj emits an error message when both the "Size" and the
+## "debug_arange" entry are specified at the same time.
+
----------------
debug_arange -> debug_aranges


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80972