[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;
+
----------------
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.. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:322
+  // DWARF subprogram name.
+  if (!Symbol->isEntryFunction && Symbol->SymbolName == ELFSymbolName)
+    Symbol->isEntryFunction = true;
----------------
nit: 

```
if (Symbol->SymbolName == ELFSymbolName)
  Symbol->isEntryFunction = true;

```


================
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 {
----------------
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. 


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


================
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
----------------
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. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:82
+  size_t RangesIdx;
+  uint64_t EndOffset;
+};
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282