noajshu marked 3 inline comments as not done.
noajshu added inline comments.


================
Comment at: llvm/lib/Support/Caching.cpp:36
 
+  SmallString<10> CacheName = CacheNameRef;
   return [=](unsigned Task, StringRef Key) -> AddStreamFn {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111371

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to