[PATCH] D143027: [clang][deps] Fix module context hash for constant strings

2023-01-31 Thread Ben Langmuir via Phabricator via cfe-commits 
benlangmuir created this revision.
benlangmuir added reviewers: jansvoboda11, Bigcheese.
Herald added a project: All.
benlangmuir requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

We were not hashing constant strings in the command-line, only ones that
required allocations. This was causing us to get the same hash across
different flag options.

rdar://101053855


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143027

Files:
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
  clang/test/ClangScanDeps/modules-context-hash.c


Index: clang/test/ClangScanDeps/modules-context-hash.c
===
--- clang/test/ClangScanDeps/modules-context-hash.c
+++ clang/test/ClangScanDeps/modules-context-hash.c
@@ -7,15 +7,18 @@
 
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_a.json.template > 
%t/cdb_a.json
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_b.json.template > 
%t/cdb_b.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_b2.json.template 
> %t/cdb_b2.json
 
-// We run two separate scans. The context hash for "a" and "b" can differ 
between
+// We run separate scans. The context hash for "a" and "b" can differ between
 // systems. If we'd scan both Clang invocations in a single run, the order of 
JSON
 // entities would be non-deterministic. To prevent this, run the scans 
separately
 // and verify that the context hashes differ with a single FileCheck 
invocation.
 //
-// RUN: clang-scan-deps -compilation-database %t/cdb_a.json -format 
experimental-full -j 1 >  %t/result.json
-// RUN: clang-scan-deps -compilation-database %t/cdb_b.json -format 
experimental-full -j 1 >> %t/result.json
-// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t 
-check-prefix=CHECK
+// RUN: clang-scan-deps -compilation-database %t/cdb_a.json -format 
experimental-full -j 1 >  %t/result_a.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_b.json -format 
experimental-full -j 1 > %t/result_b.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_b2.json -format 
experimental-full -j 1 > %t/result_b2.json
+// RUN: cat %t/result_a.json %t/result_b.json | sed 's:\?:/:g' | FileCheck 
%s -DPREFIX=%/t -check-prefix=CHECK
+// RUN: cat %t/result_b.json %t/result_b2.json | sed 's:\?:/:g' | 
FileCheck %s -DPREFIX=%/t -check-prefix=FLAG_ONLY
 
 // CHECK:  {
 // CHECK-NEXT:   "modules": [
@@ -91,3 +94,17 @@
 // CHECK-NEXT:   ],
 // CHECK-NEXT:   "input-file": "[[PREFIX]]/tu.c"
 // CHECK-NEXT: }
+
+// B and B2 only differ by -fapplication-extension
+
+// FLAG_ONLY:   "modules": [
+// FLAG_ONLY-NEXT: {
+// FLAG_ONLY:"context-hash": "[[HASH_MOD_B1:.*]]"
+// FLAG_ONLY-NOT:"-fapplication-extension"
+
+// FLAG_ONLY:   "modules": [
+// FLAG_ONLY-NEXT: {
+// FLAG_ONLY-NOT:"context-hash": "[[HASH_MOD_B1]]"
+// FLAG_ONLY:"-fapplication-extension"
+// FLAG_ONLY:   "translation-units": [
+// FLAG_ONLY-NOT:"context-hash": "[[HASH_MOD_B1]]"
Index: clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
@@ -0,0 +1,7 @@
+[
+  {
+"directory": "DIR",
+"command": "clang -c DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache 
-IDIR/b -o DIR/tu_b.o -fapplication-extension",
+"file": "DIR/tu.c"
+  }
+]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -264,13 +264,13 @@
   HashBuilder.add(serialization::VERSION_MAJOR, serialization::VERSION_MINOR);
 
   // Hash the BuildInvocation without any input files.
