[Modules] A second attempt at writing out on-disk hash tables for the

decl context lookup tables.

The first attepmt at this caused problems. We had significantly more
sources of non-determinism that I realized at first, and my change
essentially turned them from non-deterministic output into
use-after-free. Except that they weren't necessarily caught by tools
because the data wasn't really freed.

The new approach is much simpler. The first big simplification is to
inline the "visit" code and handle this directly. That works much
better, and I'll try to go and clean up the other caller of the visit
logic similarly.

The second key to the entire approach is that we need to *only* collect
names into a stable order at first. We then need to issue all of the
actual 'lookup()' calls in the stable order of the names so that we load
external results in a stable order. Once we have loaded all the results,
the table of results will stop being invalidated and we can walk all of
the names again and use the cheap 'noload_lookup()' method to quickly
get the results and serialize them.

To handle constructors and conversion functions (whose names can't be
stably ordered) in this approach, what we do is record only the visible
constructor and conversion function names at first. Then, if we have
any, we walk the decls of the class and add those names in the order
they occur in the AST. The rest falls out naturally.

This actually ends up simpler than the previous approach and seems much
more robust.

It uncovered a latent issue where we were building on-disk hash tables
for lookup results when the context was a linkage spec! This happened to
dodge all of the assert by some miracle. Instead, add a proper predicate
to the DeclContext class and use that which tests both for function
contexts and linkage specs.

It also uncovered PR23030 where we are forming somewhat bizarre negative
lookup results. I've just worked around this with a FIXME in place
because fixing this particular Clang bug seems quite hard.

I've flipped the first part of the test case I added for stability back
on in this commit. I'm taking it gradually to try and make sure the
build bots are happy this time.

llvm-svn: 233249

2 files changed, 9 insertions, 0 deletions

diff --git a/clang/include/clang/AST/DeclBase.h b/clang/include/clang/AST/DeclBase.h
index 3c61c1ea833..b3ecbf76d72 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -1209,6 +1209,11 @@ public:
     }
   }
 
+  /// \brief Test whether the context supports looking up names.
+  bool isLookupContext() const {
+    return !isFunctionOrMethod() && DeclKind != Decl::LinkageSpec;
+  }
+
   bool isFileContext() const {
     return DeclKind == Decl::TranslationUnit || DeclKind == Decl::Namespace;
   }
diff --git a/clang/include/clang/Serialization/ASTWriter.h b/clang/include/clang/Serialization/ASTWriter.h
index d120c98553f..5f702223ac6 100644
--- a/clang/include/clang/Serialization/ASTWriter.h
+++ b/clang/include/clang/Serialization/ASTWriter.h
@@ -61,6 +61,7 @@ class PreprocessingRecord;
 class Preprocessor;
 class Sema;
 class SourceManager;
+struct StoredDeclsList;
 class SwitchCase;
 class TargetInfo;
 class Token;
@@ -505,6 +506,9 @@ private:
   void WriteTypeAbbrevs();
   void WriteType(QualType T);
 
+  bool isLookupResultExternal(StoredDeclsList &Result, DeclContext *DC);
+  bool isLookupResultEntirelyExternal(StoredDeclsList &Result, DeclContext *DC);
+
   template<typename Visitor>
   void visitLocalLookupResults(const DeclContext *DC, Visitor AddLookupResult);