[PATCH] D89707: [AutoFDO][llvm-profgen] Parse mmap events from perf script

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:27
+static cl::list<std::string> PerfTraceFilenames(
+    "perfscript", cl::value_desc("perfscript"), cl::OneOrMore,
+    llvm::cl::MiscFlags::CommaSeparated,
----------------
(flyby comment) is it a "perfscript" or a "perftrace" - or maybe "perfdata" rather.

Also, probably "perf tool" not "perf script" in the description.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:34
+                    llvm::cl::MiscFlags::CommaSeparated,
+                    cl::desc("Input profiled binary files"));
+
----------------
Path to profiled binary?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:49
+  std::string Path;
+  uint64_t BaseAddress;
+  bool IsLoaded;
----------------
Nit: to help maintainability down the road, perhaps ensure primitive fields are initialized here - I realize they are init-ed in the ctor, but it's even easier to maintain things when def and init on the same line. Less typing for the ctor, too.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:66
+    // TODO:
+    IsLoaded = true;
+  }
----------------
should this set IsLoaded to false?

also, why not assert(!IsLoaded); or rename to "ensureLoaded"; 

or - is there value in a ProfiledBinary that's not loadable? If not, how about:
- private ctor
- fields are const; no need for IsLoaded
- public static factory method returning a std::unique_ptr<ProfildBinary> that's null if loading fails.

that way, a reader knows they don't need to bother worrying about ProfiledBinaries that don't work - simpler overall state. wdyt?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:74
+struct MMapEvent {
+  pid_t PID;
+  uint64_t BaseAddress;
----------------
best to initialize struct fields to avoid undefined values.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:94
+    auto Buffer = std::move(BufferOrErr.get());
+    if (uint64_t(Buffer->getBufferSize()) >
+        std::numeric_limits<uint32_t>::max())
----------------
should this rather be Buffer->getBufferSize() > static_cast<size_t>(std::numeric_limits<uint32_t>::max())?




================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:105
+      return true;
+    if (*P == '\r' && *(P + 1) == '\n')
+      return true;
----------------
could P+1 be out of bounds?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:137
+    StringRef BinaryName = llvm::sys::path::filename(BinaryPath);
+    if (!BinaryTable.count(BinaryName))
+      return;
----------------
I guess this can happen because the Event could reference other binaries than the interesting ones, correct? Could you add a comment here about this - i.e. that the event is intentionally dropped here.




================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:140
+
+    ProfiledBinary &Binary = loadBinary(BinaryPath);
+
----------------
why call load - why not just fetch the ProfiledBinary entry? Unless I'm missing something, the expected behavior is that first the user-provided binaries are loaded; and if that goes well, then the events for them are loaded. So at this point the ProfiledBinary should be there?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:143-144
+    // Update the binary address map.
+    BinaryAddrMap.erase(Binary.getBaseAddress());
+    BinaryAddrMap[Event.BaseAddress] = &Binary;
+
----------------
wmi wrote:
> Here BinaryAddrMap needs to erase an entry before inserting a new one. I guess it is to support the case that a load image is unloaded and then reloaded at a different place. If that is correct, please add some comment to make it clear. 
Naive question: that assumes no interesting events were collected when the image was loaded at the old address?


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:174
+      MMapEvent Event;
+      Fields[1].getAsInteger(10, Event.PID);
+      Fields[2].getAsInteger(0, Event.BaseAddress);
----------------
consider using an enum like:

enum EventIndex {
  WholeLine = 0,
  PID = 1,
  BaseAddress = 2... (etc)
}

so then dereferencing Fields is more readable.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:206
+
+  void run() {
+    // Load the binaries.
----------------
perhaps instead of 'run' something more specific, like "loadBinariesAndEvents".


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:222
+
+int main(int argc, const char *argv[]) {
+  InitLLVM X(argc, argv);
----------------
probably can be done later just fine, but should the main be in its own file, so then this becomes a reusable library?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89707