[PATCH] D95929: [CSSPGO][llvm-profgen] Add brackets for context id to support extended binary format

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:392
+  for (const auto &Item : ProfileMap) {
+    std::string ContextWithBracket = "[" + Item.first().str() + "]";
+    auto Ret = CxtWithBracketPMap.try_emplace(ContextWithBracket, Item.second);
----------------
hoy wrote:
> wenlei wrote:
> > wlei wrote:
> > > hoy wrote:
> > > > wenlei wrote:
> > > > > hoy wrote:
> > > > > > wenlei wrote:
> > > > > > > This seems problematic, the string could get destructed while we still keep the StringRef for it in FunctionSamples's name and context..
> > > > > > Does the try_emplace below copy construct a new one?
> > > > > CxtWithBracketPMap is a StringMap, so key is StringRef not std::string.
> > > > Oh right, this is a problem. Good catch! How did you find it out?
> > > I did think this is a problematic but after I used "Ret.first->first()" to get the StringRef, it seems something extend the lifetime of the string, otherwise `Writer->write(CxtWithBracketPMap);` will never work. do you have any case to trigger this?
> > I was doing some refactoring and a seemingly NFC change tripped this bug. I will send up a fix together with the refactoring.
> The std::string object will be deallocated out once it goes out of the loop. But the underlying memory may not be overwritten depending on the heap usage in the system. It worked probably because no other memory allocation reclaims that piece of memory.
Thanks for the explanation. If so, the global `ProfileMap` is also problematic, IIRC, it also used a local std::string used similar as here..

At that time, I tried to avoid to do memory copy for the string, so didn't copy to a global lifetime container. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95929