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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/split-dwarf.test:12
+; RUN: llvm-profgen --format=text --unsymbolized-profile=%t --dwp=%S/Inputs/split-dwarf-split-inlining.dwo --binary=%S/Inputs/split-dwarf-split-inlining.perfbin --output=%t4 --fill-zero-for-all-funcs
+; RUN: FileCheck %s --input-file %t4 --check-prefix=CHECK-SPLIT-DWARF
+
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > ayermolo wrote:
> > > > hoy wrote:
> > > > > wlei wrote:
> > > > > > hoy wrote:
> > > > > > > Add a test case for no --dwp?
> > > > > > Good point, added. Without -dwp, it will only load the inliner function.
> > > > > Thanks for adding the test. Per the dwo switch description, should the dwo file be loaded if it is not specified and placed alongside with the binary?
> > > > > 
> > > > > `When not specified, it will be <binary>.dwp in the same directory as the main binary.` 
> > > > > 
> > > > > I guess it'll look for `split-dwarf-split-inlining.perfbin.dwo` instead of `split-dwarf-split-inlining.dwo`?
> > > > .dwo/.o files don't need to be next to the binary, and there is no assumption that they will be. In fact in production environment they are not. The fully resolved path is specified in DWARF CU. If .dwo/.o files are not there then it's a warning/error. Depending on context.
> > > Yeah, but in this case we have them side by side. I guess it's an issue of the file naming that resulted in the dwo file not loaded. Should we change the arg description to not support the automatic load?
> > Yeah, for .dwo/.o files, if not specified --dwp, it will used the path encoded in the CU and it's an absolute path. In this test, the path is pointed to a directory in my machine, so it will fail for others.
> > 
> > For .dwp file, if not specified --dwp, it will be <binary>.dwp in the same directory.
> > 
> > In fact, --dwp is designed only to be used for .dwp file. In real case, we won't expect -dwp used for .dwo/.o files, here only work around for the testing.
> I see. I misunderstood dwo files as dwp files. It's clear to me now. Can you please add the clarification in the description of the dwp switch? LGTM otherwise.
Other tests for Split DWARF that need to test .dwo access use binaries/objects created with  `-fdebug-compilation-dir` (or `-fdebug-prefix-map`) to avoid baking in an absolute path. The test case can copy the .dwo to a known temp directory, have the test `cd` into that directory (or a parent, if `-fdebug-compilation-dir` is made to be a parent of some subdir the .dwo is in) and then the tool should be able to resolve the relative path.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:685
+      DWARFUnit *DWOCU = DwarfUnit->getNonSkeletonUnitDIE(false).getDwarfUnit();
+      if (!DWOCU->isDWOUnit()) {
+        std::string DWOName = dwarf::toString(
----------------
ayermolo wrote:
> wlei wrote:
> > hoy wrote:
> > > What is a DWO unit checked against here? Is it an indirection to the real compilation unit?
> > I think it's an internal check whether the current parsed result is a DWO unit.
> > The `getNonSkeletonUnitDIE` is expected to get a DWOUnit, if the parsing failed and not a DWOUnit, it's likely due to the incorrect dwo file path.
> Right. It's a bit weird implementation. If it fails to parse and produce DWO Die it invokes getUnitDIE and returns a Die for Skeleton CU.
We can change APIs if they don't behave in a reasonable way.

LLVM's symbolizer APIs have to look through from skeleton units to split units - We probably should refactor/reuse that code here, if possible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115973