[PATCH] D155666: Do not emit a .debug_addr section if the DW_AT_addr_base is not set.

[Shubham Sandeep Rastogi via Phabricator via llvm-commits]

rastogishubham added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1372
+        !AddrAttribute->getUnit()->getAddrOffsetSectionBase())
+      return 0;
     Addr = 0;
----------------
avl wrote:
> rastogishubham wrote:
> > avl wrote:
> > > No need to check for DW_AT_addr_base specifically here. Just checking for std::nullopt should be enough:
> > > 
> > > 
> > > ```
> > > if (!Addr) {
> > >   Linker.reportWarning("Cann't read address attribute value.", ObjFile);
> > >   return 0;
> > > }
> > > ```
> > > 
> > > 
> > That is incorrect, if we just check for std::nullopt, we will fail llvm/test/tools/dsymutil/X86/verify.test
> > 
> > This is because, that test adds broken dwarf where a duplicate DW_AT_language has the form DW_FORM_addr, and if we don't add that form to the Die, then the verify pass doesn't catch the error.
> This is exactly wanted behavior - dsymutil removed broken attribute and so verifier does not report the error.
> 
> We need to modify /tools/dsymutil/X86/verify.test so that it uses another kind of error. f.e. it might add DW_AT_language that has DW_FORM_ref_addr form or something like this.
Hi @avl 

I respectfully disagree, in this specific case dsymutil removed a broken attribute even though we tried to verify input dwarf, I thought the whole point of the `-verify` pass was to let us know if any input dwarf was invalid. Which we will not know in this case.

Any thoughts @JDevlieghere @aprantl ?


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

https://reviews.llvm.org/D155666