Skip to content
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

[clang-scan-deps] [P1689] Keep consistent behavior for make dependencies with clang #69551

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

ChuanqiXu9
Copy link
Member

Close #69439.

This patch tries to reuse the codes to generate make style dependencies information with P1689 format directly.

@ChuanqiXu9 ChuanqiXu9 self-assigned this Oct 19, 2023
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 19, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 19, 2023

@llvm/pr-subscribers-clang

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #69439.

This patch tries to reuse the codes to generate make style dependencies information with P1689 format directly.


Full diff: https://github.com/llvm/llvm-project/pull/69551.diff

3 Files Affected:

  • (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp (+6-2)
  • (modified) clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp (+9-3)
  • (modified) clang/test/ClangScanDeps/P1689.cppm (+10)
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 29df0c3a0afdb5c..b7911f9844a3554 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -231,12 +231,16 @@ class DependencyScanningAction : public tooling::ToolAction {
     Opts->IncludeSystemHeaders = true;
 
     switch (Format) {
+    case ScanningOutputFormat::P1689:
     case ScanningOutputFormat::Make:
       ScanInstance.addDependencyCollector(
           std::make_shared<DependencyConsumerForwarder>(
               std::move(Opts), WorkingDirectory, Consumer));
-      break;
-    case ScanningOutputFormat::P1689:
+
+      if (Format == ScanningOutputFormat::Make)
+        break;
+
+      [[fallthrough]];
     case ScanningOutputFormat::Full:
       MDC = std::make_shared<ModuleDepCollector>(
           std::move(Opts), ScanInstance, Consumer, Controller,
diff --git a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
index 40115b7b5ae25b3..ea42923f9545d97 100644
--- a/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
+++ b/clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp
@@ -436,7 +436,10 @@ void ModuleDepCollectorPP::EndOfMainFile() {
   for (const Module *M : MDC.DirectModularDeps)
     handleTopLevelModule(M);
 
-  MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
+  // With P1689 format, we get the output options from
+  // DependencyConsumerForwarder.
+  if (!MDC.IsStdModuleP1689Format)
+    MDC.Consumer.handleDependencyOutputOpts(*MDC.Opts);
 
   if (MDC.IsStdModuleP1689Format)
     MDC.Consumer.handleProvidedAndRequiredStdCXXModules(
@@ -452,8 +455,11 @@ void ModuleDepCollectorPP::EndOfMainFile() {
       MDC.Consumer.handleDirectModuleDependency(MDC.ModularDeps[M]->ID);
   }
 
-  for (auto &&I : MDC.FileDeps)
-    MDC.Consumer.handleFileDependency(I);
+  // With P1689 format, we collect the file dependencies from make's dep
+  // collector.
+  if (!MDC.IsStdModuleP1689Format)
+    for (auto &&I : MDC.FileDeps)
+      MDC.Consumer.handleFileDependency(I);
 
   for (auto &&I : MDC.DirectPrebuiltModularDeps)
     MDC.Consumer.handlePrebuiltModuleDependency(I.second);
diff --git a/clang/test/ClangScanDeps/P1689.cppm b/clang/test/ClangScanDeps/P1689.cppm
index dffb16974a3e4e4..b992502a5179c15 100644
--- a/clang/test/ClangScanDeps/P1689.cppm
+++ b/clang/test/ClangScanDeps/P1689.cppm
@@ -42,6 +42,14 @@
 // RUN: clang-scan-deps -format=p1689 \
 // RUN:   -- %clang++ -std=c++20 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -c %t/impl_part.cppm -o %t/impl_part.o \
 // RUN:   | FileCheck %t/impl_part.cppm -DPREFIX=%/t
+//
+// Check the path in the make style dependencies are generated in relative path form
+// RUN: cd %t
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t impl_part.cppm -o impl_part.o \
+// RUN:      -MT impl_part.o.ddi -MD -MF impl_part.dep
+// RUN:   cat impl_part.dep | FileCheck impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE-RELATIVE
+
 
 //--- P1689.json.in
 [
@@ -168,6 +176,8 @@ void World() {
 // CHECK-MAKE:   [[PREFIX]]/impl_part.cppm
 // CHECK-MAKE:   [[PREFIX]]/header.mock
 
+// CHECK-MAKE-RELATIVE: impl_part.o.ddi: ./impl_part.cppm ./header.mock
+
 //--- interface_part.cppm
 export module M:interface_part;
 export void World();

Copy link
Collaborator

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue with MDC's FileDeps that we are calling makeAbsoluteAndPreferred on the paths? Maybe we could instead move that call into FullDependencyConsumer. Or are there other issues?

The fact we need to add additional MDC.IsStdModuleP1689Format checks in this PR to prevent the normal consumer callbacks is not great. This makes MDC aware of implementation details of the specific output format.

@ChuanqiXu9
Copy link
Member Author

ChuanqiXu9 commented Oct 20, 2023

Is the issue with MDC's FileDeps that we are calling makeAbsoluteAndPreferred on the paths? Maybe we could instead move that call into FullDependencyConsumer. Or are there other issues?

The fact we need to add additional MDC.IsStdModuleP1689Format checks in this PR to prevent the normal consumer callbacks is not great. This makes MDC aware of implementation details of the specific output format.

Yes, the issue can be solved by not calling makeAbsoluteAndPreferred too. And it sounds indeed a good idea to move it to FullDependencyConsumer.

But during the effort to move makeAbsoluteAndPreferred to FullDependencyConsumer, I find it is problematic since FullDependencyConsumer doesn't know about FileManager. And it looks like a bad idea to assign the FileManager to FullDependencyConsumer. How do you think about the idea to add a flag to the MDC about whether or not calling makeAbsoluteAndPreferred?

ChuanqiXu9 added a commit to ChuanqiXu9/llvm-project that referenced this pull request Oct 23, 2023
… default on windows after c++20

There are already 3 issues about the broken state of
-fdelayed-template-parsing and C++20 modules:
- llvm#61068
- llvm#64810
- llvm#65027

The problem is more complex than I thought. I am not sure how to fix it
properly now. Given the complexities and -fdelayed-template-parsing is
actually an extension to support old MS codes, I think it may make sense
to not enable the -fdelayed-template-parsing option by default with
C++20 modules to give more user friendly experience. Users who still want
-fdelayed-template-parsing can specify it explicitly.

Given the discussion in llvm#69551,
we decide to not enable -fdelayed-template-parsing by default on windows
after c++20
@ChuanqiXu9
Copy link
Member Author

@benlangmuir ping~

@benlangmuir
Copy link
Collaborator

Thanks for the ping, I had missed your question

How do you think about the idea to add a flag to the MDC about whether or not calling makeAbsoluteAndPreferred?

SGTM; this seems like a good compromise since we can't easily extract this into the consumer.

@ChuanqiXu9 ChuanqiXu9 force-pushed the ClangScanDepsP1689RelativePath branch from 17b8465 to c91007b Compare October 30, 2023 05:40
@ChuanqiXu9
Copy link
Member Author

Thanks for the ping, I had missed your question

How do you think about the idea to add a flag to the MDC about whether or not calling makeAbsoluteAndPreferred?

SGTM; this seems like a good compromise since we can't easily extract this into the consumer.

Done. Does the current patch look good to you?

if (!IsStdModuleP1689Format) {
llvm::SmallString<256> Storage;
Path = makeAbsoluteAndPreferred(ScanInstance, Path, Storage);
}
FileDeps.push_back(std::string(Path));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a use-after-free of Storage since Path will point to that buffer if the string is modified. Same for the other case below.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for your catch!

…ies with clang

Close llvm#69439.

This patch tries to reuse the codes to generate make style dependencies
information with P1689 format directly.
@ChuanqiXu9 ChuanqiXu9 force-pushed the ClangScanDepsP1689RelativePath branch from c91007b to 6ee8058 Compare October 31, 2023 08:16
@github-actions
Copy link

github-actions bot commented Oct 31, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@ChuanqiXu9 ChuanqiXu9 merged commit e107c94 into llvm:main Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category clang-tools-extra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-scan-deps] [P1689] Inconsistent path style for make style dependency output
3 participants