features: Update compiler caching (#7675)

Compiler caching was using the `id()` function to refer to configuration dictionary objects. If these objects are garbage-collected, this can produce incorrect results (false positive cache hits). This change replaces `id()` with an object that keeps a reference to the config dictionary so that it is not garbage-collected.

2 files changed, 60 insertions, 34 deletions

diff --git a/lib/spack/spack/compilers/__init__.py b/lib/spack/spack/compilers/__init__.py
index 5adfd17b1a..20f40eef09 100644
--- a/lib/spack/spack/compilers/__init__.py
+++ b/lib/spack/spack/compilers/__init__.py
@@ -278,7 +278,7 @@ def all_compilers(scope=None):
     compilers = list()
     for items in config:
         items = items['compiler']
-        compilers.append(compiler_from_config_entry(items))
+        compilers.append(_compiler_from_config_entry(items))
     return compilers
 
 
@@ -305,41 +305,69 @@ def compilers_for_arch(arch_spec, scope=None):
     return list(get_compilers(config, arch_spec=arch_spec))
 
 
-def compiler_from_config_entry(items):
-    config_id = id(items)
-    compiler = _compiler_cache.get(config_id, None)
+class CacheReference(object):
+    """This acts as a hashable reference to any object (regardless of whether
+       the object itself is hashable) and also prevents the object from being
+       garbage-collected (so if two CacheReference objects are equal, they
+       will refer to the same object, since it will not have been gc'ed since
+       the creation of the first CacheReference).
+    """
+    def __init__(self, val):
+        self.val = val
+        self.id = id(val)
+
+    def __hash__(self):
+        return self.id
+
+    def __eq__(self, other):
+        return isinstance(other, CacheReference) and self.id == other.id
 
-    if compiler is None:
-        cspec = spack.spec.CompilerSpec(items['spec'])
-        os = items.get('operating_system', None)
-        target = items.get('target', None)
 
-        if not ('paths' in items and
-                all(n in items['paths'] for n in _path_instance_vars)):
-            raise InvalidCompilerConfigurationError(cspec)
+def compiler_from_dict(items):
+    cspec = spack.spec.CompilerSpec(items['spec'])
+    os = items.get('operating_system', None)
+    target = items.get('target', None)
 
-        cls  = class_for_compiler_name(cspec.name)
+    if not ('paths' in items and
+            all(n in items['paths'] for n in _path_instance_vars)):
+        raise InvalidCompilerConfigurationError(cspec)
 
-        compiler_paths = []
-        for c in _path_instance_vars:
-            compiler_path = items['paths'][c]
-            if compiler_path != 'None':
-                compiler_paths.append(compiler_path)
-            else:
-                compiler_paths.append(None)
+    cls  = class_for_compiler_name(cspec.name)
 
-        mods = items.get('modules')
-        if mods == 'None':
-            mods = []
+    compiler_paths = []
+    for c in _path_instance_vars:
+        compiler_path = items['paths'][c]
+        if compiler_path != 'None':
+            compiler_paths.append(compiler_path)
+        else:
+            compiler_paths.append(None)
 
-        alias = items.get('alias', None)
-        compiler_flags = items.get('flags', {})
-        environment = items.get('environment', {})
-        extra_rpaths = items.get('extra_rpaths', [])
+    mods = items.get('modules')
+    if mods == 'None':
+        mods = []
 
-        compiler = cls(cspec, os, target, compiler_paths, mods, alias,
-                       environment, extra_rpaths, **compiler_flags)
-        _compiler_cache[id(items)] = compiler
+    alias = items.get('alias', None)
+    compiler_flags = items.get('flags', {})
+    environment = items.get('environment', {})
+    extra_rpaths = items.get('extra_rpaths', [])
+
+    return cls(cspec, os, target, compiler_paths, mods, alias,
+               environment, extra_rpaths, **compiler_flags)
+
+
+def _compiler_from_config_entry(items):
+    """Note this is intended for internal use only. To avoid re-parsing
+       the same config dictionary this keeps track of its location in
+       memory. If you provide the same dictionary twice it will return
+       the same Compiler object (regardless of whether the dictionary
+       entries have changed).
+    """
+    config_id = CacheReference(items)
+    compiler = _compiler_cache.get(config_id, None)
+
+    if compiler is None:
+        compiler = compiler_from_dict(items)
+        _compiler_cache[config_id] = compiler
 
     return compiler
 
@@ -367,7 +395,7 @@ def get_compilers(config, cspec=None, arch_spec=None):
                                      target != 'any'):
             continue
 
-        compilers.append(compiler_from_config_entry(items))
+        compilers.append(_compiler_from_config_entry(items))
 
     return compilers
 
diff --git a/lib/spack/spack/test/compilers.py b/lib/spack/spack/test/compilers.py
index 00a0fab41c..f34b9ceb5a 100644
--- a/lib/spack/spack/test/compilers.py
+++ b/lib/spack/spack/test/compilers.py
@@ -129,7 +129,7 @@ def test_compiler_flags_from_config_are_grouped():
         'modules': None
     }
 
-    compiler = compilers.compiler_from_config_entry(compiler_entry)
+    compiler = compilers.compiler_from_dict(compiler_entry)
     assert any(x == '-foo-flag foo-val' for x in compiler.flags['cflags'])
 
 
@@ -179,9 +179,7 @@ def flag_value(flag, spec):
     else:
         compiler_entry = copy(default_compiler_entry)
         compiler_entry['spec'] = spec
-        # Disable faulty id()-based cache (issue #7647).
-        compilers._compiler_cache = {}
-        compiler = compilers.compiler_from_config_entry(compiler_entry)
+        compiler = compilers.compiler_from_dict(compiler_entry)
 
     return getattr(compiler, flag)