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

[Xing GUO via Phabricator via llvm-commits]

Higuoxing added inline comments.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:66-71
 static void writeInitialLength(const DWARFYAML::InitialLength &Length,
                                raw_ostream &OS, bool IsLittleEndian) {
   writeInteger((uint32_t)Length.TotalLength, OS, IsLittleEndian);
   if (Length.isDWARF64())
     writeInteger((uint64_t)Length.TotalLength64, OS, IsLittleEndian);
 }
----------------
jhenderson wrote:
> If this function isn't dead already, we should probably aim to delete it with other changes.
Yes, it isn't dead already. I will propose to delete it in another patch.


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:363
+                 DI.IsLittleEndian);
+
+    for (auto Pair : AddrTableEntry.SegAddrPairs) {
----------------
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?


================
Comment at: llvm/lib/ObjectYAML/DWARFEmitter.cpp:73
 
+static void writeInitialLength(const dwarf::DwarfFormat Format,
+                               const uint64_t Length, raw_ostream &OS,
----------------
jhenderson wrote:
> grimar wrote:
> > I'd replace `Format` with `Is64Bit` and put it after `IsLittleEndian`. 
> Personally, I prefer explicitly using the `Format` here, if I'm honest. I think it's clearer than a boolean.
Actually I want a variable like `Is64bitTarget` or `Is64bitAddress` in the `DWARFYAML::Data` structure like `IsLittleEndian`. With this, yaml2elf will be able to infer the `AddrSize` and `SegmentSelectorSize` below. 


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:207
+  IO.mapOptional("Length", AddrTable.Length);
+  IO.mapOptional("Version", AddrTable.Version, 5);
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
----------------
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 :-)


================
Comment at: llvm/lib/ObjectYAML/DWARFYAML.cpp:209
+  IO.mapRequired("AddressSize", AddrTable.AddrSize);
+  IO.mapRequired("SegmentSelectorSize", AddrTable.SegSelectorSize);
+  IO.mapOptional("Entries", AddrTable.SegAddrPairs);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81541