-
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?
Changes from all commits
8e126e9
b0993e5
0e8bee7
dd7f47c
cb0ac58
4b67e5f
1882993
6251cea
2d87451
f1d9265
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
//===- ModulesDriver.h - Driver managed module builds --------*- C++ -*-===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
/// | ||
/// \file | ||
/// This file defines functionality to support driver managed builds for | ||
/// compilations which use Clang modules or standard C++20 named modules. | ||
/// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef LLVM_CLANG_DRIVER_MODULESDRIVER_H | ||
#define LLVM_CLANG_DRIVER_MODULESDRIVER_H | ||
|
||
#include "clang/Driver/Types.h" | ||
#include "llvm/Support/Error.h" | ||
|
||
namespace llvm { | ||
namespace vfs { | ||
class FileSystem; | ||
} // namespace vfs | ||
namespace opt { | ||
class Arg; | ||
} // namespace opt | ||
} // namespace llvm | ||
|
||
namespace clang { | ||
class DiagnosticsEngine; | ||
namespace driver { | ||
class Compilation; | ||
} // namespace driver | ||
} // namespace clang | ||
|
||
namespace clang::driver::modules { | ||
|
||
/// A list of inputs and their types for the given arguments. | ||
/// Identical to Driver::InputTy. | ||
using InputTy = std::pair<types::ID, const llvm::opt::Arg *>; | ||
|
||
/// A list of inputs and their types for the given arguments. | ||
/// Identical to Driver::InputList. | ||
using InputList = llvm::SmallVector<InputTy, 16>; | ||
|
||
/// Checks whether the -fmodules-driver feature should be implicitly enabled. | ||
/// | ||
/// The modules driver should be enabled if both: | ||
/// 1) the compilation has more than two C++ source inputs; and | ||
/// 2) any C++ source inputs uses C++20 named modules. | ||
/// | ||
/// \param Inputs The input list for the compilation being built. | ||
/// \param VFS The virtual file system to use for all reads. | ||
/// \param Diags The diagnostics engine used for emitting remarks only. | ||
/// | ||
/// \returns True if the modules driver should be enabled, false otherwise, | ||
/// or a llvm::FileError on failure to read a source input. | ||
llvm::Expected<bool> shouldUseModulesDriver(const InputList &Inputs, | ||
llvm::vfs::FileSystem &VFS, | ||
DiagnosticsEngine &Diags); | ||
|
||
/// The parsed Standard library module manifest. | ||
struct StdModuleManifest { | ||
struct LocalModuleArgs { | ||
std::vector<std::string> SystemIncludeDirs; | ||
}; | ||
|
||
struct Module { | ||
bool IsStdlib = false; | ||
std::string LogicalName; | ||
std::string SourcePath; | ||
std::optional<LocalModuleArgs> LocalArgs; | ||
}; | ||
|
||
std::vector<Module> ModuleEntries; | ||
}; | ||
|
||
/// Reads the Standard library module manifest from the specified path. | ||
/// | ||
/// All source file paths in the returned manifest are made absolute. | ||
/// | ||
/// \param StdModuleManifestPath Path to the manifest file. | ||
/// \param VFS The llvm::vfs::FileSystem to be used for all file accesses. | ||
/// | ||
/// \returns The parsed manifest on success, a llvm::FileError on failure to | ||
/// read the manifest, or a llvm::json::ParseError on failure to parse it. | ||
llvm::Expected<StdModuleManifest> | ||
readStdModuleManifest(llvm::StringRef StdModuleManifestPath, | ||
llvm::vfs::FileSystem &VFS); | ||
|
||
/// Constructs compilation inputs for each module listed in the provided | ||
/// Standard library module manifest. | ||
/// | ||
/// \param Manifest The standard modules manifest | ||
/// \param C The compilation being built. | ||
/// \param Inputs The input list to which the corresponding input entries are | ||
/// appended. | ||
void buildStdModuleManifestInputs(const StdModuleManifest &Manifest, | ||
Compilation &C, InputList &Inputs); | ||
|
||
/// Reorders and builds compilation jobs based on discovered module | ||
/// dependencies. | ||
/// | ||
/// \param C The compilation being built. | ||
/// \param Manifest The standard modules manifest | ||
void planDriverManagedModuleCompilation(Compilation &C, | ||
const StdModuleManifest &Manifest); | ||
|
||
} // namespace clang::driver::modules | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ add_clang_library(clangDriver | |
Driver.cpp | ||
DriverOptions.cpp | ||
Job.cpp | ||
ModulesDriver.cpp | ||
Multilib.cpp | ||
MultilibBuilder.cpp | ||
OffloadBundler.cpp | ||
|
@@ -99,5 +100,6 @@ add_clang_library(clangDriver | |
LINK_LIBS | ||
clangBasic | ||
clangLex | ||
clangDependencyScanning | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe reason will be displayed to describe this comment to others. Learn more. This change would likely conflict with changing There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 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. |
||
${system_libs} | ||
) |
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.Uh oh!
There was an error while loading. Please reload this page.
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
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?