[libcxx-commits] [PATCH] D89109: [libcxx][dsl] Avoid false-positive cache hits

[Alexander Richardson via Phabricator via libcxx-commits]

arichardson created this revision.
arichardson added reviewers: libc++, ldionne.
Herald added subscribers: libcxx-commits, dexonsmith.
Herald added a project: libc++.
Herald added 1 blocking reviewer(s): libc++.
arichardson requested review of this revision.

The libc++ dsl gained a cache to improve startup performance in
5390c5a96e9774f0f7d748b5a466c308f916ecd0 <https://reviews.llvm.org/rG5390c5a96e9774f0f7d748b5a466c308f916ecd0>, but it turns out this cache
can return cache hits even when it shouldn't.
We currently pass c.substitutions as one of the cache keys. However, this
means we are storing a reference to a mutable list as the cache key.
I added some debugging code that modifies config.substitutions between
checks and it turns out that the cache hits even though it shouldn't.
As a workaround for this issue, we now store a deep copy of the cache key
in the cache. I plan to submit a follow-up change that only saves the
relevant substitutions to reduce the amount of copying.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89109

Files:
  libcxx/utils/libcxx/test/dsl.py


Index: libcxx/utils/libcxx/test/dsl.py
===================================================================
--- libcxx/utils/libcxx/test/dsl.py
+++ libcxx/utils/libcxx/test/dsl.py
@@ -6,6 +6,7 @@
 #
 #===----------------------------------------------------------------------===##
 
+import copy
 import os
 import pipes
 import platform
@@ -37,7 +38,10 @@
         result = next(res for (key, res) in cache if key == cacheKey)
       except StopIteration: # This wasn't in the cache
         result = function(*args, **kwargs)
-        cache.append((cacheKey, result))
+        # Note: we have to use copy.deepcopy() here to avoid storing a
+        # mutable list in the cache, since that would cause cache hits
+        # even when the list changed compared to when the value was cached.
+        cache.append((copy.deepcopy(cacheKey), result))
       return result
     return f
   return decorator


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89109.297161.patch
Type: text/x-patch
Size: 901 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20201009/6454edc2/attachment.bin>