[PATCH] D49676: [DWARF] support for .debug_addr (consumer)

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Related aside to my comments regarding version mismatches: is it likely that future versions of DWARF (e.g. DWARF 6) will update the version number of the .debug_addr tables without changing anything in them? I know in previous versions of DWARF, some sections didn't increase in version number in line with the overall standard version. If not, then the version mismatch check may not make sense anyway.



================
Comment at: lib/DebugInfo/DWARF/DWARFDebugAddr.cpp:95-98
+  // FIXME: For now we just treat version mismatch as error,
+  // however the correct way to associate a .debug_addr table
+  // with a .debug_info table is to look at the DW_AT_addr_base
+  // attribute in the info table.
----------------
vleschuk wrote:
> jhenderson wrote:
> > I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.
> > 
> > I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.
> > 
> > Nit: as error, however the -> as an error. However, the
> > 
> > Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.
> > I'd move this FIXME to where the UnitVersion is calculated in DWARFContext.cpp.
> 
> I think it better to leave it here where the actual comparison is performed.
> 
> > I also think the last two lines of this comment make no sense on their own, and can probably just be deleted.
> 
> Agreed.
> 
> > Aside: you don't need to manually wrap comments, because clang-format will do it for you at the correct column width.
> 
> Yep, sorry, always forget to use this tool, need to hotkey it to my vimrc =)
But the error message will still want to exist, I believe, so the issue is finding the right CU, which is not done here. If somebody where to want to call this from another place, they might copy the existing call site, and will not be aware of the FIXME in that instance, resulting in them copying potentially dangerous code.


https://reviews.llvm.org/D49676