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.

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,11 +305,25 @@ def compilers_for_arch(arch_spec, scope=None):
     return list(get_compilers(config, arch_spec=arch_spec))
 
 
-def compiler_from_config_entry(items):
+class CacheReference(object):
-    config_id = id(items)
+    """This acts as a hashable reference to any object (regardless of whether
-    compiler = _compiler_cache.get(config_id, None)
+       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)
 
-    if compiler is None:
+    def __hash__(self):
+        return self.id
+
+    def __eq__(self, other):
+        return isinstance(other, CacheReference) and self.id == other.id
+
+
+def compiler_from_dict(items):
     cspec = spack.spec.CompilerSpec(items['spec'])
     os = items.get('operating_system', None)
     target = items.get('target', None)
@@ -337,9 +351,23 @@ def compiler_from_config_entry(items):
     environment = items.get('environment', {})
     extra_rpaths = items.get('extra_rpaths', [])
 
-        compiler = cls(cspec, os, target, compiler_paths, mods, alias,
+    return cls(cspec, os, target, compiler_paths, mods, alias,
                environment, extra_rpaths, **compiler_flags)
-        _compiler_cache[id(items)] = compiler
+
+
+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
 

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).
+        compiler = compilers.compiler_from_dict(compiler_entry)
-        compilers._compiler_cache = {}
-        compiler = compilers.compiler_from_config_entry(compiler_entry)
 
     return getattr(compiler, flag)