[libcxx-commits] [libcxx] ddb2baf - [libc++] Make sure we don't cache DSL functions too aggressively

[Louis Dionne via libcxx-commits]

Author: Louis Dionne
Date: 2020-10-09T10:22:46-04:00
New Revision: ddb2baf9fbff3d574c6c2bd69c2c9569100509a4

URL: https://github.com/llvm/llvm-project/commit/ddb2baf9fbff3d574c6c2bd69c2c9569100509a4
DIFF: https://github.com/llvm/llvm-project/commit/ddb2baf9fbff3d574c6c2bd69c2c9569100509a4.diff

LOG: [libc++] Make sure we don't cache DSL functions too aggressively

To make sure we don't store a mutable object (which could be modified by
outside code without us noticing) as the cache key, we pickle the cache
key to get a byte stream. If two keys are unequal, we know for sure they
will not have the same pickling. And if they are equal, there's a large
chance they will have the same pickling. If they don't, we might end up
not reusing a cached entry when we could have, but at least the behavior
we'll have is semantically correct.

Added: 
    

Modified: 
    libcxx/test/libcxx/selftest/dsl/dsl.sh.py
    libcxx/utils/libcxx/test/dsl.py

Removed: 
    


################################################################################
diff  --git a/libcxx/test/libcxx/selftest/dsl/dsl.sh.py b/libcxx/test/libcxx/selftest/dsl/dsl.sh.py
index 3c90ebe63d91..d20781ab9ea6 100644
--- a/libcxx/test/libcxx/selftest/dsl/dsl.sh.py
+++ b/libcxx/test/libcxx/selftest/dsl/dsl.sh.py
@@ -85,6 +85,18 @@ def getSubstitution(self, substitution):
         return found[0]
 
 
+def findIndex(list, pred):
+    """Finds the index of the first element satisfying 'pred' in a list, or
+       'len(list)' if there is no such element."""
+    index = 0
+    for x in list:
+        if pred(x):
+            break
+        else:
+            index += 1
+    return index
+
+
 class TestHasCompileFlag(SetupConfigs):
     """
     Tests for libcxx.test.dsl.hasCompileFlag
@@ -172,6 +184,29 @@ def test_pass_arguments_to_program(self):
         args = ["first-argument", "second-argument"]
         self.assertEqual(dsl.programOutput(self.config, source, args=args), "")
 
+    def test_caching_is_not_too_aggressive(self):
+        # Run a program, then change the substitutions and run it again.
+        # Make sure the program is run the second time and the right result
+        # is given, to ensure we're not incorrectly caching the result of the
+        # first program run.
+        source = """
+        #include <cstdio>
+        int main(int, char**) {
+            std::printf("MACRO=%u\\n", MACRO);
+        }
+        """
+        compileFlagsIndex = findIndex(self.config.substitutions, lambda x: x[0] == '%{compile_flags}')
+        compileFlags = self.config.substitutions[compileFlagsIndex][1]
+
+        self.config.substitutions[compileFlagsIndex] = ('%{compile_flags}',  compileFlags + ' -DMACRO=1')
+        output1 = dsl.programOutput(self.config, source)
+        self.assertEqual(output1, "MACRO=1\n")
+
+        self.config.substitutions[compileFlagsIndex] = ('%{compile_flags}',  compileFlags + ' -DMACRO=2')
+        output2 = dsl.programOutput(self.config, source)
+        self.assertEqual(output2, "MACRO=2\n")
+
+
 class TestHasLocale(SetupConfigs):
     """
     Tests for libcxx.test.dsl.hasLocale

diff  --git a/libcxx/utils/libcxx/test/dsl.py b/libcxx/utils/libcxx/test/dsl.py
index ecbb59ec8a0b..78f9841c0673 100644
--- a/libcxx/utils/libcxx/test/dsl.py
+++ b/libcxx/utils/libcxx/test/dsl.py
@@ -7,6 +7,7 @@
 #===----------------------------------------------------------------------===##
 
 import os
+import pickle
 import pipes
 import platform
 import re
@@ -24,21 +25,17 @@ def _memoizeExpensiveOperation(extractCacheKey):
   """
   Allows memoizing a very expensive operation.
 
-  The caching is implemented as a list, and we search linearly for existing
-  entries. This is incredibly naive, however this allows the cache keys to
-  contain lists and other non-hashable objects. Assuming the operation we're
-  memoizing is very expensive, this is still a win anyway.
+  We pickle the cache key to make sure we store an immutable representation
+  of it. If we stored an object and the object was referenced elsewhere, it
+  could be changed from under our feet, which would break the cache.
   """
   def decorator(function):
-    cache = []
+    cache = {}
     def f(*args, **kwargs):
-      cacheKey = extractCacheKey(*args, **kwargs)
-      try:
-        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))
-      return result
+      cacheKey = pickle.dumps(extractCacheKey(*args, **kwargs))
+      if cacheKey not in cache:
+        cache[cacheKey] = function(*args, **kwargs)
+      return cache[cacheKey]
     return f
   return decorator