[PATCH] D143458: [llvm][dsymutil] Add DW_TAG_imported_declaration to accelerator table

[Michael Buch via Phabricator via llvm-commits]

Michael137 added inline comments.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1592
     Unit.addNamespaceAccelerator(Die, AttrInfo.Name);
+  } else if (Tag == dwarf::DW_TAG_imported_declaration && AttrInfo.Name) {
+    Unit.addNamespaceAccelerator(Die, AttrInfo.Name);
----------------
JDevlieghere wrote:
> aprantl wrote:
> > Now that DWARFLinker.cpp has been moved from tools/dsymutil into lib/DWARFLinker do we also have access to the debugger tuning settings?
> > My understanding is that @avl is working on a DWARFLinker for ELF so if that flag is exposed here, it might be nice to make this conditional to match the AsmPrinter behavior. If it's not exposed, I think leaving it as is is probably okay.
> > 
> The DWARFLinker has options that allow it to be configured differently between users (like dsymutil). That said, looking at the DWARF spec:
> 
> > The name index must contain an entry for each debugging information entry that defines a named subprogram, label, variable, type, or namespace, subject to the following rules: [..]
> 
> Nothing in the later stated "rules" excludes `DW_TAG_imported_declaration` from being part of the index. So following the wording of the spec, we "must" include it.  
I interpreted it as being optional to include an imported declaration (since imported declaration != namespace, although for our purposes that is the case). That said, we haven't put a version check on the codegen side either, so probably not necessary here


================
Comment at: llvm/test/tools/dsymutil/ARM/accel-imported-declarations.test:3
+RUN: dsymutil -accelerator=Apple %p/../Inputs/accel-imported-declaration.macho-arm64 -o %t.apple.dSYM
+RUN: dsymutil -accelerator=None %p/../Inputs/accel-imported-declaration.macho-arm64 -o %t.none.dSYM
+
----------------
avl wrote:
> it looks like -accelerator=None run is not necessary?
Good catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143458