[llvm] [Sample Profile Loader] Fix potential invalidated reference (PR #73181)

[William Junda Huang via llvm-commits]

huangjd wrote:

> > > > > Previously (before md5phase2) ProfiledFunctions is a llvm::StringMap, which also have the same issue theoretically when rehashing but was not observed.
> > > > 
> > > > 
> > > > This is again the consequence of you breaking reference stability with the new container. Before your change, there is no issue with StringMap -- it guarantees reference stability and rehash does not cause the underlying StringMapEntryBase to be reallocated. The code works with the assumption of reference stability.
> > > 
> > > 
> > > In all fairness, the old code was probably written without reference stability in mind -- the StringMap might be implemented without that guarantee. I think making code not depending on that behavior can be more robust and immune to breakage in the future.
> > 
> > 
> > I'm pretty sure that's not the case. FWIW, I remember instances of such cases where originally in prototype backing container is used just so we can have reference stability. But such backing container was later removed/optimized away because other container provide stability guarantee.
> > As for guarantees, LLVM is fairly explicit, document points out DenseMap doesn't have reference stability, and StringMap has it.
> > My main point is, certain changes are just riskier, and removing reference stability is one of them. Breakage is fine, let's just acknowledge it and fix forward, rather than keep trying to attribute breakages to some "existing bug".
> 
> Many good points, thanks.
> 
> DenseMap does document invalidation of iterator on insertion, but StringMap does not document the opposite (though that property can be inferred from the implementation?): https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h
> 
> Having a backing container that guarantees the behavior (and with good unit test for it) to support exposing element address sounds like a good idea if it is so desired program pattern, but removing such dependency might be better.

After looking into the source code StringMap's implementation seems to have reference stability, however it is neither explicitly mentioned in (https://llvm.org/docs/ProgrammersManual.html) nor clearly stated in the source code's comments. In this case I think it's better to use a backing container, in case the implementation of the map is changed. 

https://github.com/llvm/llvm-project/pull/73181