[PATCH] D143027: [clang][deps] Fix module context hash for constant strings
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
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
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
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