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
