[PATCH] D89707: [CSSPGO][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:66
+    // TODO:
+    IsLoaded = true;
+  }
----------------
hoy wrote:
> 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.
> 
> 
So then could the loading happen in the ctor, since failure to load => fast fail? Then the ProfiledBinary is known to be always loaded, and the only mutable field is the base address - to be clear, my suggestion is for simpler maintainability.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89707