[PATCH] D115973: [llvm-profgen] Support symbol loading for debug fission
Lei Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 20 23:19:18 PST 2021
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:
> 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.
================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:690
+ << " was not retrieved and won't be updated. Please check "
+ "relative path.\n";
continue;
----------------
ayermolo wrote:
> The path can be either relative or absolute.
> The path can be either relative or absolute.
I see, updated the comments, it appears that symbolizer requires the DWP file path, so added a cmd flag for it((BOLT also have it).
> There's no debug info update involved here, is this message a copy paste?
Removed, thanks. Yeah, I copied the `getNonSkeletonUnitDIE` part of code from BOLT code.
>And this should probably be a warning?
Changed to warning,
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115973/new/
https://reviews.llvm.org/D115973
More information about the llvm-commits
mailing list