[PATCH] D111371: [Support][ThinLTO] Move ThinLTO caching to LLVM Support library.

Teresa Johnson via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 8 23:06:38 PDT 2021


tejohnson added inline comments.


================
Comment at: llvm/lib/Support/Caching.cpp:36
 
+  SmallString<10> CacheName = CacheNameRef;
   return [=](unsigned Task, StringRef Key) -> AddStreamFn {
----------------
noajshu wrote:
> noajshu wrote:
> > tejohnson wrote:
> > > Is this copy necessary? I believe sys::path::append takes a Twine and eventually makes a copy of it.
> > Thanks, removed!
> Thanks again for this suggestion. On second thought I am concerned about lifetime issues with this approach. The `localCache` function returns a lambda which captures the arguments by copy. The Twine and StringRef classes just point to some temporary memory which could be modified/freed before the returned lambda is invoked. Making a copy (or just running the `sys::path::append` before returning the lambda) would protect from such bugs.
> 
> Here is a stripped-down example of the issue I'm concerned about:
> ```
> #include <iostream>
> #include <string.h>
> #include "llvm/ADT/Twine.h"
> 
> using namespace llvm;
> 
> auto localCacheDummy(Twine FooTwine) {
> 	return [=] () {
> 		std::cout << FooTwine.str();
> 	};
> }
> 
> int main() {
> 	std::string SpecialString = "hello here is a string\n";
> 	auto myFunction = localCacheDummy(SpecialString);
> 	myFunction();
> 	SpecialString = "ok I have replaced the string contents\n";
> 	myFunction();
> 	return 0;
> }
> ```
> This outputs:
> ```
> hello here is a string
> ok I have replaced the string contents
> ```
> This is an issue for `StringRef` as well, so I am concerned the [[ https://github.com/llvm/llvm-project/blob/dc066888bd98c0500ca7b590317dc91ccce0fd38/llvm/lib/LTO/Caching.cpp#L31 | existing caching code ]] is unsafe as well. This was probably fine when it was used solely with the usage pattern in ThinLTO. As we move it to the support library should we make it more safe? In particular, we could keep the parameters Twines but only access them in the top part of the `localCache` body outside the lambdas.
Hmm that is a good catch. I think you are right that we should be making a copy of all of these string reference parameters outside of the lambda (which we then capture by copy) to address this issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111371



More information about the cfe-commits mailing list