noajshu added inline comments.
================
Comment at: llvm/lib/Support/Caching.cpp:36
+ SmallString<10> CacheName = CacheNameRef;
return [=](unsigned Task, StringRef Key) -> AddStreamFn {
----------------
tejohnson wrote:
> 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.
Thanks! These explicit copies have been added at the top of localCache.
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
