[PATCH] D52933: Fix incorrect Twine usage in CFGPrinter

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

Thank you @kristina for the review. I must admit I do not fully understand in which way this is broken, but the attached test case clearly shows there is some memory issue. My (limited) understanding is that Twine(Weight->getZExtValue()) will be destroyed at the end of line 175 which means that already in line 176 we may potentially reference invalid memory.
http://llvm.org/docs/ProgrammersManual.html#dss-twine has a similar example and explicitly warns of such uses:

"Because Twine is constructed with temporary objects on the stack, and because these instances are destroyed at the end of the current statement, it is an inherently dangerous API. For example, this simple variant contains undefined behavior and will probably crash:

  void foo(const Twine &T);
  
  StringRef X = ...
  unsigned i = ...
  const Twine &Tmp = X + "." + Twine(i);
  foo(Tmp);

because the temporaries are destroyed before the call. That said, Twine’s are much more efficient than intermediate std::string temporaries, and they work really well with StringRef. Just be aware of their limitations."

This change also fixes https://bugs.llvm.org/PR37019.

Marcin, twines are very hard to understand. It might make sense to add both a link the the programmers manual reference I just cited as well as a link to the bug report that is addressed with this patch. We should also acknowledge that a similar patch has already been written in the context of the LLVM lite project.

@kristina, what exactly do you suggest should be changed?


Repository:
  rL LLVM

https://reviews.llvm.org/D52933