-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Dependency Scanning] Teach DependencyScanningTool::getModuleDependencies to Process a List of Module Names
#129915
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 2 commits
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 |
|---|---|---|
|
|
@@ -1213,11 +1213,17 @@ void GetDependenciesByModuleNameAction::ExecuteAction() { | |
| Preprocessor &PP = CI.getPreprocessor(); | ||
| SourceManager &SM = PP.getSourceManager(); | ||
| FileID MainFileID = SM.getMainFileID(); | ||
| SourceLocation FileStart = SM.getLocForStartOfFile(MainFileID); | ||
| SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path; | ||
| IdentifierInfo *ModuleID = PP.getIdentifierInfo(ModuleName); | ||
| Path.push_back(std::make_pair(ModuleID, FileStart)); | ||
| auto ModResult = CI.loadModule(FileStart, Path, Module::Hidden, false); | ||
| PPCallbacks *CB = PP.getPPCallbacks(); | ||
| CB->moduleImport(SourceLocation(), Path, ModResult); | ||
| SourceLocation SLoc = SM.getLocForStartOfFile(MainFileID); | ||
| for (auto ModuleName : ModuleNames) { | ||
| SmallVector<std::pair<IdentifierInfo *, SourceLocation>, 2> Path; | ||
| IdentifierInfo *ModuleID = PP.getIdentifierInfo(ModuleName); | ||
| Path.push_back(std::make_pair(ModuleID, SLoc)); | ||
| auto ModResult = CI.loadModule(SLoc, Path, Module::Hidden, false); | ||
| PPCallbacks *CB = PP.getPPCallbacks(); | ||
| CB->moduleImport(SourceLocation(), Path, ModResult); | ||
| // FIXME: how do you know that this offset is correct? | ||
| SLoc = SLoc.getLocWithOffset(1); | ||
|
Contributor
Author
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 am not quite sure setting an offset of 1 is correct. Will need to take a closer look.
Member
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 used it as an incrementer as we need to allocate one source location for each module to import. |
||
| assert(SLoc <= SM.getLocForEndOfFile(MainFileID) && | ||
| "Import location extends past file"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -287,10 +287,11 @@ class DependencyScanningAction : public tooling::ToolAction { | |
| DependencyScanningService &Service, StringRef WorkingDirectory, | ||
| DependencyConsumer &Consumer, DependencyActionController &Controller, | ||
| llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS, | ||
| bool DisableFree, std::optional<StringRef> ModuleName = std::nullopt) | ||
| bool DisableFree, | ||
| std::optional<ArrayRef<StringRef>> ModuleNames = std::nullopt) | ||
| : Service(Service), WorkingDirectory(WorkingDirectory), | ||
| Consumer(Consumer), Controller(Controller), DepFS(std::move(DepFS)), | ||
| DisableFree(DisableFree), ModuleName(ModuleName) {} | ||
| DisableFree(DisableFree), ModuleNames(ModuleNames) {} | ||
|
|
||
| bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation, | ||
| FileManager *DriverFileMgr, | ||
|
|
@@ -431,8 +432,9 @@ class DependencyScanningAction : public tooling::ToolAction { | |
|
|
||
| if (Service.getFormat() == ScanningOutputFormat::P1689) | ||
| Action = std::make_unique<PreprocessOnlyAction>(); | ||
| else if (ModuleName) | ||
| Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName); | ||
| else if (ModuleNames) | ||
| Action = | ||
| std::make_unique<GetDependenciesByModuleNameAction>(*ModuleNames); | ||
| else | ||
| Action = std::make_unique<ReadPCHAndPreprocessAction>(); | ||
|
|
||
|
|
@@ -478,7 +480,7 @@ class DependencyScanningAction : public tooling::ToolAction { | |
| DependencyActionController &Controller; | ||
| llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> DepFS; | ||
| bool DisableFree; | ||
| std::optional<StringRef> ModuleName; | ||
| std::optional<ArrayRef<StringRef>> ModuleNames; | ||
| std::optional<CompilerInstance> ScanInstanceStorage; | ||
| std::shared_ptr<ModuleDepCollector> MDC; | ||
| std::vector<std::string> LastCC1Arguments; | ||
|
|
@@ -546,7 +548,7 @@ llvm::Error DependencyScanningWorker::computeDependencies( | |
| llvm::Error DependencyScanningWorker::computeDependencies( | ||
| StringRef WorkingDirectory, const std::vector<std::string> &CommandLine, | ||
| DependencyConsumer &Consumer, DependencyActionController &Controller, | ||
| StringRef ModuleName) { | ||
| ArrayRef<StringRef> ModuleNames) { | ||
| // Capture the emitted diagnostics and report them to the client | ||
| // in the case of a failure. | ||
| std::string DiagnosticOutput; | ||
|
|
@@ -555,7 +557,7 @@ llvm::Error DependencyScanningWorker::computeDependencies( | |
| TextDiagnosticPrinter DiagPrinter(DiagnosticsOS, DiagOpts.release()); | ||
|
|
||
| if (computeDependencies(WorkingDirectory, CommandLine, Consumer, Controller, | ||
| DiagPrinter, ModuleName)) | ||
| DiagPrinter, ModuleNames)) | ||
| return llvm::Error::success(); | ||
| return llvm::make_error<llvm::StringError>(DiagnosticsOS.str(), | ||
| llvm::inconvertibleErrorCode()); | ||
|
|
@@ -625,7 +627,7 @@ bool DependencyScanningWorker::scanDependencies( | |
| StringRef WorkingDirectory, const std::vector<std::string> &CommandLine, | ||
| DependencyConsumer &Consumer, DependencyActionController &Controller, | ||
| DiagnosticConsumer &DC, llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS, | ||
| std::optional<StringRef> ModuleName) { | ||
| std::optional<ArrayRef<StringRef>> ModuleNames) { | ||
| auto FileMgr = | ||
| llvm::makeIntrusiveRefCnt<FileManager>(FileSystemOptions{}, FS); | ||
|
|
||
|
|
@@ -648,7 +650,7 @@ bool DependencyScanningWorker::scanDependencies( | |
| // always true for a driver invocation. | ||
| bool DisableFree = true; | ||
| DependencyScanningAction Action(Service, WorkingDirectory, Consumer, | ||
| Controller, DepFS, DisableFree, ModuleName); | ||
| Controller, DepFS, DisableFree, ModuleNames); | ||
|
|
||
| bool Success = false; | ||
| if (CommandLine[1] == "-cc1") { | ||
|
|
@@ -729,13 +731,14 @@ bool DependencyScanningWorker::computeDependencies( | |
| auto &FinalFS = ModifiedFS ? ModifiedFS : BaseFS; | ||
|
|
||
| return scanDependencies(WorkingDirectory, FinalCommandLine, Consumer, | ||
| Controller, DC, FinalFS, /*ModuleName=*/std::nullopt); | ||
| Controller, DC, FinalFS, | ||
| /*ModuleNames=*/std::nullopt); | ||
| } | ||
|
|
||
| bool DependencyScanningWorker::computeDependencies( | ||
| StringRef WorkingDirectory, const std::vector<std::string> &CommandLine, | ||
| DependencyConsumer &Consumer, DependencyActionController &Controller, | ||
| DiagnosticConsumer &DC, StringRef ModuleName) { | ||
| DiagnosticConsumer &DC, ArrayRef<StringRef> ModuleNames) { | ||
| // Reset what might have been modified in the previous worker invocation. | ||
| BaseFS->setCurrentWorkingDirectory(WorkingDirectory); | ||
|
|
||
|
|
@@ -748,17 +751,30 @@ bool DependencyScanningWorker::computeDependencies( | |
| InMemoryFS->setCurrentWorkingDirectory(WorkingDirectory); | ||
| SmallString<128> FakeInputPath; | ||
| // TODO: We should retry the creation if the path already exists. | ||
| llvm::sys::fs::createUniquePath(ModuleName + "-%%%%%%%%.input", FakeInputPath, | ||
| // FIXME: Using ModuleNames[0] should be sufficient to create a unique | ||
| // input file name. The diagnostic information is specifc enough about | ||
| // in which exact module an error occurs. That said any error reporting | ||
| // that relies on the path below could be confusing.We may want to | ||
| // find better ways (e.g. by concatenating the module names) to make | ||
| // the temporary file name more precise. | ||
| llvm::sys::fs::createUniquePath(ModuleNames[0] + "-%%%%%%%%.input", | ||
|
Contributor
Author
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. What is the reason that we still need to create this file in memory? Should I create a "fake" file for each module in the list?
Member
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. Someone should fact-check me, but I'm pretty sure compiler invocations always need a file to run an action over.
Contributor
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. Yeah, you always need a main file id. I'm not actually sure why we need to create a unique path on disk though. I would think the in memory file is enough.
Member
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. IIUC this shouldn't actually be writing any file to disk, just a unique name to create the in memory file. |
||
| FakeInputPath, | ||
| /*MakeAbsolute=*/false); | ||
| InMemoryFS->addFile(FakeInputPath, 0, llvm::MemoryBuffer::getMemBuffer("")); | ||
|
|
||
| // The fake file must contain at least ModuleNames.size() characters, | ||
| // since we are simulating lexing it, and we are assuming each module name | ||
| // takes the space of one character. | ||
| std::string FakeString(ModuleNames.size(), ' '); | ||
| InMemoryFS->addFile(FakeInputPath, 0, | ||
| llvm::MemoryBuffer::getMemBuffer(FakeString)); | ||
| llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> InMemoryOverlay = InMemoryFS; | ||
|
|
||
| OverlayFS->pushOverlay(InMemoryOverlay); | ||
| auto ModifiedCommandLine = CommandLine; | ||
| ModifiedCommandLine.emplace_back(FakeInputPath); | ||
|
|
||
| return scanDependencies(WorkingDirectory, ModifiedCommandLine, Consumer, | ||
| Controller, DC, OverlayFS, ModuleName); | ||
| Controller, DC, OverlayFS, ModuleNames); | ||
| } | ||
|
|
||
| DependencyActionController::~DependencyActionController() {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,7 +203,7 @@ static void ParseArgs(int argc, char **argv) { | |
| if (const llvm::opt::Arg *A = Args.getLastArg(OPT_compilation_database_EQ)) | ||
| CompilationDB = A->getValue(); | ||
|
|
||
| if (const llvm::opt::Arg *A = Args.getLastArg(OPT_module_name_EQ)) | ||
| if (const llvm::opt::Arg *A = Args.getLastArg(OPT_module_names_EQ)) | ||
| ModuleName = A->getValue(); | ||
|
|
||
| for (const llvm::opt::Arg *A : Args.filtered(OPT_dependency_target_EQ)) | ||
|
|
@@ -1024,11 +1024,17 @@ int clang_scan_deps_main(int argc, char **argv, const llvm::ToolContext &) { | |
| HadErrors = true; | ||
| } | ||
| } else if (ModuleName) { | ||
| StringRef NamesRef(*ModuleName); | ||
| SmallVector<StringRef, 16> NameList; | ||
| NamesRef.split(NameList, ","); | ||
| auto MaybeModuleDepsGraph = WorkerTool.getModuleDependencies( | ||
| *ModuleName, Input->CommandLine, CWD, AlreadySeenModules, | ||
| LookupOutput); | ||
| if (handleModuleResult(*ModuleName, MaybeModuleDepsGraph, *FD, | ||
| LocalIndex, DependencyOS, Errs)) | ||
| NameList, Input->CommandLine, CWD, | ||
| AlreadySeenModules, LookupOutput); | ||
| // FIXME: Need to have better error handling logic for a list | ||
| // of modules. Probably through some call back. | ||
|
Comment on lines
+1034
to
+1035
Contributor
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 don't think you need a callback. This should just drop the module name from the diag, as that should get printed by the normal Clang errors. |
||
| if (handleModuleResult(/* *ModuleName*/ NameList[0], | ||
| MaybeModuleDepsGraph, *FD, LocalIndex, | ||
| DependencyOS, Errs)) | ||
| HadErrors = true; | ||
| } else { | ||
| std::unique_ptr<llvm::MemoryBuffer> TU; | ||
|
|
||
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 the type here is just
ModuleLoadResult. The general rule forautoin LLVM is "Use auto if and only if it makes the code more readable or easier to maintain."