[PATCH] D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:362-363
+    auto *Symbol = Binary->findDWARFSymbolForOffset(RangeBegin);
+    if (!Symbol)
+      continue;
+
----------------
wenlei wrote:
> wlei wrote:
> > wenlei wrote:
> > > Just double check, the warnings are not being removed, but moved and covered by a separate patch, right?
> > > 
> > > Not a big deal, but it would be good for review if you can either 1) still keep it for review, 2) make a stack on top of the other patch so the warning doesn't show up on left side at all.. 
> > Good point. I actually intend to remove the warning here because I found some code in plt section which don't match any DWARF symbol will trigger this warning and it's a false positive. Added some comments to this.
> > 
> > 
> > 
> > because I found some code in plt section which don't match any DWARF symbol  
> 
> But we're looking for elf symbol here, not dwarf symbol? 
> 
> Regardless I'm hoping that we still have some sanity check for profiles so users would get a hint when they use mismatched profile and binary. 
> 
> But we're looking for elf symbol here, not dwarf symbol?
 I think the name is misleading, here it's not the elf symbol. Sorry for the confusing. Changed to `FuncRange` as you suggested.

> Regardless I'm hoping that we still have some sanity check for profiles so users would get a hint when they use mismatched profile and binary.
Yeah, I will pick up https://reviews.llvm.org/D111902 for the sanity check.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:584
+        if (R.second) {
+          ElfRangeInfo &Symbol = R.first->second;
+          Symbol.DWARFSymbolName = *It.first;
----------------
hoy wrote:
> nit: Symbol -> RangeInfo
fixed, thanks!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:78
+// skip storing start offset here.
+struct ElfRangeInfo {
+  // DWARF-based symbol name.
----------------
wenlei wrote:
> hoy wrote:
> > We are using both elf ranges and dwarf ranges as the terms. Should we stick to one, say DwarfRangeInfo and DWARFRangesVectorTy or the other way around?
> I would suggest we use `FuncRange` instead of tie this with dwarf/elf in the name. We don't support windows today, but nothing prevents us from supporting windows (PE/PDB) in the future. 
> 
> Accordingly, we can have something like `FuncSymName`, `RangeSymName`. And for `IsFunctionEntry`, the two names would be the same. What do you think?  
Thanks for the suggestions. `FuncRange` is a better name, renamed all corresponding variable names. 


> We are using both elf ranges and dwarf ranges as the terms. Should we stick to one, say DwarfRangeInfo and DWARFRangesVectorTy or the other way around?
Changed to `FuncRange` or `RangesTy`


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:352
 
-  StringRef getFuncFromStartOffset(uint64_t Offset) {
-    auto I = FuncStartOffsetMap.find(Offset);
-    if (I == FuncStartOffsetMap.end())
-      return StringRef();
-    return I->second.first;
+  ElfRangeInfo *getElfRangeInfoForStartOffset(uint64_t Offset) {
+    auto I = StartOffset2ElfRangeInfoMap.find(Offset);
----------------
hoy wrote:
> nit: getElfRangeInfoForStartOffset -> findElfRangeInfoForStartOffset to be consistent with the function naming below.
Fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282