[PATCH] D115973: [llvm-profgen] Support symbol loading for debug fission

[Alexander Yermolovich via Phabricator via llvm-commits]

ayermolo added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:674
+    if (llvm::Optional<uint64_t> DWOId = DwarfUnit->getDWOId())
+      continue;
+    loadSymbolsFromDWARFUnit(DwarfUnit);
----------------
wlei wrote:
> ayermolo wrote:
> > Right now it will always use info from DWO sections, even if -fsplit-dwarf-inlining is set. I wonder if it's worth checking that if children exist use them. I think with split dwarf enabled -fsplit-dwarf-inlining is only option that will add children to Skeleton CU.
> Yeah, I tried `-fsplit-dwarf-inlining` and it indeed add the children to Skeleton CU, then we will get two same function symbols, one from main binary, one from dwo section. It seems both have enough info, probably it's still safer to use the union set of them, so removed this check this condition. Even we have the duplication, the loader will just use the first one.
> 
> Added a test for this.
I wonder if this even justifies a warning? With Debug Fission this warning will always be printed. It implies that something is wrong with DebugInfo in binary.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:292
+  // Load debug info from DWARF unit.
+  void loadSymbolsFromDWARFUnit(DWARFUnit *CompilationUnit);
+
----------------
Maybe make this pass by reference? Since we don't expect it this be nullptr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115973