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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Couple of other test case suggestions:

1. What is the value of the AddrSize field by default, if using a 32-bit object.
2. An empty list is allowed for a table.



================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:242
+  IO.mapOptional("Length", RnglistsEntry.Length);
+  IO.mapRequired("Version", RnglistsEntry.Version);
+  IO.mapOptional("AddressSize", RnglistsEntry.AddrSize);
----------------
Higuoxing wrote:
> jhenderson wrote:
> > `Version` should be optional, and have a value of 5 by default, since it's the only supported version of the format currently.
> The version field of the .debug_addr section is required (https://reviews.llvm.org/D81541#inline-750289). Shall we make that one optional as well? Or is there any particular reason that makes these two sections different?
I don't think there's anything special about .debug_addr (it's also new in v5), so let's make that optional too (the more optional stuff the better, as long as it isn't likely to cause confusion).


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:1
+## Test that yaml2obj emits .debug_rnglists section.
+
----------------
emits a .debug_rnglists section when requested.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:205-216
+        - Entries:
+            - Operator: DW_RLE_base_addressx
+              Values:   [ 0x1234 ]
+            - Operator: DW_RLE_startx_endx
+              Values:   [ 0x1234, 0x1234 ]
+            - Operator: DW_RLE_end_of_list
+        - Entries:
----------------
Is there a motivation for having so many operators in the DWARF64 case? Could you simplify it down so that there's only end-of-list markers?


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:274
+
+## f) Test that the offset_entry_count field can be specified manually.
+
----------------
Rather than have the fields tested in independent inputs, it's probably safe to test these all in one input which overwrites each of the fields.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:327
+    - Version: 5
+      Offsets: [ 0x12, 0x34 ]
+      Lists:
----------------
I'd specify a different number of offsets (perhaps fewer) than the actual number of lists.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:417-418
+
+## j) Test that yaml2obj emits an error message if we try to assign an invalid value to
+## 'AddressSize' when there is an entry whose operands contain address.
+
----------------
You also need a test showing that an invalid size can be used when there are no operands with addresses.

You might also want multiple copies of case j), testing each of the different address-using operators individually.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-rnglists.yaml:441-442
+
+## k) Test that yaml2obj emits an error message if we specify invalid numbers of operands
+## to operator.
+
----------------
You need to test this for every operator, I think.

The easiest way to do that would be to use -D for the operator kind and values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83624