[PATCH] D154638: Emit a .debug_addr section with dsymutil

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DWARFLinker/DWARFLinker.h:701-702
 
+    /// Emit the .debug_addr section for the \p Unit. Use \p Addrs as the list
+    /// of addresses to be emitted in the section.
+    void emitDebugAddrSection(CompileUnit &Unit,
----------------



================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2011
+
+  llvm_unreachable("Didn't find DW_AT_addr_base in cloned DIE!");
+}
----------------
rastogishubham wrote:
> avl wrote:
> > rastogishubham wrote:
> > > JDevlieghere wrote:
> > > > Similar question as for the assert: is this checking internal consistency (i.e. that we have created a `DW_AT_addr_base` in the cloned DIE) or can this trip for invalid input?
> > > This one can trip for invalid input, if the input dwarf doesn't have an addr base, then the cloned die would not either.
> > This is also should be check for internal consistency: We should not miss DW_AT_addr_base when we generate .debug_addr. The patchAddrBase is called from DIECloner::emitDebugAddrSection, thus we check for DW_AT_addr_base exactly when we are going to generate .debug_addr. The only problem with current implementation is this check will fail when original DWARF does not have .debug_addr but we call emitDebugAddrSection for generation of empty .debug_addr. The check for empty addresses set needs to be added into emitDebugAddrSection.
> > 
> > For the case of invalid DWARF(DW_AT_addr_base missed in input file) - we should not generate any addresses as we could not correctly read original addresses. DWARFLinker should report warning here and do not generate any address:
> > 
> > DIECloner::cloneAddressAttribute
> > ```
> >   std::optional<uint64_t> Addr = AddrAttribute->getAsAddress();
> >   if (!Addr) {
> >     Linker.reportWarning("Cann't read address attribute value.", ObjFile);
> >     Addr = 0;
> >   }
> > 
> > ```
> > 
> > Though, this place looks a bit incorrect. Instead of using zero Addr, the attribute should probably be skipped.
> DW_AT_addr_base is of form DW_FORM_sec_offset, so it will call `DWARFLinker::DIECloner::cloneScalarAttribute(`, are you suggesting that I add a warning there if there is no addr_base found?
No, I do not suggest to add warning into DWARFLinker::DIECloner::cloneScalarAttribute which would report if DW_AT_addr_base is invalid.

My suggestion is to - not create address attributes if input DW_AT_addr_base is invalid. If DW_AT_addr_base is invalid then it is not possible to read input addresses correctly. Instead of using zero or dummy value for the address attribute, it would be better to skip it .For that purpose we have AddrAttribute->getAsAddress() to be optional. If the DW_AT_addr_base is invalid it should return std::nullopt. So we can skip address attribute and report a warning inside DIECloner::cloneAddressAttribute().

Another thing is that it would be better to not report_fatal_error here. This is the place where an output DWARF is emitted. It looks too late to validate and report errors for input DWARF. At this point we need to have llvm_unreachable() indicating that generated DWARF is inconsistent.

In short:

1. I think we need to change(with separate patch) the code in DIECloner::cloneAddressAttribute into this:

```
std::optional<uint64_t> Addr = AddrAttribute->getAsAddress();
if (!Addr) {
  Linker.reportWarning("Cann't read address attribute value.", ObjFile);
  return 0;
} 
```
 
  - 2. use llvm_unreachable() here to indicate that required DW_AT_addr_base is not generated.




================
Comment at: llvm/test/tools/dsymutil/ARM/dwarf5-addr_base.test:49
+RUN: rm -rf %t.dir && mkdir -p %t.dir
+RUN: dsymutil -y %p/dummy-debug-map-amr64.map -oso-prepend-path=%p/../Inputs/DWARF5-addr_base -o %t.dir/dwarf5-addr_base.dSYM
+RUN: llvm-dwarfdump %t.dir/dwarf5-addr_base.dSYM -a --verbose | FileCheck %s
----------------
rastogishubham wrote:
> avl wrote:
> > add a check for "-u" case, please.
> I am not sure what should happen in a -u case
> 
> Should the DW_AT_addr_base be patched in a -u case? It doesn't get patched now
For -u case we need to check that generated DWARF is exactly the same as input DWARF. That DW_AT_addr_base is presented with correct value, that indexed addresses are used, that .debug_addr contains proper values.


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

https://reviews.llvm.org/D154638