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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95929