[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D139168#4119190 , @vvereschaka 
wrote:

> @ChuanqiXu,
>
> would you provide a complete rollback for the changes and fix the 
> `C1689.cppm` test failures on the builders
>
> - 
> https://lab.llvm.org/buildbot/#/builders/119/builds/11935/steps/9/logs/FAIL__Clang__P1689_cppm
> - 
> https://lab.llvm.org/buildbot/#/builders/60/builds/10655/steps/9/logs/FAIL__Clang__P1689_cppm

The failure should be fixed by https://reviews.llvm.org/D143749, which disables 
the test in windows. (Because the inconsistent slash direction in linux and 
windows)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Vlad Vereschaka via Phabricator via cfe-commits]

vvereschaka added a comment.

@ChuanqiXu,

would you provide a complete rollback for the changes and fix the `C1689.cppm` 
test failures on the builders

- 
https://lab.llvm.org/buildbot/#/builders/119/builds/11935/steps/9/logs/FAIL__Clang__P1689_cppm
- 
https://lab.llvm.org/buildbot/#/builders/60/builds/10655/steps/9/logs/FAIL__Clang__P1689_cppm


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Tom Weaver via Phabricator via cfe-commits]

TWeaver added a comment.

@ChuanqiXu Hi there, the revert of this patch has caused the following buildbot 
to start failing:

https://lab.llvm.org/buildbot/#/builders/192/builds/267

It looks as though the removal of the // REQUIRES: !system-windows has caused 
the test to start running, and therefore failing, on Windows builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe1354763b6e6: [C++20] [Modules] [ClangScanDeps] Enable to 
print make-style dependency file… (authored by ChuanqiXu).

Changed prior to commit:
  https://reviews.llvm.org/D139168?vs=486472=496315#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -825,10 +825,47 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
-  auto MaybeRule =
-  WorkerTools[I]->getP1689ModuleDependencyFile(*Input, CWD);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  // It is useful to generate the make-format dependency output during
+  // the scanning for P1689. Otherwise the users need to scan again for
+  // it. We will generate the make-format dependency output if we find
+  // `-MF` in the command lines.
+  std::string MakeformatOutputPath;
+  std::string MakeformatOutput;
+
+  auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
+  *Input, CWD, MakeformatOutput, MakeformatOutputPath,
+  MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (!MakeformatOutputPath.empty() && !MakeformatOutput.empty() &&
+  !HadErrors) {
+static std::mutex Lock;
+// With compilation database, we may open different files
+// concurrently or we may write the same file concurrently. So we
+// use a map here to allow multiple compile commands to write to the
+// same file. Also we need a lock here to avoid data race.
+static llvm::StringMap OSs;
+std::unique_lock LockGuard(Lock);
+
+auto OSIter = OSs.find(MakeformatOutputPath);
+if (OSIter == OSs.end()) {
+  std::error_code EC;
+  OSIter = OSs.try_emplace(MakeformatOutputPath,
+   MakeformatOutputPath, EC)
+   .first;
+  if (EC)
+llvm::errs()
+<< "Failed to open P1689 make format output file \""
+<< MakeformatOutputPath << "\" for " << EC.message()
+<< "\n";
+}
+
+SharedStream MakeformatOS(OSIter->second);
+llvm::Expected MaybeOutput(MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   MakeformatOS, Errs);
+  }
 } else if (MaybeModuleName) {
   auto MaybeModuleDepsGraph = WorkerTools[I]->getModuleDependencies(
   *MaybeModuleName, Input->CommandLine, CWD, AlreadySeenModules,
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -1,3 +1,8 @@
+// It is annoying to handle different slash direction
+// in the filesystem of Windows and Linux.
+// So we disable the test on Windows here.
+// REQUIRES: !system-windows
+//
 // RUN: rm -fr %t
 // RUN: mkdir -p %t
 // RUN: split-file %s %t
@@ -25,42 +30,50 @@
 // RUN: clang-scan-deps -format=p1689 \
 // RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/User.cpp -o %t/User.o \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/impl_part.cppm -o %t/impl_part.o \
+// RUN:  -MT %t/impl_part.o.ddi -MD -MF %t/impl_part.dep
+// RUN:   cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
+//
+// Check that we can generate multiple make-style dependency information with compilation database.
+// RUN: cat %t/P1689.dep | FileCheck %t/Checks.cpp -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
 {
   "directory": "DIR",
-  "command": "clang++ -std=c++20 DIR/M.cppm -c -o DIR/M.o",
+  "command": "clang++ -std=c++20 DIR/M.cppm -c -o DIR/M.o -MT DIR/M.o.ddi -MD -MF DIR/P1689.dep",
   "file": "DIR/M.cppm",
   

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

LGTM.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D139168#4036850 , @tahonermann 
wrote:

>> Header units have even more need to be involved in the build graph than 
>> named units. ODR violations and cache invalidation problems await anyone 
>> just winging it on header units (at least that's the understanding I've 
>> gotten from SG15 meetings).
>
> I think that latter claim applies equally to all module units. The ODR 
> violation and cache invalidation concerns sometimes associated with header 
> units occur in implicit module build systems in which a header unit might be 
> built multiple times with different sets of options that result in an ODR 
> violation. The same problem can occur with other kinds of module units if 
> they are built multiple times with different options and then imported by 
> distinct TUs that are then linked together. The general rule is, given a set 
> of TUs that will be linked together, all imported module units should be 
> built exactly one time.

I think the option set will be naturally a unique set for a single module-unit 
since a module-unit is an actual translation unit. But the header units are 
synthesized units so it is not the same case.

I am pretty interested about the topic indeed but I guess we'd better to find 
another place to discuss this.




Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:775
+static llvm::StringMap OSs;
+std::unique_lock LockGuard(Lock);
+

jansvoboda11 wrote:
> How will this work when a different process tries to write the same file? 
> Could we write into a temporary file and then do atomic rename?
It may be problematic if another process tries to write the same file at the 
same time. But I feel it is an overkill to defend such cases otherwise many 
existing codes need to be refactored.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:775
+static llvm::StringMap OSs;
+std::unique_lock LockGuard(Lock);
+

How will this work when a different process tries to write the same file? Could 
we write into a temporary file and then do atomic rename?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> Header units have even more need to be involved in the build graph than named 
> units. ODR violations and cache invalidation problems await anyone just 
> winging it on header units (at least that's the understanding I've gotten 
> from SG15 meetings).

I think that latter claim applies equally to all module units. The ODR 
violation and cache invalidation concerns sometimes associated with header 
units occur in implicit module build systems in which a header unit might be 
built multiple times with different sets of options that result in an ODR 
violation. The same problem can occur with other kinds of module units if they 
are built multiple times with different options and then imported by distinct 
TUs that are then linked together. The general rule is, given a set of TUs that 
will be linked together, all imported module units should be built exactly one 
time.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Arthur Laurent via Phabricator via cfe-commits]

Arthapz added a comment.

> BTW, for header units, it is still under discussion that how should build 
> system and compiler interact about header units. It is still unclear whether 
> or not the header units should be transparent to build systems (and other 
> tools).

On XMake we built our support around MSVC then extended support to GCC and 
clang, so we handle header units separately from named modules (but in a 
similar way), i don't have problem to add some logic in XMake to support them 
for clang


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> FWIW, the same with should happen with -MT.

Yeah, it should be. Since the patch reuses the previous logics.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D139168#4034885 , @ChuanqiXu wrote:

> @Arthapz Great to hear that! It is pretty important for us to know there are 
> more build systems which are using these functionality. BTW, for header 
> units, it is still under discussion that how should build system and compiler 
> interact about header units. It is still unclear whether or not the header 
> units should be transparent to build systems (and other tools).

Header units have even more need to be involved in the build graph than named 
units. ODR violations and cache invalidation problems await anyone just winging 
it on header units (at least that's the understanding I've gotten from SG15 
meetings).

> I mean, it is possible to get the make-style format information by 
> `clang-scan-deps -format=make --compilation-database=db.json`. (Although it 
> sounds not smart indeed)

I don't see the benefit of having to run two commands to get this information.

> Then how do you feel about the current strategy? `clang-scan-deps 
> --format=p1689 --compilation-database=db.json`. Then if every commands in 
> db.json has the same `-MF` value, it should be able to satisfy your 
> requirement.

This seems…reasonable. No worse than the other lies we have to tell in a 
databse. FWIW, the same with should happen with `-MT`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> Hi, just wanted to say that i added support of these patch to XMake (i built 
> a LLVM with your 4 patches) and it work pretty well :) (except for header 
> units)

@Arthapz Great to hear that! It is pretty important for us to know there are 
more build systems which are using these functionality. BTW, for header units, 
it is still under discussion that how should build system and compiler interact 
about header units. It is still unclear whether or not the header units should 
be transparent to build systems (and other tools).

In D139168#4032866 , @ben.boeckel 
wrote:

> In D139168#4030425 , @ChuanqiXu 
> wrote:
>
>> BTW, if you are interested, it should be possible to use clang-scan-deps to 
>> get the make-style format information.
>
> I want a *single* depfile for *anything* scanned by the `clang-scan-deps` 
> invocation, not one per TU scanned. Also note that the `-MT` is the output of 
> `clang-scan-deps`; in this case, the `.ddi` file that contains P1689 
>  content. I think it'd be *very* weird to 
> have `clang-scan-deps -p1689-output=out.ddi -- clang -MF blah.d -MT out.ddi`.

I mean, it is possible to get the make-style format information by 
`clang-scan-deps -format=make --compilation-database=db.json`. (Although it 
sounds not smart indeed)

Then how do you feel about the current strategy? `clang-scan-deps 
--format=p1689 --compilation-database=db.json`. Then if every commands in 
db.json has the same `-MF` value, it should be able to satisfy your requirement.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D139168#4030425 , @ChuanqiXu wrote:

> BTW, if you are interested, it should be possible to use clang-scan-deps to 
> get the make-style format information.

I want a *single* depfile for *anything* scanned by the `clang-scan-deps` 
invocation, not one per TU scanned. Also note that the `-MT` is the output of 
`clang-scan-deps`; in this case, the `.ddi` file that contains P1689 
 content. I think it'd be *very* weird to have 
`clang-scan-deps -p1689-output=out.ddi -- clang -MF blah.d -MT out.ddi`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Arthur Laurent via Phabricator via cfe-commits]

