https://github.com/naveen-seth created https://github.com/llvm/llvm-project/pull/191258
This reverts commit 87491a4c8028636bc794193004771a49cbe32a2b. The original commit created the Clang module precompile jobs as `CC1Command` objects instead of regular `Command` objects, which introduced a memory leak. This has been fixed in this reland. >From 15a9ca37a2e6bf49d6e290500715b25b961a2209 Mon Sep 17 00:00:00 2001 From: Naveen Seth Hanig <[email protected]> Date: Thu, 9 Apr 2026 19:49:23 +0200 Subject: [PATCH] Reapply "[clang][ModulesDriver] Add support for Clang modules to -fmodules-driver" (#191122) This reverts commit 87491a4c8028636bc794193004771a49cbe32a2b. The original commit created Clang module precompile jobs as CC1Command objects instead of regular Command objects, which introduced a memory leak. This has been fixed in this reland. --- clang/lib/Driver/ModulesDriver.cpp | 84 ++++++++---- .../modules-driver-clang-modules-only.cpp | 127 ++++++++++++++++++ .../modules-driver-manifest-input-args.cpp | 16 +-- 3 files changed, 191 insertions(+), 36 deletions(-) create mode 100644 clang/test/Driver/modules-driver-clang-modules-only.cpp diff --git a/clang/lib/Driver/ModulesDriver.cpp b/clang/lib/Driver/ModulesDriver.cpp index 2cd274a1f3099..783f2270c4e0f 100644 --- a/clang/lib/Driver/ModulesDriver.cpp +++ b/clang/lib/Driver/ModulesDriver.cpp @@ -25,10 +25,12 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/DepthFirstIterator.h" #include "llvm/ADT/DirectedGraph.h" +#include "llvm/ADT/PostOrderIterator.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SmallVectorExtras.h" #include "llvm/ADT/TypeSwitch.h" #include "llvm/ADT/iterator_range.h" +#include "llvm/Option/ArgList.h" #include "llvm/Support/Casting.h" #include "llvm/Support/GraphWriter.h" #include "llvm/Support/JSON.h" @@ -1221,7 +1223,7 @@ static SmallVector<JobNode *> createNodesForUnusedStdlibModuleJobs( } /// Creates a job for the Clang module described by \p MD. -static std::unique_ptr<CC1Command> +static std::unique_ptr<Command> createClangModulePrecompileJob(Compilation &C, const Command &ImportingJob, const deps::ModuleDeps &MD) { DerivedArgList &Args = C.getArgs(); @@ -1241,27 +1243,27 @@ createClangModulePrecompileJob(Compilation &C, const Command &ImportingJob, for (const auto &Arg : BuildArgs) JobArgs.push_back(TCArgs.MakeArgString(Arg)); - return std::make_unique<CC1Command>( - *PA, ImportingJob.getCreator(), ResponseFileSupport::AtFileUTF8(), - C.getDriver().getClangProgramPath(), JobArgs, - /*Inputs=*/ArrayRef<InputInfo>{}, - /*Outputs=*/ArrayRef<InputInfo>{}); + return std::make_unique<Command>(*PA, ImportingJob.getCreator(), + ResponseFileSupport::AtFileUTF8(), + C.getDriver().getClangProgramPath(), JobArgs, + /*Inputs=*/ArrayRef<InputInfo>{}, + /*Outputs=*/ArrayRef<InputInfo>{}); } -/// Creates a ClangModuleJobNode and its job for each unique Clang module -/// in \p ModuleDepGraphsForScannedJobs. +/// Creates a \c ClangModuleJobNode with associated job for each unique Clang +/// module in \p ModuleDepGraphsForScannedJobs. /// -/// Only the jobs at indices \p ScannedJobIndices in \p Jobs are expected to be -/// non-null. +/// \param ImportingJobs Jobs whose module dependencies were scanned. +/// \param ModuleDepGraphsForScannedJobs Full Clang module dependency graphs +/// corresponding to \p ImportingJobs, in order. static void createClangModuleJobsAndNodes( CompilationGraph &Graph, Compilation &C, - ArrayRef<std::unique_ptr<Command>> Jobs, ArrayRef<size_t> ScannedJobIndices, + ArrayRef<std::unique_ptr<Command>> ImportingJobs, SmallVectorImpl<deps::ModuleDepsGraph> &&ModuleDepGraphsForScannedJobs) { llvm::DenseSet<deps::ModuleID> AlreadySeen; - for (auto &&[ScanIndex, ModuleDepsGraph] : - llvm::enumerate(ModuleDepGraphsForScannedJobs)) { - const auto &ImportingJob = *Jobs[ScannedJobIndices[ScanIndex]]; - + for (auto &&[ImportingJob, ModuleDepsGraph] : + llvm::zip_equal(llvm::make_pointee_range(ImportingJobs), + ModuleDepGraphsForScannedJobs)) { for (auto &MD : ModuleDepsGraph) { const auto Inserted = AlreadySeen.insert(MD.ID).second; if (!Inserted) @@ -1274,6 +1276,31 @@ static void createClangModuleJobsAndNodes( } } +/// Installs the command lines produced by the dependency scan into +/// \p ScannedJobs. +static void +installScanCommandLines(Compilation &C, + MutableArrayRef<std::unique_ptr<Command>> ScannedJobs, + ArrayRef<InputDependencies> InputDepsForScannedJobs) { + for (auto &&[Job, InputDeps] : llvm::zip_equal( + llvm::make_pointee_range(ScannedJobs), InputDepsForScannedJobs)) { + const auto &BuildArgs = InputDeps.BuildArgs; + ArgStringList JobArgs; + JobArgs.reserve(BuildArgs.size()); + + const auto &SourceAction = Job.getSource(); + const auto &TC = Job.getCreator().getToolChain(); + auto &TCArgs = + C.getArgsForToolChain(&TC, SourceAction.getOffloadingArch(), + SourceAction.getOffloadingDeviceKind()); + + for (const auto &Arg : BuildArgs) + JobArgs.push_back(TCArgs.MakeArgString(Arg)); + + Job.replaceArguments(std::move(JobArgs)); + } +} + /// Creates nodes for all jobs which were scanned for dependencies. /// /// The updated command lines produced by the dependency scan are installed at a @@ -1506,16 +1533,17 @@ void driver::modules::runModulesDriver( Graph, takeJobsAtIndices(Jobs, ScanResult.NonScannableJobIndices)); auto UnusedStdlibModuleJobNodes = createNodesForUnusedStdlibModuleJobs( Graph, takeJobsAtIndices(Jobs, ScanResult.UnusedStdlibModuleJobIndices)); + + auto ScannedJobs = takeJobsAtIndices(Jobs, ScanResult.ScannedJobIndices); createClangModuleJobsAndNodes( - Graph, C, Jobs, ScanResult.ScannedJobIndices, + Graph, C, /*ImportingJobs*/ ScannedJobs, std::move(ScanResult.ModuleDepGraphsForScannedJobs)); - createNodesForScannedJobs( - Graph, takeJobsAtIndices(Jobs, ScanResult.ScannedJobIndices), - std::move(ScanResult.InputDepsForScannedJobs)); - createRegularEdges(Graph); + installScanCommandLines(C, ScannedJobs, ScanResult.InputDepsForScannedJobs); + createNodesForScannedJobs(Graph, std::move(ScannedJobs), + std::move(ScanResult.InputDepsForScannedJobs)); + createRegularEdges(Graph); pruneUnusedStdlibModuleJobs(Graph, UnusedStdlibModuleJobNodes); - if (!createModuleDependencyEdges(Graph, Diags)) return; createAndConnectRoot(Graph); @@ -1524,12 +1552,14 @@ void driver::modules::runModulesDriver( if (!Diags.isLastDiagnosticIgnored()) llvm::WriteGraph<const CompilationGraph *>(llvm::errs(), &Graph); - // TODO: Install all updated command-lines produced by the dependency scan. // TODO: Fix-up command-lines for named module imports. - // TODO: Sort the graph topologically before adding jobs back to the - // Compilation being built. - for (auto *N : Graph) - if (auto *JN = dyn_cast<JobNode>(N)) - C.addCommand(std::move(JN->Job)); + llvm::ReversePostOrderTraversal<CompilationGraph *> TopologicallySortedNodes( + &Graph); + assert(isa<RootNode>(*TopologicallySortedNodes.begin()) && + "First node in topological order must be the root!"); + auto TopologicallySortedJobNodes = llvm::map_range( + llvm::drop_begin(TopologicallySortedNodes), llvm::CastTo<JobNode>); + for (auto *JN : TopologicallySortedJobNodes) + C.addCommand(std::move(JN->Job)); } diff --git a/clang/test/Driver/modules-driver-clang-modules-only.cpp b/clang/test/Driver/modules-driver-clang-modules-only.cpp new file mode 100644 index 0000000000000..37a803f9b6fc0 --- /dev/null +++ b/clang/test/Driver/modules-driver-clang-modules-only.cpp @@ -0,0 +1,127 @@ +// Checks that -fmodules-driver correctly handles compilations using Clang modules. + +// RUN: split-file %s %t +// RUN: rm -rf %t/modules-cache + +// RUN: %clang -std=c++23 \ +// RUN: -fmodules-driver -Rmodules-driver \ +// RUN: -fmodules -Rmodule-import \ +// RUN: -fmodule-map-file=%t/module.modulemap \ +// RUN: -fmodules-cache-path=%t/modules-cache \ +// RUN: -fsyntax-only %t/main.cpp 2>&1 \ +// RUN: | sed 's:\\\\\?:/:g' \ +// RUN: | FileCheck -DPREFIX=%/t --check-prefix=CHECK-REMARKS %s + +// RUN: rm -rf %t/modules-cache +// RUN: %clang -std=c++23 \ +// RUN: -fmodules-driver \ +// RUN: -fmodules \ +// RUN: -fmodule-map-file=%t/module.modulemap \ +// RUN: -fmodules-cache-path=%t/modules-cache \ +// RUN: -fsyntax-only %t/main.cpp \ +// RUN: -### 2>&1 \ +// RUN: | sed 's:\\\\\?:/:g' \ +// RUN: | FileCheck --check-prefix=CHECK-CC1 %s + +// The scan itself will also produce [-Rmodule-import] remarks. +// Let's skip past them, we only care about the final -cc1 commands. +// CHECK-REMARKS: clang: remark: printing module dependency graph [-Rmodules-driver] +// CHECK-REMARKS-NEXT: digraph "Module Dependency Graph" { +// CHECK-REMARKS: } + +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'root' from +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'direct1' into 'root' +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'transitive1' into 'direct1' +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'transitive2' into 'direct1' +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'direct2' into 'root' +// CHECK-REMARKS: [[PREFIX]]/main.cpp:1:2: remark: importing module 'transitive2' into 'direct2' + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-o" "[[TRANSITIVE2PCM:[^"]+]]" +// CHECK-CC1-SAME: "-emit-module" +// CHECK-CC1-SAME: "-fmodule-name=transitive2" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-o" "[[DIRECT2PCM:[^"]+]]" +// CHECK-CC1-SAME: "-emit-module" +// CHECK-CC1-SAME: "-fmodule-file=transitive2=[[TRANSITIVE2PCM]]" +// CHECK-CC1-SAME: "-fmodule-name=direct2" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-o" "[[TRANSITIVE1PCM:[^"]+]]" +// CHECK-CC1-SAME: "-emit-module" +// CHECK-CC1-SAME: "-fmodule-name=transitive1" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-o" "[[DIRECT1PCM:[^"]+]]" +// CHECK-CC1-SAME: "-emit-module" +// CHECK-CC1-SAME: "-fmodule-file=transitive1=[[TRANSITIVE1PCM]]" +// CHECK-CC1-SAME: "-fmodule-file=transitive2=[[TRANSITIVE2PCM]]" +// CHECK-CC1-SAME: "-fmodule-name=direct1" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-o" "[[ROOTPCM:[^"]+]]" +// CHECK-CC1-SAME: "-emit-module" +// CHECK-CC1-SAME: "-fmodule-file=direct1=[[DIRECT1PCM]]" +// CHECK-CC1-SAME: "-fmodule-file=direct2=[[DIRECT2PCM]]" +// CHECK-CC1-SAME: "-fmodule-name=root" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// CHECK-CC1: "-cc1" +// CHECK-CC1-SAME: "-fsyntax-only" +// CHECK-CC1-SAME: "{{.*}}/main.cpp" +// CHECK-CC1-SAME: "-fmodule-file=root=[[ROOTPCM]]" +// CHECK-CC1-SAME: "-fno-implicit-modules" + +// (Because of missing include guards, this example would also run into +// redefinition errors when compiling without modules.) + +/--- module.modulemap +module root { header "root.h"} +module transitive1 { header "transitive1.h" } +module transitive2 { header "transitive2.h" } +module direct1 { header "direct1.h" } +module direct2 { header "direct2.h" } + +//--- root.h +#include "direct1.h" +#include "direct2.h" +int fromRoot() { + return fromDirect1() + fromDirect2(); +} + +//--- direct1.h +#include "transitive1.h" +#include "transitive2.h" + +int fromDirect1() { + return fromTransitive1() + fromTransitive2(); +} + +//--- direct2.h +#include "transitive2.h" + +int fromDirect2() { + return fromTransitive2() + 2; +} + +//--- transitive1.h +int fromTransitive1() { + return 20; +} + +//--- transitive2.h +int fromTransitive2() { + return 10; +} + +//--- main.cpp +#include "root.h" + +int main() { + fromRoot(); +} diff --git a/clang/test/Driver/modules-driver-manifest-input-args.cpp b/clang/test/Driver/modules-driver-manifest-input-args.cpp index 99765a1943faf..726fe03e27840 100644 --- a/clang/test/Driver/modules-driver-manifest-input-args.cpp +++ b/clang/test/Driver/modules-driver-manifest-input-args.cpp @@ -26,17 +26,15 @@ // RUN: %t/main.cpp \ // RUN: -### 2>&1 \ // RUN: | sed 's:\\\\\?:/:g' \ -// RUN: | FileCheck %s -check-prefix=MAIN-CC1 -check-prefix=STDLIB-MOD-CC1 -DPREFIX=%/t +// RUN: | FileCheck %s -DPREFIX=%/t -// MAIN-CC1: "-cc1" -// MAIN-CC1-SAME: "main.cpp" -// MAIN-CC1-NOT: "-Wno-reserved-module-identifier" -// MAIN-CC1-NOT: "-internal-isystem" "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/" +// CHECK: "-cc1" {{.*}} "-Wno-reserved-module-identifier" {{.*}} "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/std.cppm" {{.*}} "-internal-isystem" "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/" -// STDLIB-MOD-CC1: "-cc1" -// STDLIB-MOD-CC1-SAME: "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/std.cppm" -// STDLIB-MOD-CC1-SAME: "-Wno-reserved-module-identifier" -// STDLIB-MOD-CC1-SAME: "-internal-isystem" "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/" +// The adjustments should only be made for inputs from the Standard Library module manifest: +// Check that the -cc1 command line does not contain those adjustments! +// CHECK: "-cc1" {{.*}}main +// CHECK-NOT: "-Wno-reserved-module-identifier" +// CHECK-NOT: "-internal-isystem" "[[PREFIX]]/Inputs/usr/lib/x86_64-linux-gnu/../share/libc++/v1/" //--- main.cpp import std; _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
