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

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1372
+        !AddrAttribute->getUnit()->getAddrOffsetSectionBase())
+      return 0;
     Addr = 0;
----------------
rastogishubham wrote:
> 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. 
Is attribute really removed while input DWARF verified? I think that we verify input DWARF before modifications done by dsymutil.


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

https://reviews.llvm.org/D155666