[clang] 0480748 - [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via cfe-commits]

Author: Steven Wu
Date: 2023-02-02T15:16:20-08:00
New Revision: 0480748ea6728392886931b8470969ae17aaa91f

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

LOG: [DeclContext] Sort the Decls before adding into DeclContext

Fix a non-deterministic issue in clang module generation, which the
anonymous declaration number from a function context is not
deterministic. This is due to the unstable iteration order for decls in
scope so the order after moving the decls into function decl context is
not deterministic.

>From https://reviews.llvm.org/D135118, we can't use a set that preserves
the order without the performance penalty. Fix the issue by sorting the
decls based on raw encoding of their source location.

rdar://104097976

Reviewed By: akyrtzi, vsapsai

Differential Revision: https://reviews.llvm.org/D141625

Added: 
    clang/test/Modules/decl-params-determinisim.m

Modified: 
    clang/lib/Parse/ParseDecl.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index e6812ac72c885..96c25ba0c853c 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -6987,6 +6987,15 @@ void Parser::ParseFunctionDeclarator(Declarator &D,
         continue;
       DeclsInPrototype.push_back(ND);
     }
+    // Sort DeclsInPrototype based on raw encoding of the source location.
+    // Scope::decls() is iterating over a SmallPtrSet so sort the Decls before
+    // moving to DeclContext. This provides a stable ordering for traversing
+    // Decls in DeclContext, which is important for tasks like ASTWriter for
+    // deterministic output.
+    llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+      return D1->getLocation().getRawEncoding() <
+             D2->getLocation().getRawEncoding();
+    });
   }
 
   // Remember that we parsed a function type, and remember the attributes.

diff  --git a/clang/test/Modules/decl-params-determinisim.m b/clang/test/Modules/decl-params-determinisim.m
new file mode 100644
index 0000000000000..d7e873bb4938e
--- /dev/null
+++ b/clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,96 @@
+/// Test determinisim when serializing anonymous decls. Create enough Decls in
+/// DeclContext that can overflow the small storage of SmallPtrSet to make sure
+/// the serialization does not rely on iteration order of SmallPtrSet.
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+/// Check the order of the decls first. If LLVM_ENABLE_REVERSE_ITERATION is on,
+/// it will fail the test early if the output is depending on the order of items
+/// in containers that has non-deterministic orders.
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm | FileCheck %s
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: 
diff  %t1.pcm %t2.pcm
+
+/// Spot check entries to make sure they are in current ordering.
+/// op13 encodes the anonymous decl number which should be in order.
+// CHECK: <TYPE_FUNCTION_PROTO
+// CHECK-NEXT: <DECL_PARM_VAR
+// CHECK-SAME: op13=2
+// CHECK-NEXT: <DECL_PARM_VAR
+// CHECK-SAME: op13=3
+// CHECK-NEXT: <DECL_PARM_VAR
+// CHECK-SAME: op13=4
+// CHECK-NEXT: <DECL_PARM_VAR
+// CHECK-SAME: op13=5
+
+/// Decl records start at 43
+// CHECK: <DECL_RECORD
+// CHECK-SAME: op13=43
+// CHECK-NEXT: <DECL_RECORD
+// CHECK-SAME: op13=44
+// CHECK-NEXT: <DECL_RECORD
+// CHECK-SAME: op13=45
+// CHECK-NEXT: <DECL_RECORD
+// CHECK-SAME: op13=46
+
+//--- headers/a.h
+void f(struct A0 *a0,
+       struct A1 *a1,
+       struct A2 *a2,
+       struct A3 *a3,
+       struct A4 *a4,
+       struct A5 *a5,
+       struct A6 *a6,
+       struct A7 *a7,
+       struct A8 *a8,
+       struct A9 *a9,
+       struct A10 *a10,
+       struct A11 *a11,
+       struct A12 *a12,
+       struct A13 *a13,
+       struct A14 *a14,
+       struct A15 *a15,
+       struct A16 *a16,
+       struct A17 *a17,
+       struct A18 *a18,
+       struct A19 *a19,
+       struct A20 *a20,
+       struct A21 *a21,
+       struct A22 *a22,
+       struct A23 *a23,
+       struct A24 *a24,
+       struct A25 *a25,
+       struct A26 *a26,
+       struct A27 *a27,
+       struct A28 *a28,
+       struct A29 *a29,
+       struct A30 *a30,
+       struct A31 *a31,
+       struct A32 *a32,
+       struct A33 *a33,
+       struct A34 *a34,
+       struct A35 *a35,
+       struct A36 *a36,
+       struct A37 *a37,
+       struct A38 *a38,
+       struct A39 *a39,
+       struct A40 *a40);
+
+
+//--- headers/module.modulemap
+
+module A {
+  header "a.h"
+}
+
+//--- main.m
+
+#import <a.h>
+