-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang][modules-driver] Add dependency scan and dependency graph #152770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-driver Author: Naveen Seth Hanig (naveen-seth) ChangesThis patch is part of a series to support driver-managed module builds. The dependency scan and graph support both Clang modules and C++ named modules. This patch follows the discussion in the RFC linked below and links
RFC discussing linking the driver against additional libraries: RFC for driver-managed module builds: Patch is 66.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/152770.diff 13 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticDriverKinds.td b/clang/include/clang/Basic/DiagnosticDriverKinds.td
index 0f17f4aa761ea..68532ba58a03b 100644
--- a/clang/include/clang/Basic/DiagnosticDriverKinds.td
+++ b/clang/include/clang/Basic/DiagnosticDriverKinds.td
@@ -581,6 +581,18 @@ def err_drv_reduced_module_output_overrided : Warning<
"please consider use '-fmodule-output=' to specify the output file for reduced BMI explicitly">,
InGroup<DiagGroup<"reduced-bmi-output-overrided">>;
+def remark_found_cxx20_module_usage : Remark<
+ "found C++20 module usage in file '%0'">,
+ InGroup<ModulesDriver>;
+def remark_performing_driver_managed_module_build : Remark<
+ "performing driver managed module build">,
+ InGroup<ModulesDriver>;
+def err_failed_dependency_scan : Error <
+ "failed to perform dependency scan">;
+def remark_module_dependency_graph : Remark<
+ "printing module dependency graph">,
+ InGroup<ModulesDriver>;
+
def warn_drv_delayed_template_parsing_after_cxx20 : Warning<
"-fdelayed-template-parsing is deprecated after C++20">,
InGroup<DiagGroup<"delayed-template-parsing-in-cxx20">>;
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index ccb18aa37447e..78726ecc869db 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -628,6 +628,7 @@ def ModuleConflict : DiagGroup<"module-conflict">;
def ModuleFileExtension : DiagGroup<"module-file-extension">;
def ModuleIncludeDirectiveTranslation : DiagGroup<"module-include-translation">;
def ModuleMap : DiagGroup<"module-map">;
+def ModulesDriver : DiagGroup<"modules-driver">;
def RoundTripCC1Args : DiagGroup<"round-trip-cc1-args">;
def NewlineEOF : DiagGroup<"newline-eof">;
def Nullability : DiagGroup<"nullability">;
diff --git a/clang/include/clang/Driver/DependencyScanner.h b/clang/include/clang/Driver/DependencyScanner.h
new file mode 100644
index 0000000000000..a2d56aba462ca
--- /dev/null
+++ b/clang/include/clang/Driver/DependencyScanner.h
@@ -0,0 +1,388 @@
+#ifndef LLVM_CLANG_DRIVER_DEPENDENCYSCANNER_H
+#define LLVM_CLANG_DRIVER_DEPENDENCYSCANNER_H
+
+#include "clang/Driver/Driver.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/DirectedGraph.h"
+#include "llvm/Support/DOTGraphTraits.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/GraphWriter.h"
+
+namespace clang {
+class SourceManager;
+class CharSourceRange;
+class DiagnosticsEngine;
+} // namespace clang
+
+namespace llvm::opt {
+class DerivedArgList;
+} // namespace llvm::opt
+
+namespace clang {
+namespace driver {
+namespace dependencies {
+
+using clang::tooling::dependencies::TranslationUnitDeps;
+
+//===----------------------------------------------------------------------===//
+// Dependency Scan
+//===----------------------------------------------------------------------===//
+
+class DependencyScanError : public llvm::ErrorInfo<DependencyScanError> {
+public:
+ static char ID;
+
+ void log(llvm::raw_ostream &OS) const override {
+ OS << "error while performing dependency scan\n";
+ }
+
+ std::error_code convertToErrorCode() const override {
+ return llvm::errc::not_supported;
+ }
+};
+
+/// Performs a full dependency scan for the given driver command line and
+/// returns all scan results or an error on scanning failure.
+///
+/// \param ClangProgramPath Path to the clang executable
+/// \param Diags The calling driver's diagnostics engine
+/// \param Args The calling driver's command line arguments
+///
+/// \returns The scan results for all inputs, or an error if scanning fails
+llvm::Expected<SmallVector<TranslationUnitDeps, 0>>
+scanModuleDependencies(llvm::StringRef ClangProgramPath,
+ clang::DiagnosticsEngine &Diags,
+ const llvm::opt::DerivedArgList &Args);
+
+//===----------------------------------------------------------------------===//
+// Module Dependency Graph
+//===----------------------------------------------------------------------===//
+
+class MDGNode;
+class MDGEdge;
+using MDGNodeBase = llvm::DGNode<MDGNode, MDGEdge>;
+using MDGEdgeBase = llvm::DGEdge<MDGNode, MDGEdge>;
+using MDGBase = llvm::DirectedGraph<MDGNode, MDGEdge>;
+
+/// Base class for module dependency graph nodes.
+///
+/// Represents a node in the ModuleDepGraph, which can be a translation unit
+/// which doesn't provide any module, a Clang module, or a C++ named module.
+class MDGNode : public MDGNodeBase {
+public:
+ enum class NodeKind {
+ ClangModule,
+ NonModuleTU,
+ CXXNamedModule,
+ };
+
+ using Command = tooling::dependencies::Command;
+
+ MDGNode(const NodeKind K) : Kind(K) {}
+ MDGNode(const NodeKind K, std::vector<Command> Commands)
+ : Kind(K), Commands(Commands) {}
+
+ virtual ~MDGNode() = 0;
+
+ NodeKind getKind() const { return Kind; }
+
+ ArrayRef<Command> getCommands() const { return Commands; }
+
+private:
+ const NodeKind Kind;
+
+protected:
+ std::vector<Command> Commands;
+};
+
+/// ClangModuleNode - represents a Clang module in the ModuleDepGraph.
+class ClangModuleNode : public MDGNode {
+public:
+ ClangModuleNode(StringRef ModuleName)
+ : MDGNode(NodeKind::ClangModule), ModuleName(ModuleName) {}
+ ~ClangModuleNode() = default;
+
+ static bool classof(const MDGNode *N) {
+ return N->getKind() == NodeKind::ClangModule;
+ }
+
+ StringRef getModuleName() const { return ModuleName; }
+
+ void setCommands(std::vector<tooling::dependencies::Command> Commands) {
+ this->Commands = Commands;
+ }
+
+private:
+ std::string ModuleName;
+};
+
+/// NonModuleTUNode - represents a regular TU which doesn't provide any module,
+/// in the ModuleDepGraph.
+class NonModuleTUNode : public MDGNode {
+public:
+ NonModuleTUNode(StringRef InputFile)
+ : MDGNode(NodeKind::NonModuleTU), InputFile(InputFile) {}
+ ~NonModuleTUNode() override = default;
+
+ static bool classof(const MDGNode *N) {
+ return N->getKind() == NodeKind::NonModuleTU;
+ }
+
+ StringRef getInputFile() const { return InputFile; }
+
+private:
+ const std::string InputFile;
+};
+
+/// CXXNamedModuleNode - represents a C++ named module node in the
+/// ModuleDepGraph.
+///
+/// Unresolved nodes are those discovered as imports but missing a module
+/// definition.
+class CXXNamedModuleNode : public MDGNode {
+public:
+ CXXNamedModuleNode(StringRef ModuleName, StringRef InputFile = "")
+ : MDGNode(NodeKind::CXXNamedModule), InputFile(InputFile),
+ ModuleName(ModuleName) {}
+ ~CXXNamedModuleNode() = default;
+
+ static bool classof(const MDGNode *N) {
+ return N->getKind() == NodeKind::CXXNamedModule;
+ }
+
+ StringRef getInputFile() const { return InputFile; }
+
+ StringRef getModuleName() const { return ModuleName; }
+
+ bool isUnresolved() const { return InputFile.empty(); }
+
+ void setInputFile(StringRef FilePath) { this->InputFile = FilePath; }
+
+ void setCommands(std::vector<tooling::dependencies::Command> Commands) {
+ this->Commands = Commands;
+ }
+
+private:
+ std::string InputFile;
+ std::string ModuleName;
+};
+
+/// MDGEdge - represents an import relationship, directed from the importing
+/// unit to the imported unit.
+class MDGEdge : public MDGEdgeBase {
+public:
+ MDGEdge() = delete;
+ MDGEdge(MDGNode &N) : MDGEdgeBase(N) {}
+};
+
+class ModuleDepGraphBuilder;
+
+/// ModuleDepGraph - A directed graph that represents dependency relationships
+/// from dependency scan results, with ownership of its nodes and edges.
+class ModuleDepGraph : public MDGBase {
+ friend ModuleDepGraphBuilder;
+
+ template <typename NodeTy, typename... Args>
+ NodeTy *MakeWithBumpAlloc(Args &&...args);
+
+ llvm::BumpPtrAllocator BumpPtrAlloc;
+};
+
+/// Fully constructs a ModuleDepGraph from the dependency scan results.
+///
+/// \param ScanResults The list of scan results.
+/// \param Inputs The calling drivers list of input list in its original order.
+/// \param Path to the clang executable.
+ModuleDepGraph
+buildModuleDepGraph(SmallVectorImpl<TranslationUnitDeps> &&ScanResults,
+ clang::driver::Driver::InputList Inputs,
+ StringRef ClangProgramPath);
+
+} // namespace dependencies
+} // namespace driver
+} // namespace clang
+
+//===----------------------------------------------------------------------===//
+// Module Dependency Graph: GraphTraits specialization
+//===----------------------------------------------------------------------===//
+
+namespace llvm {
+/// non-const versions of the GraphTrait specializations for MDG
+template <> struct GraphTraits<clang::driver::dependencies::MDGNode *> {
+ using NodeRef = clang::driver::dependencies::MDGNode *;
+
+ static NodeRef MDGGetTargetNode(clang::driver::dependencies::MDGEdgeBase *P) {
+ return &P->getTargetNode();
+ }
+
+ // Provide a mapped iterator so that the GraphTrait-based implementations can
+ // find the target nodes without having to explicitly go through the edges.
+ using ChildIteratorType =
+ mapped_iterator<clang::driver::dependencies::MDGNode::iterator,
+ decltype(&MDGGetTargetNode)>;
+ using ChildEdgeIteratorType = clang::driver::dependencies::MDGNode::iterator;
+
+ static NodeRef getEntryNode(NodeRef N) { return N; }
+ static ChildIteratorType child_begin(NodeRef N) {
+ return ChildIteratorType(N->begin(), &MDGGetTargetNode);
+ }
+ static ChildIteratorType child_end(NodeRef N) {
+ return ChildIteratorType(N->end(), &MDGGetTargetNode);
+ }
+
+ static ChildEdgeIteratorType child_edge_begin(NodeRef N) {
+ return N->begin();
+ }
+ static ChildEdgeIteratorType child_edge_end(NodeRef N) { return N->end(); }
+};
+
+template <>
+struct GraphTraits<clang::driver::dependencies::ModuleDepGraph *>
+ : public GraphTraits<clang::driver::dependencies::MDGNode *> {
+ using nodes_iterator = clang::driver::dependencies::ModuleDepGraph::iterator;
+ static NodeRef getEntryNode(clang::driver::dependencies::ModuleDepGraph *G) {
+ return *G->begin();
+ }
+ static nodes_iterator
+ nodes_begin(clang::driver::dependencies::ModuleDepGraph *G) {
+ return G->begin();
+ }
+ static nodes_iterator
+ nodes_end(clang::driver::dependencies::ModuleDepGraph *G) {
+ return G->end();
+ }
+};
+
+/// const versions of the GraphTrait specializations for MDG
+template <> struct GraphTraits<const clang::driver::dependencies::MDGNode *> {
+ using NodeRef = const clang::driver::dependencies::MDGNode *;
+
+ static NodeRef
+ MDGGetTargetNode(const clang::driver::dependencies::MDGEdgeBase *P) {
+ return &P->getTargetNode();
+ }
+
+ using ChildIteratorType =
+ mapped_iterator<clang::driver::dependencies::MDGNode::const_iterator,
+ decltype(&MDGGetTargetNode)>;
+ using ChildEdgeIteratorType =
+ clang::driver::dependencies::MDGNode::const_iterator;
+
+ static NodeRef getEntryNode(NodeRef N) { return N; }
+ static ChildIteratorType child_begin(NodeRef N) {
+ return ChildIteratorType(N->begin(), &MDGGetTargetNode);
+ }
+ static ChildIteratorType child_end(NodeRef N) {
+ return ChildIteratorType(N->end(), &MDGGetTargetNode);
+ }
+
+ static ChildEdgeIteratorType child_edge_begin(NodeRef N) {
+ return N->begin();
+ }
+ static ChildEdgeIteratorType child_edge_end(NodeRef N) { return N->end(); }
+};
+
+template <>
+struct GraphTraits<const clang::driver::dependencies::ModuleDepGraph *>
+ : public GraphTraits<const clang::driver::dependencies::MDGNode *> {
+ using nodes_iterator =
+ clang::driver::dependencies::ModuleDepGraph::const_iterator;
+
+ static NodeRef
+ getEntryNode(const clang::driver::dependencies::ModuleDepGraph *G) {
+ return *G->begin();
+ }
+ static nodes_iterator
+ nodes_begin(const clang::driver::dependencies::ModuleDepGraph *G) {
+ return G->begin();
+ }
+ static nodes_iterator
+ nodes_end(const clang::driver::dependencies::ModuleDepGraph *G) {
+ return G->end();
+ }
+};
+
+//===----------------------------------------------------------------------===//
+// Module Dependency Graph: DOTGraphTraits & GraphWriter specializations
+//===----------------------------------------------------------------------===//
+
+template <>
+struct DOTGraphTraits<const clang::driver::dependencies::ModuleDepGraph *>
+ : public DefaultDOTGraphTraits {
+ DOTGraphTraits(bool isSimple = false) : DefaultDOTGraphTraits(isSimple) {}
+
+ static StringRef
+ getGraphName(const clang::driver::dependencies::ModuleDepGraph *MDG) {
+ return "Module Dependency Graph";
+ }
+
+ static StringRef
+ getNodeKindLabel(const clang::driver::dependencies::NonModuleTUNode *N) {
+ return "Non-Module TU";
+ }
+
+ static StringRef
+ getNodeKindLabel(const clang::driver::dependencies::ClangModuleNode *N) {
+ return "Clang Module";
+ }
+
+ static StringRef
+ getNodeKindLabel(const clang::driver::dependencies::CXXNamedModuleNode *N) {
+ return "C++ Named Module";
+ }
+
+ static std::string
+ getNodeIdentifierLabel(const clang::driver::dependencies::MDGNode *N,
+ const clang::driver::dependencies::ModuleDepGraph *G) {
+ using namespace clang::driver::dependencies;
+ if (const auto *ClangModule = dyn_cast<ClangModuleNode>(N))
+ return (Twine(getNodeKindLabel(ClangModule)) + " '" +
+ ClangModule->getModuleName() + "'")
+ .str();
+ if (const auto *CXXNamedModule = dyn_cast<CXXNamedModuleNode>(N))
+ return (Twine(getNodeKindLabel(CXXNamedModule)) + " '" +
+ CXXNamedModule->getModuleName() + "'")
+ .str();
+ if (const auto *NonModuleTU = dyn_cast<NonModuleTUNode>(N))
+ return (Twine(getNodeKindLabel(NonModuleTU)) + " '" +
+ NonModuleTU->getInputFile() + "'")
+ .str();
+ llvm_unreachable("Unhandled MDGNode kind!");
+ }
+
+ static std::string
+ getGraphProperties(const clang::driver::dependencies::ModuleDepGraph *G) {
+ return "\tnode [shape=Mrecord];\n\tedge [dir=\"back\"];\n";
+ }
+};
+
+template <>
+class GraphWriter<const clang::driver::dependencies::ModuleDepGraph *>
+ : public GraphWriterBase<
+ const clang::driver::dependencies::ModuleDepGraph *,
+ GraphWriter<const clang::driver::dependencies::ModuleDepGraph *>> {
+public:
+ using GraphType = const clang::driver::dependencies::ModuleDepGraph *;
+ using Base = GraphWriterBase<GraphType, GraphWriter<GraphType>>;
+
+ GraphWriter(raw_ostream &o, const GraphType &g, bool SN) : Base(o, g, SN) {}
+
+ void writeNodes();
+
+private:
+ using Base::DOTTraits;
+ using Base::GTraits;
+ using Base::NodeRef;
+
+ void writeNodeDeclarations(ArrayRef<NodeRef> Nodes);
+ void writeNodeDeclaration(NodeRef Node);
+ void writeNodeRelations(ArrayRef<NodeRef> Nodes);
+ void writeNodeRelation(NodeRef Node);
+
+ DenseMap<NodeRef, std::string> NodeIDLabels;
+};
+
+} // namespace llvm
+
+#endif
diff --git a/clang/include/clang/Driver/Driver.h b/clang/include/clang/Driver/Driver.h
index 4d32552b7f85f..b9b187ada8add 100644
--- a/clang/include/clang/Driver/Driver.h
+++ b/clang/include/clang/Driver/Driver.h
@@ -512,6 +512,9 @@ class Driver {
/// BuildActions - Construct the list of actions to perform for the
/// given arguments, which are only done for a single architecture.
+ /// If the compilation is an explicit module build, delegates to
+ /// BuildDriverManagedModuleBuildActions. Otherwise, BuildDefaultActions is
+ /// used.
///
/// \param C - The compilation that is being built.
/// \param Args - The input arguments.
@@ -796,6 +799,35 @@ class Driver {
/// compilation based on which -f(no-)?lto(=.*)? option occurs last.
void setLTOMode(const llvm::opt::ArgList &Args);
+ /// BuildDefaultActions - Constructs the list of actions to perform
+ /// for the provided arguments, which are only done for a single architecture.
+ ///
+ /// \param C - The compilation that is being built.
+ /// \param Args - The input arguments.
+ /// \param Actions - The list to store the resulting actions onto.
+ void BuildDefaultActions(Compilation &C, llvm::opt::DerivedArgList &Args,
+ const InputList &Inputs, ActionList &Actions) const;
+
+ /// BuildDriverManagedModuleBuildActions - Performs a dependency
+ /// scan and constructs the list of actions to perform for dependency order
+ /// and the provided arguments. This is only done for a single a architecture.
+ ///
+ /// \param C - The compilation that is being built.
+ /// \param Args - The input arguments.
+ /// \param Actions - The list to store the resulting actions onto.
+ void BuildDriverManagedModuleBuildActions(Compilation &C,
+ llvm::opt::DerivedArgList &Args,
+ const InputList &Inputs,
+ ActionList &Actions) const;
+
+ /// Scans the leading lines of the C++ source inputs to detect C++20 module
+ /// usage.
+ ///
+ /// \returns True if module usage is detected, false otherwise, or an error on
+ /// read failure.
+ llvm::ErrorOr<bool>
+ ScanInputsForCXX20ModulesUsage(const InputList &Inputs) const;
+
/// Retrieves a ToolChain for a particular \p Target triple.
///
/// Will cache ToolChains for the life of the driver object, and create them
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 6aab43c9ed57f..ae8ddb0c2de88 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -3296,6 +3296,13 @@ defm modules_reduced_bmi : BoolOption<"f", "modules-reduced-bmi",
PosFlag<SetTrue, [], [ClangOption, CC1Option],
"Generate the reduced BMI">>;
+def fmodules_driver : Flag<["-"], "fmodules-driver">,
+ Group<f_Group>, Visibility<[ClangOption]>,
+ HelpText<"Enable support for driver managed module builds (experimental)">;
+def fno_modules_driver : Flag<["-"], "fno-modules-driver">,
+ Group<f_Group>, Visibility<[ClangOption]>,
+ HelpText<"Disable support for driver managed module builds (experimental)">;
+
def experimental_modules_reduced_bmi : Flag<["-"], "fexperimental-modules-reduced-bmi">,
Group<f_Group>, Visibility<[ClangOption, CC1Option]>, Alias<fmodules_reduced_bmi>;
diff --git a/clang/include/clang/Lex/DependencyDirectivesScanner.h b/clang/include/clang/Lex/DependencyDirectivesScanner.h
index f9fec3998ca53..c0b742d652a03 100644
--- a/clang/include/clang/Lex/DependencyDirectivesScanner.h
+++ b/clang/include/clang/Lex/DependencyDirectivesScanner.h
@@ -135,6 +135,13 @@ void printDependencyDirectivesAsSource(
ArrayRef<dependency_directives_scan::Directive> Directives,
llvm::raw_ostream &OS);
+/// Scan an input source buffer for C++20 named module usage.
+///
+/// \param Source The input source buffer.
+///
+/// \returns true if any C++20 named modules related directive was found.
+bool scanInputForCXX20ModulesUsage(StringRef Source);
+
/// Functor that returns the dependency directives for a given file.
class DependencyDirectivesGetter {
public:
diff --git a/clang/lib/Driver/CMakeLists.txt b/clang/lib/Driver/CMakeLists.txt
index 45782cbd9d16d..4d6375633c17a 100644
--- a/clang/lib/Driver/CMakeLists.txt
+++ b/clang/lib/Driver/CMakeLists.txt
@@ -17,6 +17,7 @@ endif()
add_clang_library(clangDriver
Action.cpp
Compilation.cpp
+ DependencyScanner.cpp
Distro.cpp
Driver.cpp
DriverOptions.cpp
@@ -98,5 +99,7 @@ add_clang_library(clangDriver
LINK_LIBS
clangBasic
+ clangDependencyScanning
+ clangLex
${system_libs}
)
diff --git a/clang/lib/Driver/DependencyScanner.cpp b/clang/lib/Driver/DependencyScanner.cpp
new file mode 100644
index 0000000000000..3b2dc7af67bcb
--- /dev/null
+++ b/clang/lib/Driver/DependencyScanner.cpp
@@ -0,0 +1,696 @@
+#include "clang/Driver/DependencyScanner.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Driver/Compilation.h"
+#include "clang/Driver/Driver.h"
+#include "clang/Driver/Tool.h"
+#include "clang/Frontend/TextDiagnosticPrinter.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/ADT/STLExtras.h"
+#inclu...
[truncated]
|
d5b66ab
to
0c6d9ba
Compare
0c6d9ba
to
905e78d
Compare
82ccdec
to
f9dad8d
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
4ebde16
to
3ddbf65
Compare
3ddbf65
to
a348caa
Compare
This patch is part of a series to support driver-managed module builds. It adds support for the discovery of module dependencies from within the driver and for generation of the module dependency graph. Source inputs provided by the command-line can import modules defined in the standard library module manifest, which are then scanned on demand. The dependency scan and graph support both Clang modules and C++ named modules. The generated dependency graph can be output in Graphviz DOT format as a remark.
a348caa
to
8e126e9
Compare
5e44a59
to
d78e844
Compare
d78e844
to
b0993e5
Compare
Inputs.emplace_back(types::TY_CXXModule, A); | ||
} | ||
|
||
void buildStdModuleManifestInputs(const StdModuleManifest &Manifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, this allows for importing any module defined in the standard module manifest, even if the manifest contains is-std-library: false
for that module entry.
Should we constrain this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the status quo of distributed different std manifest files ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libc++ and libstdc++ only distribute the standard modules (marked with is-std-library: true
), and no other modules. Allowing additional system modules beyond the standard library modules to be scanned on demand should almost never cause any issues though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, if all of the standard libraries distribute the file with is-std-library: true
now, let's constrain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. (And also thank you for reviewing!)
/// Identical to Driver::InputList. | ||
using InputList = llvm::SmallVector<InputTy, 16>; | ||
|
||
/// Checks whether the -fmodules-driver feature should be implicitly enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid enabling it implicitly? I fear there will be more problems than helps. It is not complex to add -fmodules-driver
for users. And after all, if we find this is really useful in practice, we can add the feature (enable it implicitly in different conditions) in another PR. It won't be late and hard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the logic to implicitly enable the feature is only used for diagnostics but doesn't actually do anything.
This was already introduced in #153497 and is just being relocated in this patch. Since this patch introduces the ModulesDriver.cpp
file, I thought it made sense to also move as much modules-driver–related code there as possible.
While I was moving the code, the comment documenting this was cut in half. It is fixed now:
llvm-project/clang/lib/Driver/Driver.cpp
Lines 1838 to 1841 in cb0ac58
// TODO: When -fmodules-driver is no longer experimental, allow implicit | |
// activation of the modules driver. For now, keep the detection of whether | |
// the modules driver should be enabled here for diagnostics only, and do | |
// not implicitly enable the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to not get it in the first place. But the implicitly enabled feature is still concerning to me. I feel it will be easy to add them later. Can we not the implicitly enable now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, you also want to revert #153497 for now?
clang/lib/Driver/Driver.cpp
Outdated
// pruned later. | ||
const auto StdModuleManifestPath = | ||
GetStdModuleManifestPath(*C, C->getDefaultToolChain()); | ||
if (StdModuleManifestPath == "<NOT PRESENT>") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe we can avoid hardcoding by judging this by if it is a path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
clang/lib/Driver/ModulesDriver.cpp
Outdated
static bool hasCXXNamedModuleInput(const InputList &Inputs) { | ||
const auto IsTypeCXXModule = [](const auto &Input) -> bool { | ||
const auto TypeID = Input.first; | ||
return (TypeID == types::TY_CXXModule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we judge preprocessed CXX module for TY_PP_CXXModule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Inputs.emplace_back(types::TY_CXXModule, A); | ||
} | ||
|
||
void buildStdModuleManifestInputs(const StdModuleManifest &Manifest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the status quo of distributed different std manifest files ?
FixIts.emplace_back(SrcMgr, LangOpts, FixIt); | ||
} | ||
|
||
/// Translates \c StandaloneDiag into a StoredDiagnostic, associating it with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain why we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier this was only documented at the StandaloneDiagnostic
declaration:
llvm-project/clang/lib/Driver/ModulesDriver.cpp
Lines 263 to 265 in 0e8bee7
/// To report the diagnostic, it must first be translated back into a | |
/// StoredDiagnostic with a new associated SourceManager. | |
struct StandaloneDiagnostic { |
I've now also added a comment to translateStandaloneDiag
.
clang/lib/Driver/ModulesDriver.cpp
Outdated
|
||
private: | ||
/// We need the output of dependency scan to be deterministic. However, | ||
/// the dependency graph may contain two modules with the same name. How |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it is an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a merged graph from multiple inputs, multiple modules with the same name is not an error. When doing multi-arch builds, or offloading you need different PCMs for each target arch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in named modules, this need to be an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can happen with named modules too, although the current code probably only works for the multi-arch case.
While the current code doesn't support it, I would like to eventually support:
clang++ file.cpp file.mm module.cppm -o program
Where both file.cpp and file.mm import module.cppm. This requires two different PCMs and one .o file for the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in named modules, this need to be an error
For named modules, we currently signal an error here, while building the graph:
https://github.com/naveen-seth/llvm-project/blob/f1d92657d1d47bd1f436546d823bc75757e1443a/clang/lib/Driver/ModulesDriver.cpp#L1043-L1046
clang/lib/Driver/ModulesDriver.cpp
Outdated
SmallVector<std::unique_ptr<ModuleDeps>> takeOrderedModuleDeps(); | ||
|
||
private: | ||
/// We need the output of dependency scan to be deterministic. However, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read correctly, let's add a FIXME:
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn’t mean this as a FIXME, but just to explain why we are making the extra effort to sort by (module name, index of the first importing input) instead of (module name, module id).
The comment was just copied from ClangScanDeps.cpp
, where the same sorting is done (but slightly different, since there we store the final results in a sorted vector rather than a map):
/// We need the output of clang-scan-deps to be deterministic. However, |
Happy to adjust this if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW it might be worth simplifying this code like so: jansvoboda11@11b88811 instead of replicating this hack elsewhere. I think it changes the ordering of checks in our tests - maybe we can do some extra post-processing to keep it the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is a lot better and have updated the code.
Is it important that we perform this outside of the main thread (#65691)?
llvm-project/clang/tools/clang-scan-deps/ClangScanDeps.cpp
Lines 433 to 437 in 12a5854
// First call to \c getBuildArguments is somewhat expensive. Let's call it | |
// on the current thread (instead of the main one), and outside the | |
// critical section. | |
for (ModuleDeps *MD : NewMDs) | |
(void)MD->getBuildArguments(); |
If this is important, I can revert this last change (2d87451)
BTW, if this is not too late, I feel this work might be worth to be in other clang module (maybe in supporting) than driver (and let driver to link it). The idea is, the tools needs to understand modules, but it is complex. You can find similar works in clangd's ModulesBuilder. It is too expensive for every tool (and clang-tidy or whatever else) to do the same thing. It will be good if clang can provide such ability for it. And I think your work is a good candidate. In one word, I hope this to be an ability of clang to allow clang based tools to understand modules (of course, including the clang driver itself) instead of a feature of clang driver only. Previously I didn't pay a lot attention since I feel this may not be too useful. I won't call clang directly except for toys. But if we are able to make this to be a foundation for all clang based tools to understand modules. This will be pretty valuable. |
This was something I was considering during the initial proposal, but I didn't suggest doing it that way because I don't think we know what that API should look like yet, and there are still open questions about how to handle 3rd party modules and header units. I think the exercise of making all this work in the constrained environment of the Clang driver will help answer what the eventual API should be. With that, there are definitely parts of ModuleDriver.cpp that could be moved to the DependencyScanning library, and this library will soon move out of tooling into its own top level library. This did make me notice an issue with this patch is that it likely will break shared library builds because DependencyScanning currently depends on Driver, and this makes Driver depend on DependencyScanning. I think this should be fixed by just making DependencyScanning not depend on the driver (I'll expand on that in another comment). I think a few parts of ModuleDriver.cpp should land in DependencyScanning in this patch, and after the modules driver is working well see how best to combine that with what clangd and other tools need, but until then I'd like the modules driver to be flexible as we figure things out, moving shared things to DependencyScanning as needed. Longer term I've been wondering if something like how the new Swift driver works would be interesting for Clang. The Swift driver can be directly used by build systems to figure out all the work needed to build arbitrary sets of code, including any Clang modules that are needed. It can also use its internal build system to build directly. This would be a big change for Clang in that it takes on a lot of build system responsibilities, but it has worked really well for Swift. |
LINK_LIBS | ||
clangBasic | ||
clangLex | ||
clangDependencyScanning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my previous comment, this causes a cycle because clangDependencyScanning depends on clangDriver. This is fine when statically linking, but will fail with dynamic linking which we support.
This is only used by DependencyScanningWorker::scanDependencies
to handle driver commands. To fix this I think it would be best to remove this dependency and instead have scanDependencies
only work on frontend commands. I would then add a function to clangDriver
that takes a DependencyScanningWorker
and runs the scanner on all of frontend jobs, this would then be used to replace calls to scanDependencies
.
This will need to be handled before (or maybe as part of) this PR lands to avoid build issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change would likely conflict with changing scanDependecnies
, so I'd do it after that lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#161300 has landed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to land this as a separate PR first; I think the only reusable code for this might be isJobForDependencyScan
.
Apologies for the delay, I haven’t been able to work on this during the week, but I’ll be back to it by Friday at the earliest or Monday at the latest.
This patch is part of a series to support driver-managed module builds.
It adds support for discovery of module dependencies from within the driver and for generation of the module dependency graph.
Source inputs provided on the command line can import modules defined in the standard module manifest, which are then scanned on demand.
The dependency scan and graph support both Clang modules and C++ named modules. The generated dependency graph can be output in Graphviz DOT format as a remark.
RFC discussing linking the driver against additional libraries:
https://discourse.llvm.org/t/rfc-driver-link-the-driver-against-clangdependencyscanning-clangast-clangfrontend-clangserialization-and-clanglex
RFC for driver-managed module builds:
https://discourse.llvm.org/t/rfc-modules-support-simple-c-20-modules-use-from-the-clang-driver-without-a-build-system