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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
