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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:674
+    if (llvm::Optional<uint64_t> DWOId = DwarfUnit->getDWOId())
+      continue;
+    loadSymbolsFromDWARFUnit(DwarfUnit);
----------------
ayermolo wrote:
> 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.
Good point, warning removed.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:48
+    DWPPath("dwp", cl::init(""), cl::ZeroOrMore,
+            cl::desc("Path of split dwarf .dwp/.dwo/.o file."));
+
----------------
ayermolo wrote:
> wlei wrote:
> > ayermolo wrote:
> > > There is one DWP file. There could be multiple .o/.dwo files in multiple directories. I don't think this option makes sense for the latter. The path to those should be encoded in DWARF CUs.
> > > Also for DWP it's not exactly a path, but fully resolved path+filename
> > > auto Obj = object::ObjectFile::createObjectFile(
> > >           this->DWPName.empty()
> > >               ? (DObj->getFileName() + ".dwp").toStringRef(DWPName)
> > >               : StringRef(this->DWPName));
> > Thanks! I see, while I was creating the test cases, I found that ".o and .dwo" were all encoded in absolute paths, then the tests will fail in others' machine, so I used this "-dwp" as a workaround to find the .o/.dwo file.
> > For this reason, how about that we give warning while using "-dwp" for .o and .dwo path?
> > 
> > > Also for DWP it's not exactly a path, but fully resolved path+filename
> > 
> > I see, so even if the DWP path is missing, it can search the dwp in the same directory of obj file with ".dwp" suffix. Updated its description.
> Warning seems like an overkill. Presumably people know what they are doing. I don't have a strong opinion one way or the other.
Sounds good, it seems most of time o/.dwo files are not visible to user, removed the warning.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115973