From 864054500830d708cc9f7d8a53195a95488e4e0b Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Wed, 1 Mar 2023 11:31:49 -0800 Subject: [PATCH] [clang][deps] Preserve input ordering in the full output This patch makes sure the ordering of TUs in the input CDB is preserved in the full output. Previously, the results were pushed into a global vector, potentially out-of-order and then sorted by the input file name. This is non-deterministic when the CDB contains multiple entries with the same input file. This patch fixes that by pre-allocating the output vector and writing directly to the position corresponding to the current input. This also eliminates one critical section. Reviewed By: benlangmuir Differential Revision: https://reviews.llvm.org/D145098 --- .../modules-full-output-tu-order.c | 64 ++++++++ clang/test/ClangScanDeps/modules-full.cpp | 138 +++++++++--------- clang/tools/clang-scan-deps/ClangScanDeps.cpp | 26 ++-- 3 files changed, 147 insertions(+), 81 deletions(-) create mode 100644 clang/test/ClangScanDeps/modules-full-output-tu-order.c diff --git a/clang/test/ClangScanDeps/modules-full-output-tu-order.c b/clang/test/ClangScanDeps/modules-full-output-tu-order.c new file mode 100644 index 000000000000..dd5002809daa --- /dev/null +++ b/clang/test/ClangScanDeps/modules-full-output-tu-order.c @@ -0,0 +1,64 @@ +// This test checks that ordering of TUs in the input CDB is preserved in the full output. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +//--- cdb.json.template +[ + { + "directory": "DIR", + "file": "DIR/tu.c", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -c DIR/tu.c -o DIR/tu1.o" + }, + { + "directory": "DIR", + "file": "DIR/tu.c", + "command": "clang -fmodules -fmodules-cache-path=DIR/cache -c DIR/tu.c -o DIR/tu2.o" + } +] + +//--- tu.c + +// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json +// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/result.json +// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck -DPREFIX=%/t %s + +// CHECK: { +// CHECK-NEXT: "modules": [], +// CHECK-NEXT: "translation-units": [ +// CHECK-NEXT: { +// CHECK-NEXT: "commands": [ +// CHECK-NEXT: { +// CHECK-NEXT: "clang-context-hash": "{{.*}}", +// CHECK-NEXT: "clang-module-deps": [], +// CHECK-NEXT: "command-line": [ +// CHECK: "-o", +// CHECK-NEXT: "[[PREFIX]]/tu1.o", +// CHECK: ], +// CHECK-NEXT: "executable": "clang", +// CHECK-NEXT: "file-deps": [ +// CHECK-NEXT: "[[PREFIX]]/tu.c" +// CHECK-NEXT: ], +// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c" +// CHECK-NEXT: } +// CHECK: ] +// CHECK-NEXT: }, +// CHECK-NEXT: { +// CHECK-NEXT: "commands": [ +// CHECK-NEXT: { +// CHECK-NEXT: "clang-context-hash": "{{.*}}", +// CHECK-NEXT: "clang-module-deps": [], +// CHECK-NEXT: "command-line": [ +// CHECK: "-o", +// CHECK-NEXT: "[[PREFIX]]/tu2.o", +// CHECK: ], +// CHECK-NEXT: "executable": "clang", +// CHECK-NEXT: "file-deps": [ +// CHECK-NEXT: "[[PREFIX]]/tu.c" +// CHECK-NEXT: ], +// CHECK-NEXT: "input-file": "[[PREFIX]]/tu.c" +// CHECK-NEXT: } +// CHECK: ] +// CHECK-NEXT: } +// CHECK-NEXT: ] +// CHECK-NEXT: } diff --git a/clang/test/ClangScanDeps/modules-full.cpp b/clang/test/ClangScanDeps/modules-full.cpp index a19aafddcc14..59efef0ecbaa 100644 --- a/clang/test/ClangScanDeps/modules-full.cpp +++ b/clang/test/ClangScanDeps/modules-full.cpp @@ -84,75 +84,6 @@ // CHECK-NEXT: { // CHECK-NEXT: "commands": [ // CHECK-NEXT: { -// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", -// CHECK-NEXT: "clang-module-deps": [ -// CHECK-NEXT: { -// CHECK-NEXT: "context-hash": "[[HASH_H1]]", -// CHECK-NEXT: "module-name": "header1" -// CHECK-NEXT: } -// CHECK-NEXT: ], -// CHECK-NEXT: "command-line": [ -// CHECK-NOT: "-fimplicit-modules" -// CHECK-NOT: "-fimplicit-module-maps" -// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" -// CHECK: ], -// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" -// CHECK-NEXT: "file-deps": [ -// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: ], -// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: } -// CHECK-NEXT: ] -// CHECK-NEXT: }, -// CHECK-NEXT: { -// CHECK-NEXT: "commands": [ -// CHECK-NEXT: { -// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", -// CHECK-NEXT: "clang-module-deps": [ -// CHECK-NEXT: { -// CHECK-NEXT: "context-hash": "[[HASH_H1]]", -// CHECK-NEXT: "module-name": "header1" -// CHECK-NEXT: } -// CHECK-NEXT: ], -// CHECK-NEXT: "command-line": [ -// CHECK-NOT: "-fimplicit-modules" -// CHECK-NOT: "-fimplicit-module-maps" -// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" -// CHECK: ], -// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" -// CHECK-NEXT: "file-deps": [ -// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: ], -// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: } -// CHECK-NEXT: ] -// CHECK-NEXT: }, -// CHECK-NEXT: { -// CHECK-NEXT: "commands": [ -// CHECK-NEXT: { -// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", -// CHECK-NEXT: "clang-module-deps": [ -// CHECK-NEXT: { -// CHECK-NEXT: "context-hash": "[[HASH_H1]]", -// CHECK-NEXT: "module-name": "header1" -// CHECK-NEXT: } -// CHECK-NEXT: ], -// CHECK-NEXT: "command-line": [ -// CHECK-NOT: "-fimplicit-modules" -// CHECK-NOT: "-fimplicit-module-maps" -// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" -// CHECK: ], -// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" -// CHECK-NEXT: "file-deps": [ -// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: ], -// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" -// CHECK-NEXT: } -// CHECK-NEXT: ] -// CHECK-NEXT: }, -// CHECK-NEXT: { -// CHECK-NEXT: "commands": [ -// CHECK-NEXT: { // CHECK-NEXT: "clang-context-hash": "[[HASH_TU_DINCLUDE:[A-Z0-9]+]]", // CHECK-NEXT: "clang-module-deps": [ // CHECK-NEXT: { @@ -172,6 +103,75 @@ // CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input2.cpp" // CHECK-NEXT: } // CHECK-NEXT: ] +// CHECK-NEXT: }, +// CHECK-NEXT: { +// CHECK-NEXT: "commands": [ +// CHECK-NEXT: { +// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", +// CHECK-NEXT: "clang-module-deps": [ +// CHECK-NEXT: { +// CHECK-NEXT: "context-hash": "[[HASH_H1]]", +// CHECK-NEXT: "module-name": "header1" +// CHECK-NEXT: } +// CHECK-NEXT: ], +// CHECK-NEXT: "command-line": [ +// CHECK-NOT: "-fimplicit-modules" +// CHECK-NOT: "-fimplicit-module-maps" +// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" +// CHECK: ], +// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" +// CHECK-NEXT: "file-deps": [ +// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: ], +// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: } +// CHECK-NEXT: ] +// CHECK-NEXT: }, +// CHECK-NEXT: { +// CHECK-NEXT: "commands": [ +// CHECK-NEXT: { +// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", +// CHECK-NEXT: "clang-module-deps": [ +// CHECK-NEXT: { +// CHECK-NEXT: "context-hash": "[[HASH_H1]]", +// CHECK-NEXT: "module-name": "header1" +// CHECK-NEXT: } +// CHECK-NEXT: ], +// CHECK-NEXT: "command-line": [ +// CHECK-NOT: "-fimplicit-modules" +// CHECK-NOT: "-fimplicit-module-maps" +// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" +// CHECK: ], +// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" +// CHECK-NEXT: "file-deps": [ +// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: ], +// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: } +// CHECK-NEXT: ] +// CHECK-NEXT: }, +// CHECK-NEXT: { +// CHECK-NEXT: "commands": [ +// CHECK-NEXT: { +// CHECK-NEXT: "clang-context-hash": "[[HASH_TU:[A-Z0-9]+]]", +// CHECK-NEXT: "clang-module-deps": [ +// CHECK-NEXT: { +// CHECK-NEXT: "context-hash": "[[HASH_H1]]", +// CHECK-NEXT: "module-name": "header1" +// CHECK-NEXT: } +// CHECK-NEXT: ], +// CHECK-NEXT: "command-line": [ +// CHECK-NOT: "-fimplicit-modules" +// CHECK-NOT: "-fimplicit-module-maps" +// CHECK: "-fmodule-file={{.*}}[[PREFIX]]/module-cache{{(_clangcl)?}}/[[HASH_H1]]/header1-{{[A-Z0-9]+}}.pcm" +// CHECK: ], +// CHECK-NEXT: "executable": "{{.*}}clang{{.*}}" +// CHECK-NEXT: "file-deps": [ +// CHECK-NEXT: "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: ], +// CHECK-NEXT: "input-file": "[[PREFIX]]/modules_cdb_input.cpp" +// CHECK-NEXT: } +// CHECK-NEXT: ] // CHECK-NEXT: } // CHECK-NEXT: ] // CHECK-NEXT: } diff --git a/clang/tools/clang-scan-deps/ClangScanDeps.cpp b/clang/tools/clang-scan-deps/ClangScanDeps.cpp index 2cca188e28bb..f200c8d6d8b7 100644 --- a/clang/tools/clang-scan-deps/ClangScanDeps.cpp +++ b/clang/tools/clang-scan-deps/ClangScanDeps.cpp @@ -274,6 +274,8 @@ static llvm::json::Array toJSONSorted(std::vector V) { // Thread safe. class FullDeps { public: + FullDeps(size_t NumInputs) : Inputs(NumInputs) {} + void mergeDeps(StringRef Input, TranslationUnitDeps TUDeps, size_t InputIndex) { mergeDeps(std::move(TUDeps.ModuleGraph), InputIndex); @@ -286,8 +288,9 @@ public: ID.DriverCommandLine = std::move(TUDeps.DriverCommandLine); ID.Commands = std::move(TUDeps.Commands); - std::unique_lock ul(Lock); - Inputs.push_back(std::move(ID)); + assert(InputIndex < Inputs.size() && "Input index out of bounds"); + assert(Inputs[InputIndex].FileName.empty() && "Result already populated"); + Inputs[InputIndex] = std::move(ID); } void mergeDeps(ModuleDepsGraph Graph, size_t InputIndex) { @@ -344,10 +347,6 @@ public: std::tie(B.ID.ModuleName, B.InputIndex); }); - llvm::sort(Inputs, [](const InputDeps &A, const InputDeps &B) { - return A.FileName < B.FileName; - }); - using namespace llvm::json; Array OutModules; @@ -782,11 +781,14 @@ int main(int argc, const char **argv) { AdjustingCompilations->getAllCompileCommands(); std::atomic HadErrors(false); - FullDeps FD; + std::optional FD; P1689Deps PD; std::mutex Lock; size_t Index = 0; + if (Format == ScanningOutputFormat::Full) + FD.emplace(ModuleName.empty() ? Inputs.size() : 0); + if (Verbose) { llvm::outs() << "Running clang-scan-deps on " << Inputs.size() << " files using " << Pool.getThreadCount() << " workers\n"; @@ -875,14 +877,14 @@ int main(int argc, const char **argv) { auto MaybeModuleDepsGraph = WorkerTools[I]->getModuleDependencies( *MaybeModuleName, Input->CommandLine, CWD, AlreadySeenModules, LookupOutput); - if (handleModuleResult(*MaybeModuleName, MaybeModuleDepsGraph, FD, + if (handleModuleResult(*MaybeModuleName, MaybeModuleDepsGraph, *FD, LocalIndex, DependencyOS, Errs)) HadErrors = true; } else { auto MaybeTUDeps = WorkerTools[I]->getTranslationUnitDependencies( Input->CommandLine, CWD, AlreadySeenModules, LookupOutput); - if (handleTranslationUnitResult(Filename, MaybeTUDeps, FD, LocalIndex, - DependencyOS, Errs)) + if (handleTranslationUnitResult(Filename, MaybeTUDeps, *FD, + LocalIndex, DependencyOS, Errs)) HadErrors = true; } } @@ -891,11 +893,11 @@ int main(int argc, const char **argv) { Pool.wait(); if (RoundTripArgs) - if (FD.roundTripCommands(llvm::errs())) + if (FD && FD->roundTripCommands(llvm::errs())) HadErrors = true; if (Format == ScanningOutputFormat::Full) - FD.printFullOutput(llvm::outs()); + FD->printFullOutput(llvm::outs()); else if (Format == ScanningOutputFormat::P1689) PD.printDependencies(llvm::outs());