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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:362-363
+    auto *Symbol = Binary->findDWARFSymbolForOffset(RangeBegin);
+    if (!Symbol)
+      continue;
+
----------------
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. 



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:78
+// skip storing start offset here.
+struct ElfRangeInfo {
+  // DWARF-based symbol name.
----------------
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?  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282