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

Fri Apr 2 10:05:46 PDT 2021

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95929