Arthapz added a comment.

Hi, just wanted to say that i added support of these patch to XMake and it work 
pretty well :)

  > xmake b -vD 
  
  [  0%]: generating.module.deps src/main.cpp
  checking for clang-scan-deps ... /usr/bin/clang-scan-deps
  checking for flags (-std=c++20) ... ok
  > clang "-std=c++20"
  checking for flags (-fmodules) ... ok
  > clang "-fmodules"
  /usr/bin/clang-scan-deps --format=p1689 -- /usr/bin/clang -x c++ -c 
src/main.cpp -o build/.objs/dependence/linux/x86_64/release/src/main.cpp.o 
-Qunused-arguments -m64 -std=c++20 -fmodules -fno-implicit-module-maps
  [  0%]: generating.module.deps src/foo.mpp
  /usr/bin/clang-scan-deps --format=p1689 -- /usr/bin/clang -x c++ -c 
src/foo.mpp -o build/.objs/dependence/linux/x86_64/release/src/foo.mpp.o 
-Qunused-arguments -m64 -std=c++20 -fmodules -fno-implicit-module-maps
  [  0%]: generating.module.deps src/zoo.mpp
  /usr/bin/clang-scan-deps --format=p1689 -- /usr/bin/clang -x c++ -c 
src/zoo.mpp -o build/.objs/dependence/linux/x86_64/release/src/zoo.mpp.o 
-Qunused-arguments -m64 -std=c++20 -fmodules -fno-implicit-module-maps
  [  0%]: generating.module.deps src/cat.mpp
  /usr/bin/clang-scan-deps --format=p1689 -- /usr/bin/clang -x c++ -c 
src/cat.mpp -o build/.objs/dependence/linux/x86_64/release/src/cat.mpp.o 
-Qunused-arguments -m64 -std=c++20 -fmodules -fno-implicit-module-maps
  [  0%]: generating.module.deps src/bar.mpp
  /usr/bin/clang-scan-deps --format=p1689 -- /usr/bin/clang -x c++ -c 
src/bar.mpp -o build/.objs/dependence/linux/x86_64/release/src/bar.mpp.o 
-Qunused-arguments -m64 -std=c++20 -fmodules -fno-implicit-module-maps
  checking for flags (clang_modules_cache_path) ... ok
  > clang "-fmodules-cache-path=/dev/shm/.xmake1000/230107"
  [ 10%]: compiling.module.release zoo
  /usr/bin/clang -c -x c++-module --precompile -Qunused-arguments -m64 
-std=c++20 -fmodules -fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 -o build/.gens/dependence/linux/x86_64/release/rules/modules/cache/zoo.pcm 
src/zoo.mpp
  /usr/bin/clang -c -Qunused-arguments -m64 -std=c++20 -fmodules 
-fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 -o build/.objs/dependence/linux/x86_64/release/src/zoo.mpp.o 
build/.gens/dependence/linux/x86_64/release/rules/modules/cache/zoo.pcm
  checking for flags (-MMD -MF) ... ok
  > clang "-MMD" "-MF" "/dev/null"
  [ 10%]: compiling.module.release cat
  /usr/bin/clang -c -x c++-module --precompile -Qunused-arguments -m64 
-std=c++20 -fmodules -fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 -o build/.gens/dependence/linux/x86_64/release/rules/modules/cache/cat.pcm 
src/cat.mpp
  /usr/bin/clang -c -Qunused-arguments -m64 -std=c++20 -fmodules 
-fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 -o build/.objs/dependence/linux/x86_64/release/src/cat.mpp.o 
build/.gens/dependence/linux/x86_64/release/rules/modules/cache/cat.pcm
  checking for flags (-fdiagnostics-color=always) ... ok
  > clang "-fdiagnostics-color=always"
  checking for flags (clang_module_file) ... ok
  > clang 
"-fmodule-file=/dev/shm/.xmake1000/230107/_11EA40624C464D10876A6DA3D0E41320.pcm"
  [ 30%]: compiling.module.release bar
  /usr/bin/clang -c -x c++-module --precompile -Qunused-arguments -m64 
-std=c++20 -fmodules -fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 
-fmodule-file=build/.gens/dependence/linux/x86_64/release/rules/modules/cache/zoo.pcm
 -o build/.gens/dependence/linux/x86_64/release/rules/modules/cache/bar.pcm 
src/bar.mpp
  /usr/bin/clang -c -Qunused-arguments -m64 -std=c++20 -fmodules 
-fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 
-fmodule-file=build/.gens/dependence/linux/x86_64/release/rules/modules/cache/zoo.pcm
 -o build/.objs/dependence/linux/x86_64/release/src/bar.mpp.o 
build/.gens/dependence/linux/x86_64/release/rules/modules/cache/bar.pcm
  [ 40%]: compiling.module.release foo
  /usr/bin/clang -c -x c++-module --precompile -Qunused-arguments -m64 
-std=c++20 -fmodules -fno-implicit-module-maps 
-fmodules-cache-path=build/.gens/dependence/linux/x86_64/release/rules/modules/cache
 
-fmodule-file=build/.gens/dependence/linux/x86_64/release/rules/modules/cache/bar.pcm
 
-fmodule-file=build/.gens/dependence/linux/x86_64/release/rules/modules/cache/zoo.pcm
 
-fmodule-file=build/.gens/dependence/linux/x86_64/release/rules/modules/cache/cat.pcm
 -o build/.gens/dependence/linux/x86_64/release/rules/modules/cache/foo.pcm 
src/foo.mpp
  /usr/bin/clang -c -Qunused-arguments -m64 -std=c++20 -fmodules 
-fno-implicit-module-maps 

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

>> In what mode will clang output #include information without erroring about 
>> missing .pcm files for import statements? -E -fdirectives-only perhaps? This 
>> further exacerbates the need for "fake" command lines and costs on platforms 
>> with expensive process execution.
>
> After we land D137526 , we are able to do 
> this by: `-M -MF  -E`.

