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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:2011
+
+  llvm_unreachable("Didn't find DW_AT_addr_base in cloned DIE!");
+}
----------------
avl wrote:
> 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.
> 
> 
>My suggestion is to - not create address attributes if input DW_AT_addr_base is invalid.

one thing to clarify - if all address attributes are skipped because DW_AT_addr_base is missed then we do not need to generate .debug_addr and then it is not a problem that DW_AT_addr_base is missed.


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

https://reviews.llvm.org/D154638