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