BTW, if you are interested, it should be possible to use clang-scan-deps to get 
the make-style format information.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> IMO, this is not suitable; there must be *one* depfile for Ninja to work 
> (otherwise build tools will need to manually collate all of this into one for 
> ninja.

Yeah, but the producer of the compilation database is able to create a 
compilation db with the same `-MF` option. Do you mean you will feel much 
better that the clang-scan-deps will have a strong check?

>> 2. (The original way) Specify -MF in the command line of clang-scan-deps.
>
> I feel this scales and communicates what is happening much better because it 
> actually is a flag for clang-scan-deps itself.

There is a related problem that how do we set the target of the dependency? In 
per file mode, we can easily specify it in the command line of clang-scan-deps. 
But we have to extract them ("-MT") from the command line with compilation 
database. (Otherwise all the targets have the same name, which is incorrect). 
Do you think it is acceptable?

> In what mode will clang output #include information without erroring about 
> missing .pcm files for import statements? -E -fdirectives-only perhaps? This 
> further exacerbates the need for "fake" command lines and costs on platforms 
> with expensive process execution.

After we land D137526 , we are able to do 
this by: `-M -MF  -E`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D139168#4027637 , @ChuanqiXu wrote:

> Currently, clang-scan-deps won't check for this. If we have multiple command 
> lines with different `-MF` value, the make-style dependency information will 
> be written to these different depfiles.

IMO, this is not suitable; there must be *one* depfile for Ninja to work 
(otherwise build tools will need to manually collate all of this into one for 
`ninja`.

> I feel like we have 2 (or 3) options
>
> 1. (The current way) Extract `-MF` in the command line of clang (from 
> compilation database or from the args after `--`)

See above; it works for the file-by-file, but falls over with batch scanning.

> 2. (The original way) Specify `-MF` in the command line of clang-scan-deps.

I feel this scales and communicates what is happening much better because it 
actually is a flag for `clang-scan-deps` itself.

> 3. (Not good) Do nothing. I feel like it is possible for build systems to get 
> the make-style dependency information by scanning twice. One for P1689 
>  format and one for make-format. It may be 
> workable but it sounds a little bit silly.

In what mode will `clang` output `#include` information without erroring about 
missing `.pcm` files for `import` statements? `-E -fdirectives-only` perhaps? 
This further exacerbates the need for "fake" command lines and costs on 
platforms with expensive process execution.

> the make-style dependency information of input_file in  will be 
> overwritten. Or would cmake like to concat the results manually?

Every command gets its own unique ``. Scanning, compilation, etc. 
Overlapping is just asking for race conditions in the build.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@ben.boeckel I updated the test with compilation db. BTW, I am curious that 
according to https://reviews.llvm.org/D137534, cmake may not use 
clang-scan-deps with compilation db for P1689 . 
So this is a pure review comment and it is not related with the planned 
practice of CMake, right? Or did I misunderstand anything?

---

Also I am wondering if cmake will only use clang-scan-deps without compilation 
db for P1689 , how can it generate multiple 
make-style dependency information into one file.

I mean, if we execute:

  clang-scan-deps -format=p1689 -- clang++ -std=c++20 -MD -MT  -MF 
 -c input_file  -o output_file

then we execute:

  clang-scan-deps -format=p1689 -- clang++ -std=c++20 -MD -MT 
 -MF  -c another_input_file 
 -o another_output_file

the make-style dependency information of `input_file` in  will be 
overwritten. Or would cmake like to concat the results manually?




Comment at: clang/test/ClangScanDeps/P1689.cppm:283-294
+// CHECK-MAKE-DAG: [[PREFIX]]/impl_part.o.ddi: \
+// CHECK-MAKE-DAG-NEXT:   [[PREFIX]]/impl_part.cppm \
+// CHECK-MAKE-DAG-NEXT:   [[PREFIX]]/header.mock
+// CHECK-MAKE-DAG: [[PREFIX]]/interface_part.o.ddi: \
+// CHECK-MAKE-DAG-NEXT:   [[PREFIX]]/interface_part.cppm
+// CHECK-MAKE-DAG: [[PREFIX]]/M.o.ddi: \
+// CHECK-MAKE-DAG-NEXT:   [[PREFIX]]/M.cppm

We can find the result to write make-style dependency information with 
compilation database here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 486472.
ChuanqiXu added a comment.

Add the test to generate make-style dependency file with compilation database.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -751,10 +751,47 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
+  // It is useful to generate the make-format dependency output during
+  // the scanning for P1689. Otherwise the users need to scan again for
+  // it. We will generate the make-format dependency output if we find
+  // `-MF` in the command lines.
+  std::string MakeformatOutputPath;
+  std::string MakeformatOutput;
+
   auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
-  *Input, CWD, MaybeModuleName);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  *Input, CWD, MakeformatOutput, MakeformatOutputPath,
+  MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (!MakeformatOutputPath.empty() && !MakeformatOutput.empty() &&
+  !HadErrors) {
+static std::mutex Lock;
+// With compilation database, we may open different files
+// concurrently or we may write the same file concurrently. So we
+// use a map here to allow multiple compile commands to write to the
+// same file. Also we need a lock here to avoid data race.
+static llvm::StringMap OSs;
+std::unique_lock LockGuard(Lock);
+
+auto OSIter = OSs.find(MakeformatOutputPath);
+if (OSIter == OSs.end()) {
+  std::error_code EC;
+  OSIter = OSs.try_emplace(MakeformatOutputPath,
+   MakeformatOutputPath, EC)
+   .first;
+  if (EC)
+llvm::errs()
+<< "Failed to open P1689 make format output file \""
+<< MakeformatOutputPath << "\" for " << EC.message()
+<< "\n";
+}
+
+SharedStream MakeformatOS(OSIter->second);
+llvm::Expected MaybeOutput(MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   MakeformatOS, Errs);
+  }
 } else if (DeprecatedDriverCommand) {
   auto MaybeFullDeps =
   WorkerTools[I]->getFullDependenciesLegacyDriverCommand(
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -22,42 +22,50 @@
 // RUN: clang-scan-deps -format=p1689 \
 // RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/User.cpp -o %t/User.o \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/impl_part.cppm -o %t/impl_part.o \
+// RUN:  -MT %t/impl_part.o.ddi -MD -MF %t/impl_part.dep
+// RUN:   cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
+//
+// Check that we can generate multiple make-style dependency information with compilation database.
+// RUN: cat %t/P1689.dep | FileCheck %t/Checks.cpp -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
 {
   "directory": "DIR",
-  "command": "clang++ -std=c++20 DIR/M.cppm -c -o DIR/M.o",
+  "command": "clang++ -std=c++20 DIR/M.cppm -c -o DIR/M.o -MT DIR/M.o.ddi -MD -MF DIR/P1689.dep",
   "file": "DIR/M.cppm",
   "output": "DIR/M.o"
 },
 {
   "directory": "DIR",
-  "command": "clang++ -std=c++20 DIR/Impl.cpp -c -o DIR/Impl.o",
+  "command": "clang++ -std=c++20 DIR/Impl.cpp -c -o DIR/Impl.o -MT DIR/Impl.o.ddi -MD -MF DIR/P1689.dep",
   "file": "DIR/Impl.cpp",
   "output": "DIR/Impl.o"
 },
 {
   "directory": "DIR",
-  "command": "clang++ -std=c++20 DIR/impl_part.cppm -c -o DIR/impl_part.o",
+  "command": "clang++ -std=c++20 DIR/impl_part.cppm -c -o DIR/impl_part.o -MT DIR/impl_part.o.ddi -MD -MF DIR/P1689.dep",
   "file": "DIR/impl_part.cppm",
   "output": "DIR/impl_part.o"
 },
 {
   "directory": 

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D139168#4026799 , @ben.boeckel 
wrote:

> In D139168#4025277 , @ChuanqiXu 
> wrote:
>
>> Currently we will detect `-MF` in the command line and we will write the 
>> make-format dependency output to the corresponding file once we find `-MF`.
>
> Which is fine, but docs need to mention that some clang-looking flags are 
> actually for `clang-scan-deps` and that, as such, these flags cannot just be 
> grabbed blindly from a compilation database.

Sure, agreed. I'll add docs for the series patches once they got accepted 
(otherwise we might do many useless works).

> When using clang-scan-deps with a compdb with multiple command lines, which 
> depfile path will it use? Or must all agree on the same path and options? 
> Because there should be a single one for all scanning that is performed 
> (e.g., in batch mode).

Currently, clang-scan-deps won't check for this. If we have multiple command 
lines with different `-MF` value, the make-style dependency information will be 
written to these different depfiles. If all the command lines use the same 
depfile, then the make-style dependency information will be written to the 
depfile. (BTW, We have a lock for this so we don't need to worry about data 
racing).

> Is this really simpler than just adding -MF and friends to clang-scan-deps 
> directly?

Personally, I feel the complexity of both methods are acceptable. I rewrite the 
patch since it looks like @jansvoboda11 don't like to add new command line 
options to clang-scan-deps.

I feel like we have 2 (or 3) options

1. (The current way) Extract `-MF` in the command line of clang (from 
compilation database or from the args after `--`)
2. (The original way) Specify `-MF` in the command line of clang-scan-deps.
3. (Not good) Do nothing. I feel like it is possible for build systems to get 
the make-style dependency information by scanning twice. One for P1689 
 format and one for make-format. It may be 
workable but it sounds a little bit silly.

Personally, I feel the first (current) style is cleaner.

> I'll note that I'm becoming more and more convinced that 
> `compilation_database.json` is being abused for this as we are quite 
> contorting it from its intended use case: listing the command lines to 
> compile sources. Instead, we are using it as a source of *related* 
> information that differs from the *real* command line in some important ways. 
> Namely:
>
> - dependency information extraction is re-used and therefore must be 
> different from the *real* command
> - module information must be missing from the scanning command line as it 
> cannot be known at this point as scanning is used to discover those flags in 
> the first place (usually a response file)

Agreed. But this may beyond the scope of the current patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D139168#4025277 , @ChuanqiXu wrote:

> Currently we will detect `-MF` in the command line and we will write the 
> make-format dependency output to the corresponding file once we find `-MF`.

When using `clang-scan-deps` with a compdb with multiple command lines, which 
depfile path will it use? Or must all agree on the same path and options? 
Because there should be a single one for all scanning that is performed (e.g., 
in batch mode). Is this really simpler than just adding `-MF` and friends to 
`clang-scan-deps` directly?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D139168#4025277 , @ChuanqiXu wrote:

> Currently we will detect `-MF` in the command line and we will write the 
> make-format dependency output to the corresponding file once we find `-MF`.

Which is fine, but docs need to mention that some clang-looking flags are 
actually for `clang-scan-deps` and that, as such, these flags cannot just be 
grabbed blindly from a compilation database. I'll note that I'm becoming more 
and more convinced that `compilation_database.json` is being abused for this as 
we are quite contorting it from its intended use case: listing the command 
lines to compile sources. Instead, we are using it as a source of *related* 
information that differs from the *real* command line in some important ways. 
Namely:

- dependency information extraction is re-used and therefore must be different 
from the *real* command
- module information must be missing from the scanning command line as it 
cannot be known at this point as scanning is used to discover those flags in 
the first place (usually a response file)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

Currently we will detect `-MF` in the command line and we will write the 
make-format dependency output to the corresponding file once we find `-MF`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 486196.
ChuanqiXu added a comment.

Update since https://reviews.llvm.org/D137534 changes.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -751,10 +751,33 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
+  // It is useful to generate the make-format dependency output during
+  // the scanning for P1689. Otherwise the users need to scan again for
+  // it. We will generate the make-format dependency output if we find
+  // `-MF` in the command lines.
+  std::string MakeformatOutputPath;
+  std::string MakeformatOutput;
+
   auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
-  *Input, CWD, MaybeModuleName);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  *Input, CWD, MakeformatOutput, MakeformatOutputPath,
+  MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (!MakeformatOutputPath.empty() && !MakeformatOutput.empty() &&
+  !HadErrors) {
+std::error_code EC;
+llvm::raw_fd_ostream OS(MakeformatOutputPath, EC);
+if (EC)
+  llvm::errs() << "Failed to open P1689 make format output file \""
+   << MakeformatOutputPath << "\" for " << EC.message()
+   << "\n";
+SharedStream MakeformatOS(OS);
+
+llvm::Expected MaybeOutput(MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   MakeformatOS, Errs);
+  }
 } else if (DeprecatedDriverCommand) {
   auto MaybeFullDeps =
   WorkerTools[I]->getFullDependenciesLegacyDriverCommand(
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -22,6 +22,12 @@
 // RUN: clang-scan-deps -format=p1689 \
 // RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/User.cpp -o %t/User.o \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/impl_part.cppm -o %t/impl_part.o \
+// RUN:  -MT %t/impl_part.o.ddi -MD -MF %t/impl_part.dep
+// RUN:   cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
@@ -57,7 +63,6 @@
 }
 ]
 
-
 //--- M.cppm
 export module M;
 export import :interface_part;
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -39,67 +39,69 @@
 llvm::IntrusiveRefCntPtr FS)
 : Worker(Service, std::move(FS)) {}
 
