[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;
----------------
rastogishubham wrote:
> 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 ?
I am happy to modify verify.test if that is the consensus, but I am just giving my 2 cents on the matter at hand currently. 


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

https://reviews.llvm.org/D155666