[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:
> 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.





================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:322
+  // DWARF subprogram name.
+  if (!Symbol->isEntryFunction && Symbol->SymbolName == ELFSymbolName)
+    Symbol->isEntryFunction = true;
----------------
wenlei wrote:
> nit: 
> 
> ```
> if (Symbol->SymbolName == ELFSymbolName)
>   Symbol->isEntryFunction = true;
> 
> ```
This is because we can have multiple offset query the same function.  String cmp is heavy, so skip if isEntryFunction is already set.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:75
+// Function symbol extracted from DWARF-based debug info. A function can be
+// split into multiple ranges, each range corresponds to one DWARFSymbol.
+struct DWARFSymbol {
----------------
wenlei wrote:
> This is a bit confusing - the name `DWARFSymbol` indicates this is 1:1 mapping to actual dwarf symbol, however for dwarf there's one symbol for one function, instead of one symbol for one range. 
I see, understand it's not a clear name. It's not a function in dwarf, it's more like the range in ELF but it's not only the range and has other info. One function can have multiple ranges, I changed to `ElfRangeInfo` meaning a mixed info of the elf range. see if this looks good to you?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:77
+struct DWARFSymbol {
+  bool isEntryFunction = false;
+  std::string SymbolName;
----------------
wenlei wrote:
> `EntryFunction` is a confusing name - usually that means entry point of a program. I guess what you meant here `FunctionEntry`?
`FunctionEntry `sounds good


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:78
+  bool isEntryFunction = false;
+  std::string SymbolName;
+  // Index to access AllDWARFRanges, used to look up all ranges of the
----------------
wenlei wrote:
> If this is indeed dwarf symbol as the struct name suggests, all the ranges of a function would share the same name, and we'd be duplicating std::string in that case. If this is actual ELF symbol name, they'll be different, then we need to name it explicitly as ElfSymbolName. 
this is the Dwarf-based name, renamed and added the code to deduplicate the std::string


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:82
+  size_t RangesIdx;
+  uint64_t EndOffset;
+};
----------------
wenlei wrote:
> As struct representation for a symbol, having EndOffset only but not start offset seem weird. I think we either "complete" it as a general symbol definition, or rename it to make it explicit that this is just some special purpose struct to specific bookkeeping map. 
> 
> Also please comment if the end offset is inclusive or exclusive. 
I added some comments to clarify this, i, e. this struct is always accessed by `StartOffset2ElfRangeInfoMap` whose key is the start offset so just skip it here. See if this's clearer, I'm also fine to add the "StartOffset" for good readability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282