-llvm::Expected DependencyScanningTool::getDependencyFile(
-const std::vector , StringRef CWD,
-llvm::Optional ModuleName) {
-  /// Prints out all of the gathered dependencies into a string.
-  class MakeDependencyPrinterConsumer : public DependencyConsumer {
-  public:
-void handleBuildCommand(Command) override {}
+namespace {
+/// Prints out all of the gathered dependencies into a string.
+class MakeDependencyPrinterConsumer : public DependencyConsumer {
+public:
+  void handleBuildCommand(Command) override {}
+
+  void
+  handleDependencyOutputOpts(const DependencyOutputOptions ) override {
+this->Opts = std::make_unique(Opts);
+  }
 
-void
-handleDependencyOutputOpts(const DependencyOutputOptions ) override {
-  this->Opts = std::make_unique(Opts);
-}
+  void handleFileDependency(StringRef File) override {
+Dependencies.push_back(std::string(File));
+  }
 
-void handleFileDependency(StringRef File) override {
-  Dependencies.push_back(std::string(File));
-}
+  void handlePrebuiltModuleDependency(PrebuiltModuleDep PMD) override {
+// Same as `handleModuleDependency`.
+  }
 
-void handlePrebuiltModuleDependency(PrebuiltModuleDep PMD) override {
-  // 

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@Bigcheese @jansvoboda11 gentle ping~


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 483083.
ChuanqiXu added a comment.

Rebase onto main.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -187,6 +187,13 @@
"dependencies are to be computed."),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt P1689MakeformatOutputpath(
+"p1689-makeformat-output", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689. Print the make-style dependency "
+   "output to the specified output. This is a helper for build "
+   "systems to do duplicate scanning."),
+llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt P1689TargettedCommand(
 llvm::cl::Positional, llvm::cl::ZeroOrMore,
 llvm::cl::desc("The command line flags for the target of which "
@@ -584,6 +591,19 @@
   return std::move(FixedCompilationDatabase);
 }
 
+static raw_ostream () {
+  if (!P1689MakeformatOutputpath.empty()) {
+std::error_code EC;
+static llvm::raw_fd_ostream OS(P1689MakeformatOutputpath, EC);
+if (EC)
+  llvm::errs() << "Failed to open P1689 make format output file \""
+   << P1689MakeformatOutputpath << "\" for " << EC.message()
+   << "\n";
+return OS;
+  }
+  return llvm::outs();
+}
+
 int main(int argc, const char **argv) {
   std::string ErrorMessage;
   std::unique_ptr Compilations =
@@ -662,7 +682,7 @@
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
-  SharedStream DependencyOS(llvm::outs());
+  SharedStream DependencyOS(getDependencyOS());
 
   DependencyScanningService Service(ScanMode, Format, OptimizeArgs,
 EagerLoadModules);
@@ -722,10 +742,20 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
+  llvm::Optional MakeformatOutput;
+  if (!P1689MakeformatOutputpath.empty())
+MakeformatOutput.emplace();
+
   auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
-  *Input, CWD, MaybeModuleName);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  *Input, CWD, MakeformatOutput, MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (MakeformatOutput && !MakeformatOutput->empty() && !HadErrors) {
+llvm::Expected MaybeOutput(*MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   DependencyOS, Errs);
+  }
 } else if (DeprecatedDriverCommand) {
   auto MaybeFullDeps =
   WorkerTools[I]->getFullDependenciesLegacyDriverCommand(
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -22,6 +22,11 @@
 // RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/User.cpp --p1689-targeted-output=%t/User.o \
 // RUN:   -- -std=c++20 -c \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/impl_part.cppm --p1689-targeted-output=%t/impl_part.o \
+// RUN:   --p1689-makeformat-output=%t/impl_part.dep -- -std=c++20 -c -MT %t/impl_part.o.ddi -MD
+// RUN: cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
@@ -57,7 +62,6 @@
 }
 ]
 
-
 //--- M.cppm
 export module M;
 export import :interface_part;
@@ -145,6 +149,10 @@
 // CHECK-NEXT:   "version": 1
 // CHECK-NEXT: }
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o.ddi:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm
+// CHECK-MAKE:   [[PREFIX]]/header.mock
+
 //--- interface_part.cppm
 export module M:interface_part;
 export void World();
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -39,67 +39,69 @@
 llvm::IntrusiveRefCntPtr FS)
 : Worker(Service, std::move(FS)) {}
 
-llvm::Expected 

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

ping @Bigcheese @jansvoboda11 would you take a look at this if it will affect 
the existing users and the development? Thanks!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

This worked for CMake in a previous version, so is good enough on the 
functionality front. I'll retry with the current state to verify.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ben.boeckel wrote:
> ben.boeckel wrote:
> > ChuanqiXu wrote:
> > > ben.boeckel wrote:
> > > > ChuanqiXu wrote:
> > > > > ben.boeckel wrote:
> > > > > > For scanning, this cannot be the object file. The output needs to 
> > > > > > be the P1689 output (or whatever the "main output" for the scanning 
> > > > > > rule is). This is the purpose behind the `-MT ` flag: to 
> > > > > > say what goes in this slot. I think it'll be necessary here (even 
> > > > > > if `clang-scan-deps` learns an `-o` flag because it is the build 
> > > > > > system that determines the "primary output").
> > > > > > 
> > > > > > I don't know if the `-MMD` and `-MD` differences are important or 
> > > > > > not; I don't think I particularly care which is default (I've used 
> > > > > > `-MD` FWIW), but it may matter for other situations.
> > > > > I am confused since the output `[[PREFIX]]/impl_part.o` is the same 
> > > > > with `P1689` output `[[PREFIX]]/impl_part.o` and the one in the 
> > > > > compilation database and the one specified in the command line option 
> > > > > `--p1689-targeted-output`. What's the expected output for you in this 
> > > > > case? (and the related command line input.)
> > > > P1689 is about specifying dependencies of *another* rule found by the 
> > > > dynamic content of some source. `-MF` is about *discovered* 
> > > > dependencies of *this* rule.
> > > hmmm sorry, I don't understand it a lot. May you explain your expectation 
> > > in the form of the input and the corresponding output?
> > *This* rule outputs `foo.ddi` (in CMake terms). We need to tell make or 
> > ninja what files, if they change, *this* rule needs rerun for. That is what 
> > `-MF` is for. What I need is spelled `-MT` "normally".
> > 
> > P1689, what this rule is *doing*, is writing dependencies for the *compile* 
> > rule, so it is hooked up by *its output*. Two rules cannot have the same 
> > output, so P1689 and `-MF` have completely different things to put for 
> > their "output".
> > 
> > You can see here: 
> > https://gitlab.kitware.com/cmake/cmake/-/blob/master/.gitlab/ci/cxx_modules_rules_gcc.cmake
> >  where `-MT` gets the `` which is the `-fdep-file=` argument. 
> > `-fdep-file=` tells GCC what rule the P1689 itself is for.
> > -fdep-output= tells GCC what rule the P1689 itself is for.
> 
> Fixed.
Oh, I guess I get your point. Now we can set the output name with `-MT 
[[outputname]] -MD` in the input after `--`. See the above changes for example. 
Is this what you want?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu updated this revision to Diff 480329.
ChuanqiXu added a comment.

Address comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -187,6 +187,13 @@
"dependencies are to be computed."),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt P1689MakeformatOutputpath(
+"p1689-makeformat-output", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689. Print the make-style dependency "
+   "output to the specified output. This is a helper for build "
+   "systems to do duplicate scanning."),
+llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt P1689TargettedCommand(
 llvm::cl::Positional, llvm::cl::ZeroOrMore,
 llvm::cl::desc("The command line flags for the target of which "
@@ -584,6 +591,19 @@
   return std::move(FixedCompilationDatabase);
 }
 
+static raw_ostream () {
+  if (!P1689MakeformatOutputpath.empty()) {
+std::error_code EC;
+static llvm::raw_fd_ostream OS(P1689MakeformatOutputpath, EC);
+if (EC)
+  llvm::errs() << "Failed to open P1689 make format output file \""
+   << P1689MakeformatOutputpath << "\" for " << EC.message()
+   << "\n";
+return OS;
+  }
+  return llvm::outs();
+}
+
 int main(int argc, const char **argv) {
   std::string ErrorMessage;
   std::unique_ptr Compilations =
@@ -662,7 +682,7 @@
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
-  SharedStream DependencyOS(llvm::outs());
+  SharedStream DependencyOS(getDependencyOS());
 
   DependencyScanningService Service(ScanMode, Format, OptimizeArgs,
 EagerLoadModules);
@@ -722,10 +742,20 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
+  llvm::Optional MakeformatOutput;
+  if (!P1689MakeformatOutputpath.empty())
+MakeformatOutput.emplace();
+
   auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
-  *Input, CWD, MaybeModuleName);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  *Input, CWD, MakeformatOutput, MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (MakeformatOutput && !MakeformatOutput->empty() && !HadErrors) {
+llvm::Expected MaybeOutput(*MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   DependencyOS, Errs);
+  }
 } else if (DeprecatedDriverCommand) {
   auto MaybeFullDeps =
   WorkerTools[I]->getFullDependenciesLegacyDriverCommand(
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -22,6 +22,11 @@
 // RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/User.cpp --p1689-targeted-output=%t/User.o \
 // RUN:   -- -std=c++20 -c \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/impl_part.cppm --p1689-targeted-output=%t/impl_part.o \
+// RUN:   --p1689-makeformat-output=%t/impl_part.dep -- -std=c++20 -c -MT %t/impl_part.o.ddi -MD
+// RUN: cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
@@ -57,7 +62,6 @@
 }
 ]
 
