[PATCH] D81541: [ObjectYAML][DWARF] Implement the .debug_addr section.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Looks mostly fine to me. How are you ensuring your section data is actually correct? Are you just verifying it against the specification/using llvm-dwarfdump/something else?



================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:363
+                 DI.IsLittleEndian);
+
+    for (auto Pair : AddrTableEntry.SegAddrPairs) {
----------------
Higuoxing wrote:
> jhenderson wrote:
> > `writeVariableSizedInteger` emits an assertion if you are writing values of size other than 1/2/4/8. You'll need to add an error check here for other sizes, or add some way of allowing writing of other sized integers. If you add an error check, you probably only want to trigger the check when there are one or more entries in the table, so that you can still have an invalid size without error if the table is empty.
> I want to do the error check in `writeVariableSizedInteger` in another patch.
> 
> ```
> Error Err = Error::success();
> cantFail(Err);
> for (const SegAddrPair& Pair : SegAddrPairs) {
>   if (Seg.Size != 0) {
>     Err = writeVariableSizedInteger();
>     if (Err)
>       return std::move(Err);
>   }
>   if (Addr.Size != 0) {
>     Err = writeVariableSizedInteger();
>     if (Err)
>       return std::move(Err);
>   }
> }
> ```
> 
> What do you think of it?
Seems reasonable, and similar to what we did with .debug_ranges Offset before.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:209
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
+  IO.mapRequired("SegmentSelectorSize", AddrTable.SegSelectorSize);
+  IO.mapOptional("Entries", AddrTable.SegAddrPairs);
----------------
Higuoxing wrote:
> jhenderson wrote:
> > I would make `SegmentSelectorSize` optional, with a default case of 0. That is by far and away the most common case.
> > 
> > You could probably make `AddrSize` optional too, by referring to the ELF class (i.e. 64/32) if it is unspecified.
> I will make `SegmentSelectorSize` be 0 by default. As for the `AddrSize`, we have to add another field `Is64bitTarget` in the `DWARFYAML::Data` structure, I will make it optional in a separate patch.
I still feel like there should be a way to use the file header data to infer the address, rather than needing an explicit field, but it's fine to wait to a subsequent patch for that.


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:207
+  IO.mapOptional("Length", AddrTable.Length);
+  IO.mapOptional("Version", AddrTable.Version, 5);
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
----------------
Higuoxing wrote:
> grimar wrote:
> > Should it be required? I guess we will be unable to change this value in the future here, because otherwise
> > our tests that do not specify it explicitly might start to fail. So I am not sure that we want a default value of 5 (or any default value).
> `.debug_addr` is new in DWARFv5, I am not sure if the version will be changed in DWARFv6? So I take your advice and change it to `mapRequired()` here :-)
Yeah, I'm inclined to be cautious on this one. We can discuss it with more developers in the future. The conclusion may be that having to explicitly specify the DWARF version in individual tables is annoying. Perhaps there could be a top-level DefaultVersion entry in the DWARF tag that says what version to assume when generating tables, if otherwise unspecified.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:25
+# DWARF32-LE-NEXT:   0000: 0C000000 05000400 34120000 78560000
+##                         |        |   | |  |        |
+##                         |        |   | |  |        +------- address (4-byte)
----------------
MaskRay wrote:
> The ASCII art appears to be too verbose.
Do you have a better suggestion for how to illustrate the section data without relying on llvm-dwarfdump (which in turn would be relying on yaml2obj in the future, and thus be circular logic)?

This is the same approach as has been taken in the other tests in this directory.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:155
+  debug_addr:
+    ## 1) Only address (8-byte) exists.
+    - Format:              [[FORMAT]]
----------------
8 -> 4


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:234
+    ## 9) Omit the 'Format' field, DWARF32 by default.
+    - Version:             5
+      AddressSize:         0x04
----------------
Up to you, but I think it would make things a little less verbose in this test if you just left these all to be DWARF32, and add additional minimal test cases just showing the different behaviour for different DWARF formats, since the format only impacts the header fields.


================
Comment at: llvm/test/tools/yaml2obj/ELF/DWARF/debug-addr.yaml:243
+      SegmentSelectorSize: 0x00
+      Entries:             []
+
----------------
You need a test case each for missing the version and address size fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81541