[PATCH] D54923: [Modules] Remove non-determinism while serializing DECL_CONTEXT_LEXICAL and DECL_RECORD

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: rsmith, v.g.vassilev.
Herald added subscribers: dexonsmith, mgrang, jkorous.

A module signature hashes out all relevant content in a module in order
to guarantee that in a dep chain, an inconsistent module never gets
imported. When using implicit modules, clang is capable of rebuilding a
module in case its signature doesn't match what's expected in the
importer. However, when using PCHs + implicit modules, clang cannot
rebuild a imported module with a signature mismatch, since it doesn't
know how to rebuild PCHs.

Non-determinism in the module content, can lead to changes to the
signature of a PCM across compiler invocations that are expected to
produce the same result, causing:

- Build time penalties: rebuilding modules can be expensive.
- Failures: when using PCHs, leads to hard errors one cannot recover

from.

This patch address non-determinism when serializing DECL_CONTEXT_LEXICAL
and DECL_RECORD by sorting the decls by ID (which is supposed to be
deterministic across multiple invocations) prior to writing out the
records. Another approach would be to fix all the places that trigger
adding a Decl to a DeclContext in non deterministic order, but I
couldn't spot any after an audit, but probably didn't look at everything
since the number of different sites and nested calls that do that is
pretty high. Also, by fixing it right before serialization, we enforce
that future changes also don't mess up with the order.

This isn't new, I consistently tracked this behavior in clang all the
way back to 2016, where my testcase stops working for other reasons.

I have no sensible and reduced testcases to add to this patch.

rdar://problem/43442957


https://reviews.llvm.org/D54923

Files:
  lib/Serialization/ASTCommon.h
  lib/Serialization/ASTWriter.cpp


Index: lib/Serialization/ASTWriter.cpp
===================================================================
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -3182,7 +3182,16 @@
 
   uint64_t Offset = Stream.GetCurrentBitNo();
   SmallVector<uint32_t, 128> KindDeclPairs;
-  for (const auto *D : DC->decls()) {
+
+  // Decls have deterministic IDs, but they might show up in different order in
+  // a DeclContext. Make sure serialization gets the same order everytime by
+  // sorting the decls by ID.
+  SmallVector<Decl *, 64> SortedDecls(DC->decls());
+  llvm::sort(SortedDecls, [](const Decl *L, const Decl *R) {
+    return L->getID() < R->getID();
+  });
+
+  for (const auto *D : SortedDecls) {
     KindDeclPairs.push_back(D->getKind());
     KindDeclPairs.push_back(GetDeclRef(D));
   }
Index: lib/Serialization/ASTCommon.h
===================================================================
--- lib/Serialization/ASTCommon.h
+++ lib/Serialization/ASTCommon.h
@@ -96,6 +96,7 @@
 template<typename Fn> void numberAnonymousDeclsWithin(const DeclContext *DC,
                                                       Fn Visit) {
   unsigned Index = 0;
+  SmallVector<NamedDecl *, 2> DeclsToVisit;
   for (Decl *LexicalD : DC->decls()) {
     // For a friend decl, we care about the declaration within it, if any.
     if (auto *FD = dyn_cast<FriendDecl>(LexicalD))
@@ -105,8 +106,17 @@
     if (!ND || !needsAnonymousDeclarationNumber(ND))
       continue;
 
-    Visit(ND, Index++);
+    DeclsToVisit.push_back(ND);
   }
+
+  // Decls have deterministic IDs, but they might show up in different order in
+  // a DeclContext. Make sure serialization gets the same order everytime by
+  // sorting the decls by ID.
+  llvm::sort(DeclsToVisit, [](const NamedDecl *L, const NamedDecl *R) {
+    return L->getID() < R->getID();
+  });
+  for (auto *ND : DeclsToVisit)
+    Visit(ND, Index++);
 }
 
 } // namespace serialization


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D54923.175372.patch
Type: text/x-patch
Size: 1952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181127/08eaabd8/attachment-0001.bin>