-
 //--- M.cppm
 export module M;
 export import :interface_part;
@@ -145,6 +149,10 @@
 // CHECK-NEXT:   "version": 1
 // CHECK-NEXT: }
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o.ddi:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm
+// CHECK-MAKE:   [[PREFIX]]/header.mock
+
 //--- interface_part.cppm
 export module M:interface_part;
 export void World();
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
@@ -39,67 +39,69 @@
 llvm::IntrusiveRefCntPtr FS)
 : Worker(Service, std::move(FS)) {}
 
-llvm::Expected 

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ben.boeckel wrote:
> ChuanqiXu wrote:
> > ben.boeckel wrote:
> > > ChuanqiXu wrote:
> > > > ben.boeckel wrote:
> > > > > For scanning, this cannot be the object file. The output needs to be 
> > > > > the P1689 output (or whatever the "main output" for the scanning rule 
> > > > > is). This is the purpose behind the `-MT ` flag: to say what 
> > > > > goes in this slot. I think it'll be necessary here (even if 
> > > > > `clang-scan-deps` learns an `-o` flag because it is the build system 
> > > > > that determines the "primary output").
> > > > > 
> > > > > I don't know if the `-MMD` and `-MD` differences are important or 
> > > > > not; I don't think I particularly care which is default (I've used 
> > > > > `-MD` FWIW), but it may matter for other situations.
> > > > I am confused since the output `[[PREFIX]]/impl_part.o` is the same 
> > > > with `P1689` output `[[PREFIX]]/impl_part.o` and the one in the 
> > > > compilation database and the one specified in the command line option 
> > > > `--p1689-targeted-output`. What's the expected output for you in this 
> > > > case? (and the related command line input.)
> > > P1689 is about specifying dependencies of *another* rule found by the 
> > > dynamic content of some source. `-MF` is about *discovered* dependencies 
> > > of *this* rule.
> > hmmm sorry, I don't understand it a lot. May you explain your expectation 
> > in the form of the input and the corresponding output?
> *This* rule outputs `foo.ddi` (in CMake terms). We need to tell make or ninja 
> what files, if they change, *this* rule needs rerun for. That is what `-MF` 
> is for. What I need is spelled `-MT` "normally".
> 
> P1689, what this rule is *doing*, is writing dependencies for the *compile* 
> rule, so it is hooked up by *its output*. Two rules cannot have the same 
> output, so P1689 and `-MF` have completely different things to put for their 
> "output".
> 
> You can see here: 
> https://gitlab.kitware.com/cmake/cmake/-/blob/master/.gitlab/ci/cxx_modules_rules_gcc.cmake
>  where `-MT` gets the `` which is the `-fdep-file=` argument. 
> `-fdep-file=` tells GCC what rule the P1689 itself is for.
> -fdep-output= tells GCC what rule the P1689 itself is for.

Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ChuanqiXu wrote:
> ben.boeckel wrote:
> > ChuanqiXu wrote:
> > > ben.boeckel wrote:
> > > > For scanning, this cannot be the object file. The output needs to be 
> > > > the P1689 output (or whatever the "main output" for the scanning rule 
> > > > is). This is the purpose behind the `-MT ` flag: to say what 
> > > > goes in this slot. I think it'll be necessary here (even if 
> > > > `clang-scan-deps` learns an `-o` flag because it is the build system 
> > > > that determines the "primary output").
> > > > 
> > > > I don't know if the `-MMD` and `-MD` differences are important or not; 
> > > > I don't think I particularly care which is default (I've used `-MD` 
> > > > FWIW), but it may matter for other situations.
> > > I am confused since the output `[[PREFIX]]/impl_part.o` is the same with 
> > > `P1689` output `[[PREFIX]]/impl_part.o` and the one in the compilation 
> > > database and the one specified in the command line option 
> > > `--p1689-targeted-output`. What's the expected output for you in this 
> > > case? (and the related command line input.)
> > P1689 is about specifying dependencies of *another* rule found by the 
> > dynamic content of some source. `-MF` is about *discovered* dependencies of 
> > *this* rule.
> hmmm sorry, I don't understand it a lot. May you explain your expectation in 
> the form of the input and the corresponding output?
*This* rule outputs `foo.ddi` (in CMake terms). We need to tell make or ninja 
what files, if they change, *this* rule needs rerun for. That is what `-MF` is 
for. What I need is spelled `-MT` "normally".

