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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:66
+    // TODO:
+    IsLoaded = true;
+  }
----------------
mtrofin wrote:
> 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?
Asserting the function is only run once per binary is reasonable since all provided binaries are loaded as soon as the tool starts. The initial design allowed for a load on demand, i.e, when binaries are not provided via the command line option, the profiled binaries record by mmap events will be automatically loaded while processing the events. `IsLoaded` was introduced as a signal for that. However that complicated the tool and was not continued. 

A load failure would result the tool to error and exit instead of returning a null object. Please see subsequent patch D89712.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89707