[PATCH] D157061: [SampleProfile] Potential use after move in SampleProfileLoader::promoteMergeNotInlinedContextSamples

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1626
+        if (!OutlineFS)
+          OutlineFS = &OutlineFunctionSamples[
+              FunctionSamples::getCanonicalFnName(Callee->getName())];
----------------
huangjd wrote:
> huangjd wrote:
> > wenlei wrote:
> > > We could also be inserting into `OutlineFunctionSamples` which can cause rehash while using a FunctionSamples from `OutlineFunctionSamples`? 
> > > 
> > > I'm wondering if `OutlineFunctionSamples` should use a container that guarantees reference stability, e.g. `std::map` instead of `std::unordered_map`.
> > > 
> > > 
> > That's OK, there is no pointer reference OutlineFunctionSamples, so there's no concern of segfault. Performance is trivial because very few inlined functions are actually promoted to outline function (say < 10 in a production project )
> The pointer here is used immediately and won't be used outside of the function 
> That's OK, there is no pointer reference OutlineFunctionSamples, so there's no concern of segfault. 

Imagine this case: we are processing a function (inside `SampleProfileLoader::runOnFunction`) whose own profile is retrieved from `OutlineFunctionSamples`, now its callee profile got inserted into `OutlineFunctionSamples`, causing a rehash, so `SampleProfileLoader::Samples` now becomes a dangling reference, 

> Performance is trivial because very few inlined functions are actually promoted to outline function (say < 10 in a production project )

Right, so we can afford to use `std::map`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157061