P1689, what this rule is *doing*, is writing dependencies for the *compile* 
rule, so it is hooked up by *its output*. Two rules cannot have the same 
output, so P1689 and `-MF` have completely different things to put for their 
"output".

You can see here: 
https://gitlab.kitware.com/cmake/cmake/-/blob/master/.gitlab/ci/cxx_modules_rules_gcc.cmake
 where `-MT` gets the `` which is the `-fdep-file=` argument. 
`-fdep-file=` tells GCC what rule the P1689 itself is for.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ben.boeckel wrote:
> ChuanqiXu wrote:
> > ben.boeckel wrote:
> > > For scanning, this cannot be the object file. The output needs to be the 
> > > P1689 output (or whatever the "main output" for the scanning rule is). 
> > > This is the purpose behind the `-MT ` flag: to say what goes in 
> > > this slot. I think it'll be necessary here (even if `clang-scan-deps` 
> > > learns an `-o` flag because it is the build system that determines the 
> > > "primary output").
> > > 
> > > I don't know if the `-MMD` and `-MD` differences are important or not; I 
> > > don't think I particularly care which is default (I've used `-MD` FWIW), 
> > > but it may matter for other situations.
> > I am confused since the output `[[PREFIX]]/impl_part.o` is the same with 
> > `P1689` output `[[PREFIX]]/impl_part.o` and the one in the compilation 
> > database and the one specified in the command line option 
> > `--p1689-targeted-output`. What's the expected output for you in this case? 
> > (and the related command line input.)
> P1689 is about specifying dependencies of *another* rule found by the dynamic 
> content of some source. `-MF` is about *discovered* dependencies of *this* 
> rule.
hmmm sorry, I don't understand it a lot. May you explain your expectation in 
the form of the input and the corresponding output?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ChuanqiXu wrote:
> ben.boeckel wrote:
> > For scanning, this cannot be the object file. The output needs to be the 
> > P1689 output (or whatever the "main output" for the scanning rule is). This 
> > is the purpose behind the `-MT ` flag: to say what goes in this 
> > slot. I think it'll be necessary here (even if `clang-scan-deps` learns an 
> > `-o` flag because it is the build system that determines the "primary 
> > output").
> > 
> > I don't know if the `-MMD` and `-MD` differences are important or not; I 
> > don't think I particularly care which is default (I've used `-MD` FWIW), 
> > but it may matter for other situations.
> I am confused since the output `[[PREFIX]]/impl_part.o` is the same with 
> `P1689` output `[[PREFIX]]/impl_part.o` and the one in the compilation 
> database and the one specified in the command line option 
> `--p1689-targeted-output`. What's the expected output for you in this case? 
> (and the related command line input.)
P1689 is about specifying dependencies of *another* rule found by the dynamic 
content of some source. `-MF` is about *discovered* dependencies of *this* rule.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