-  SmallVector DummyArgs;
-  CI.generateCC1CommandLine(DummyArgs, [&](const Twine &Arg) {
-Scratch.clear();
-StringRef Str = Arg.toStringRef(Scratch);
-HashBuilder.add(Str);
-return "";
+  SmallVector Args;
+  llvm::BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  CI.generateCC1CommandLine(Args, [&](const Twine &Arg) {
+return Saver.save(Arg).data();
   });
+  HashBuilder.addRange(Args);
 
   // Hash the module dependencies. These paths may differ even if the 
invocation
   // is identical if they depend on the contents of the files in the TU -- for


Index: clang/test/ClangScanDeps/modules-context-hash.c
===
--- clang/test/ClangScanDeps/modules-context-hash.c
+++ clang/test/ClangScanDeps/modules-context-hash.c
@@ -7,15 +7,18 @@
 
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_a.

[PATCH] D143027: [clang][deps] Fix module context hash for constant strings

2023-01-31 Thread Jan Svoboda via Phabricator via cfe-commits 
jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143027/new/

https://reviews.llvm.org/D143027

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143027: [clang][deps] Fix module context hash for constant strings

2023-01-31 Thread Michael Spencer via Phabricator via cfe-commits 
Bigcheese accepted this revision.
Bigcheese added a comment.

Thanks for fixing this. LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143027/new/

https://reviews.llvm.org/D143027

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143027: [clang][deps] Fix module context hash for constant strings

2023-02-03 Thread Ben Langmuir via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG223e99fb698d: [clang][deps] Fix module context hash for 
constant strings (authored by benlangmuir).

Changed prior to commit:
  https://reviews.llvm.org/D143027?vs=493764&id=494752#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143027/new/

https://reviews.llvm.org/D143027

Files:
  clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
  clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
  clang/test/ClangScanDeps/modules-context-hash.c


Index: clang/test/ClangScanDeps/modules-context-hash.c
===
--- clang/test/ClangScanDeps/modules-context-hash.c
+++ clang/test/ClangScanDeps/modules-context-hash.c
@@ -7,15 +7,18 @@
 
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_a.json.template > 
%t/cdb_a.json
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_b.json.template > 
%t/cdb_b.json
+// RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_b2.json.template 
> %t/cdb_b2.json
 
-// We run two separate scans. The context hash for "a" and "b" can differ 
between
+// We run separate scans. The context hash for "a" and "b" can differ between
 // systems. If we'd scan both Clang invocations in a single run, the order of 
JSON
 // entities would be non-deterministic. To prevent this, run the scans 
separately
 // and verify that the context hashes differ with a single FileCheck 
invocation.
 //
-// RUN: clang-scan-deps -compilation-database %t/cdb_a.json -format 
experimental-full -j 1 >  %t/result.json
-// RUN: clang-scan-deps -compilation-database %t/cdb_b.json -format 
experimental-full -j 1 >> %t/result.json
-// RUN: cat %t/result.json | sed 's:\?:/:g' | FileCheck %s -DPREFIX=%/t 
-check-prefix=CHECK
+// RUN: clang-scan-deps -compilation-database %t/cdb_a.json -format 
experimental-full -j 1 >  %t/result_a.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_b.json -format 
experimental-full -j 1 > %t/result_b.json
+// RUN: clang-scan-deps -compilation-database %t/cdb_b2.json -format 
experimental-full -j 1 > %t/result_b2.json
+// RUN: cat %t/result_a.json %t/result_b.json | sed 's:\?:/:g' | FileCheck 
%s -DPREFIX=%/t -check-prefix=CHECK
+// RUN: cat %t/result_b.json %t/result_b2.json | sed 's:\?:/:g' | 
FileCheck %s -DPREFIX=%/t -check-prefix=FLAG_ONLY
 
 // CHECK:  {
 // CHECK-NEXT:   "modules": [
@@ -91,3 +94,17 @@
 // CHECK-NEXT:   ],
 // CHECK-NEXT:   "input-file": "[[PREFIX]]/tu.c"
 // CHECK-NEXT: }
+
+// B and B2 only differ by -fapplication-extension
+
+// FLAG_ONLY:   "modules": [
+// FLAG_ONLY-NEXT: {
+// FLAG_ONLY:"context-hash": "[[HASH_MOD_B1:.*]]"
+// FLAG_ONLY-NOT:"-fapplication-extension"
+
+// FLAG_ONLY:   "modules": [
+// FLAG_ONLY-NEXT: {
+// FLAG_ONLY-NOT:"context-hash": "[[HASH_MOD_B1]]"
+// FLAG_ONLY:"-fapplication-extension"
+// FLAG_ONLY:   "translation-units": [
+// FLAG_ONLY-NOT:"context-hash": "[[HASH_MOD_B1]]"
Index: clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/modules-context-hash/cdb_b2.json.template
@@ -0,0 +1,7 @@
+[
+  {
+"directory": "DIR",
+"command": "clang -c DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache 
-IDIR/b -o DIR/tu_b.o -fapplication-extension",
+"file": "DIR/tu.c"
+  }
+]
Index: clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
===
--- clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -267,13 +267,12 @@
   HashBuilder.add(serialization::VERSION_MAJOR, serialization::VERSION_MINOR);
 
   // Hash the BuildInvocation without any input files.
-  SmallVector DummyArgs;
-  CI.generateCC1CommandLine(DummyArgs, [&](const Twine &Arg) {
-Scratch.clear();
-StringRef Str = Arg.toStringRef(Scratch);
-HashBuilder.add(Str);
-return "";
-  });
+  SmallVector Args;
+  llvm::BumpPtrAllocator Alloc;
+  llvm::StringSaver Saver(Alloc);
+  CI.generateCC1CommandLine(
+  Args, [&](const Twine &Arg) { return Saver.save(Arg).data(); });
+  HashBuilder.addRange(Args);
 
   // Hash the module dependencies. These paths may differ even if the 
invocation
   // is identical if they depend on the contents of the files in the TU -- for


Index: clang/test/ClangScanDeps/modules-context-hash.c
===
--- clang/test/ClangScanDeps/modules-context-hash.c
+++ clang/test/ClangScanDeps/modules-context-hash.c
@@ -7,15 +7,18 @@
 
 // RUN: sed "s|DIR|%/t|g" %S/Inputs/modules-context-hash/cdb_a.json.template