ben.boeckel wrote:
> For scanning, this cannot be the object file. The output needs to be the 
> P1689 output (or whatever the "main output" for the scanning rule is). This 
> is the purpose behind the `-MT ` flag: to say what goes in this slot. 
> I think it'll be necessary here (even if `clang-scan-deps` learns an `-o` 
> flag because it is the build system that determines the "primary output").
> 
> I don't know if the `-MMD` and `-MD` differences are important or not; I 
> don't think I particularly care which is default (I've used `-MD` FWIW), but 
> it may matter for other situations.
I am confused since the output `[[PREFIX]]/impl_part.o` is the same with 
`P1689` output `[[PREFIX]]/impl_part.o` and the one in the compilation database 
and the one specified in the command line option `--p1689-targeted-output`. 
What's the expected output for you in this case? (and the related command line 
input.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:155
 
+// CHECK-MAKE: [[PREFIX]]/impl_part.o:
+// CHECK-MAKE:   [[PREFIX]]/impl_part.cppm

For scanning, this cannot be the object file. The output needs to be the P1689 
output (or whatever the "main output" for the scanning rule is). This is the 
purpose behind the `-MT ` flag: to say what goes in this slot. I think 
it'll be necessary here (even if `clang-scan-deps` learns an `-o` flag because 
it is the build system that determines the "primary output").

I don't know if the `-MMD` and `-MD` differences are important or not; I don't 
think I particularly care which is default (I've used `-MD` FWIW), but it may 
matter for other situations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139168/new/

https://reviews.llvm.org/D139168

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139168: [C++20] [Modules] [ClangScanDeps] Enable to print make-style dependency file within P1689 format (4/4)

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu created this revision.
ChuanqiXu added reviewers: ben.boeckel, Bigcheese, jansvoboda11.
Herald added a project: All.
ChuanqiXu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Required in https://reviews.llvm.org/D137534.

The build systems needs the information to know that "header X changed, 
scanning may have changed, so please rerun scanning". Although it is possible 
to get the information by running clang-scan-deps for the second time with make 
format, it is not user friendly clearly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139168

Files:
  clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
  clang/lib/Tooling/DependencyScanning/DependencyScanningTool.cpp
  clang/test/ClangScanDeps/P1689.cppm
  clang/tools/clang-scan-deps/ClangScanDeps.cpp

Index: clang/tools/clang-scan-deps/ClangScanDeps.cpp
===
--- clang/tools/clang-scan-deps/ClangScanDeps.cpp
+++ clang/tools/clang-scan-deps/ClangScanDeps.cpp
@@ -187,6 +187,13 @@
"dependencies are to be computed."),
 llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt P1689MakeformatOutputpath(
+"p1689-makeformat-output", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689. Print the make-style dependency "
+   "output to the specified output. This is a helper for build "
+   "systems to do duplicate scanning."),
+llvm::cl::cat(DependencyScannerCategory));
+
 llvm::cl::opt P1689TargettedCommand(
 llvm::cl::Positional, llvm::cl::ZeroOrMore,
 llvm::cl::desc("The command line flags for the target of which "
@@ -584,6 +591,19 @@
   return std::move(FixedCompilationDatabase);
 }
 
+static raw_ostream () {
+  if (!P1689MakeformatOutputpath.empty()) {
+std::error_code EC;
+static llvm::raw_fd_ostream OS(P1689MakeformatOutputpath, EC);
+if (EC)
+  llvm::errs() << "Failed to open P1689 make format output file \""
+   << P1689MakeformatOutputpath << "\" for " << EC.message()
+   << "\n";
+return OS;
+  }
+  return llvm::outs();
+}
+
 int main(int argc, const char **argv) {
   std::string ErrorMessage;
   std::unique_ptr Compilations =
@@ -662,7 +682,7 @@
 
   SharedStream Errs(llvm::errs());
   // Print out the dependency results to STDOUT by default.
-  SharedStream DependencyOS(llvm::outs());
+  SharedStream DependencyOS(getDependencyOS());
 
   DependencyScanningService Service(ScanMode, Format, OptimizeArgs,
 EagerLoadModules);
@@ -722,10 +742,20 @@
  Errs))
 HadErrors = true;
 } else if (Format == ScanningOutputFormat::P1689) {
+  llvm::Optional MakeformatOutput;
+  if (!P1689MakeformatOutputpath.empty())
+MakeformatOutput.emplace();
+
   auto MaybeRule = WorkerTools[I]->getP1689ModuleDependencyFile(
-  *Input, CWD, MaybeModuleName);
-  if (handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs))
-HadErrors = true;
+  *Input, CWD, MakeformatOutput, MaybeModuleName);
+  HadErrors =
+  handleP1689DependencyToolResult(Filename, MaybeRule, PD, Errs);
+
+  if (MakeformatOutput && !MakeformatOutput->empty() && !HadErrors) {
+llvm::Expected MaybeOutput(*MakeformatOutput);
+HadErrors = handleMakeDependencyToolResult(Filename, MaybeOutput,
+   DependencyOS, Errs);
+  }
 } else if (DeprecatedDriverCommand) {
   auto MaybeFullDeps =
   WorkerTools[I]->getFullDependenciesLegacyDriverCommand(
Index: clang/test/ClangScanDeps/P1689.cppm
===
--- clang/test/ClangScanDeps/P1689.cppm
+++ clang/test/ClangScanDeps/P1689.cppm
@@ -22,6 +22,14 @@
 // RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/User.cpp --p1689-targeted-output=%t/User.o \
 // RUN:   -- -std=c++20 -c \
 // RUN:   | FileCheck %t/User.cpp -DPREFIX=%/t
+//
+// Check we can generate the make-style dependencies as expected.
+// RUN: clang-scan-deps --compilation-database %t/P1689.json -format=p1689 --p1689-makeformat-output=%t/P1689.dep
+// RUN: cat %t/P1689.dep | FileCheck %t/Checks.cpp -DPREFIX=%/t --check-prefix=CHECK-MAKE
+//
+// RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/impl_part.cppm --p1689-targeted-output=%t/impl_part.o \
+// RUN:   --p1689-makeformat-output=%t/impl_part.dep -- -std=c++20 -c
+// RUN: cat %t/impl_part.dep | FileCheck %t/impl_part.cppm -DPREFIX=%/t --check-prefix=CHECK-MAKE
 
 //--- P1689.json.in
 [
@@ -57,7 +65,6 @@
 }
 ]
 
-
 //--- M.cppm
 export module M;
 export import :interface_part;
@@ -145,6 +152,10 @@
 // CHECK-NEXT:   "version": 1
 // CHECK-NEXT: }