[C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/4)

[ChuanqiXu]

Summary

In a private chat with @ben.boeckel , we get in consensus it would be great for cmake if the invocation of clang-scan-deps can get rid of compilation database. Due to the compilation database can't do very well for the files which are not existed yet. @ben.boeckel may have more context to add here.

This patch should be innocent for others usages.

Documents and ReleaseNotes would be added later if this get accepted.

[ben.boeckel]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

201

I'm not sure what this means. Is it the compiler's working directory when compiling?

206

Can this be something like -- so that I don't have to figure out how to quote the thing (for the shell and whatever parsing Clang does internally)?

[ChuanqiXu]

Use -- instead - for parameters in tests to avoid confusion.

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

201

In fact, the option is not used actually for P1689 now. The idea of the patch is to construct a compilation database from these 4 command line options. And the existence for the option is the placeholder and for the consistency. So you can fill the value as if you (or cmake) are generating a compilation database entry for this.

206

Yeah, it can. Both - and -- are accepted. I've updated the test to disambiguate.

[ben.boeckel]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

I don't mean the flag using -- as a prefix. I don't care about that. What I *do* care about is having to quote everything I'd give to clang here. I'd vastly prefer something like:

clang-scan-deps -p1689-targeted-file-name=… -p1689-use-command -- -flags --for ---clang --go --here

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

I got your point. But I prefer the current style if it won't be a problem for you to quote the options. In my imagination, it would be easier for the build systems to quote the flags than we synthesis things here. I guess there should already be one existing command line in the build system. And I feel like the current style may be more convenient and friendly for other tools to use. Could you try to use the current style?

[ben.boeckel]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

I don't think CMake has a mechanism for that at all right now. The problem is knowing how to quote things. Is it specified how Clang will parse this string into arguments? Is it platform-dependent?

[ben.boeckel]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

Ah, it is the same as the compilation database parsing.

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

Yeah, given cmake can generate compilation database for a long time. So I think it should be easy for cmake to fill the value.

[ChuanqiXu]

Address comments. Parse flags after "--" as the command line argument for compilation database.

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

206

@ben.boeckel hi, I found that there is existing component to parse the flags after "--" as the input for compilation database. So I can avoid to do too many works. Could you visit if the current interface good for you?

[ben.boeckel]

So a few things I see when trying to put this into CMake:

• P1689 output is only to stdout?
• Is there a way to get -MF output for the files read during scanning? This is useful to know that "header X changed, scanning may have changed, so please rerun scanning".

Other than that, I have success with CMake's test suite with this patchset as well as the patchset of D137059.

[ChuanqiXu]

In D137534#3965012, @ben.boeckel wrote:

So a few things I see when trying to put this into CMake:

• P1689 output is only to stdout?
• Is there a way to get -MF output for the files read during scanning? This is useful to know that "header X changed, scanning may have changed, so please rerun scanning".

Other than that, I have success with CMake's test suite with this patchset as well as the patchset of D137059.

Great to hear that!

• P1689 output is only to stdout?

From the current implementation, it looks true that the output of clang-scan-deps is only to stdout. I guess it may not be problem since we can redirect it to the file we want. Or is it necessary to specify an output file in the flags?

Is there a way to get -MF output for the files read during scanning? This is useful to know that "header X changed, scanning may have changed, so please rerun scanning".

I sent https://reviews.llvm.org/D139168 to address. You can specify the output of -MF in --p1689-makeformat-output= flags. Is this satisfying?

[jansvoboda11]

Why is it necessary to add new command-line flags for this? Can't the input and output be inherited from the specified Clang command line? Would such command line make sense?

clang/test/ClangScanDeps/P1689.cppm

10

What does "separated" mean in this context?

[ben.boeckel]

clang/test/ClangScanDeps/P1689.cppm

10

Yeah, this isn't the right term. There are two things being done:

• discovering dependencies for a future compile (P1689)
• collecting deps for the scanning itself to know that "if included file X changes, I need to rescan"

Both are required for correct builds.

[ChuanqiXu]

Why is it necessary to add new command-line flags for this? Can't the input and output be inherited from the specified Clang command line? Would such command line make sense?

CMake wants to query the dependency information for a single file from time to time due to its current structure. And according to @ben.boeckel , the compilation database can't do very well for the files which don't exist during the configuration time. (Maybe @ben.boeckel can add some additional information).

For the reason why we need --p1689-targeted-file-name and --p1689-targeted-output is that FixedCompilationDatabase wouldn't generate the input and output entry from the command line. See the inline comments for example. I feel it is easier and simpler to add 2 flags for it. I add the prefix -p1689 to tell all other users to not use it unintentionally.

clang/lib/Tooling/CompilationDatabase.cpp

376–378 ↗

(On Diff #479168)

Here: we can find the file path and the output entry are left empty for the FixedCompilationDatabase.

[ben.boeckel]

In D137534#4014571, @ChuanqiXu wrote:

Why is it necessary to add new command-line flags for this? Can't the input and output be inherited from the specified Clang command line? Would such command line make sense?

CMake wants to query the dependency information for a single file from time to time due to its current structure. And according to @ben.boeckel , the compilation database can't do very well for the files which don't exist during the configuration time. (Maybe @ben.boeckel can add some additional information).

A special compilation database would be required for scanning because the "real" command lines have @rspfile which CMake writes *during the build* with module dependency information (where to put the .pcm from this compile and where anything imported actually lives). Since this file is non-existent in a fresh build and potentially out-of-date after that, CMake would need to write a compilation database just for scanning purposes. Note that this would mean that any change to any command line would necessitate scanning *all* TUs sharing that database because none of them know if their command line is the one that changed. I feel this is a pessimization in the general case (batch scanning may be better for scratch builds, but measurements need to be made).

For the reason why we need --p1689-targeted-file-name and --p1689-targeted-output is that FixedCompilationDatabase wouldn't generate the input and output entry from the command line. See the inline comments for example. I feel it is easier and simpler to add 2 flags for it. I add the prefix -p1689 to tell all other users to not use it unintentionally.

My GCC patch names it -fdepfile-output= instead of --p1689-targeted-output, gets --p1689-targeted-file-name from the regular command line (it looks like a preprocessor command overall) and the --p1689-makeformat-output flag is handled by the normal -MT and -MF flags. While I'm not thrilled with the P1689 naming in the flags (as it'll become P2222 once the TR it's heading for is accepted, aliases should be simple to maintain. My GCC patch uses -fdep-format=p1689r5 so a rename there is easier than a whole flag name. But I suspect that clang-scan-deps is much more of a "tooling's tool" than something people type out by hand, so it's probably not a big deal?

[ChuanqiXu]

Address comments: fix a typo.

[ChuanqiXu]

My GCC patch names it -fdepfile-output= instead of --p1689-targeted-output, gets --p1689-targeted-file-name from the regular command line (it looks like a preprocessor command overall) and the --p1689-makeformat-output flag is handled by the normal -MT and -MF flags. While I'm not thrilled with the P1689 naming in the flags (as it'll become P2222 once the TR it's heading for is accepted, aliases should be simple to maintain. My GCC patch uses -fdep-format=p1689r5 so a rename there is easier than a whole flag name. But I suspect that clang-scan-deps is much more of a "tooling's tool" than something people type out by hand, so it's probably not a big deal?

Agreed. I suggest to keep the current name in the process of reviewing if @jansvoboda11 don't feel unhappy.

[jansvoboda11]

While I was not suggesting using compilation database instead of the approach taken by this patch, I appreciate your explanation. But it still leaves me wondering whether generating one compilation database per file-to-be-scanned would be a reasonable strategy? That might help us avoid increasing the complexity of the clang-scan-deps command-line interface.

If we really want to avoid writing compilation databases, I think that clang-scan-deps -format=p1689 -- <clang_flags> -std=c++20 <input> -o <output> is much cleaner/generic/reusable than clang-scan-deps -format=p1689 --p1689-targeted-file-name=<input> --p1689-targeted-output=<output> -- <clang_flags> -std=c++20. I understand that FixedCompilationDatabase currently doesn't know what the input/output is unless you explicitly pass that in. What if we were able to construct some kind of CompilationDatabase by properly parsing the given complete command line into CompilerInvocation and poking at FrontendOptions?

clang/lib/Tooling/CompilationDatabase.cpp

378 ↗

(On Diff #485022)

I don't think this is correct in cases where the command line already contains an input, or is malformed. As an example, if the last element is an option that expects a value (e.g. "-ferror-limit"), appending FilePath will trigger diagnostics in the driver that the argument to -ferror-limit is not numeric and that an input needs to be specified. While the correct/expected behavior would be to emit single diagnostic complaining about missing value for the last option.

Clang's tooling is littered with ad-hoc command-line parsing/manipulation and it's rarely handling all the edge-cases. I'd prefer this complexity lived somewhere outside of Clang, in the tool that actually constructed the command line in the first place.

[ben.boeckel]

In D137534#4024022, @jansvoboda11 wrote:

While I was not suggesting using compilation database instead of the approach taken by this patch, I appreciate your explanation. But it still leaves me wondering whether generating one compilation database per file-to-be-scanned would be a reasonable strategy? That might help us avoid increasing the complexity of the clang-scan-deps command-line interface.

My preferred solution is to just use clang directly, but this is where we ended up. A database file would still require -MF flags for the scan itself (unless it expects us to "lie" about the command line that we'll actually use…which is already true since we'll have to skip the @module_info file as it either won't exist or may be out-of-date). We also need to communicate:

• the file the scanning output should go to
• if -MF isn't "stolen" from the "lying" command line, we'll need those flags too

I guess I don't see the benefit of writing out all of these JSON files dealing with escaping and command line parsing when we can just communicate in the same language we do with the compiler anyways (the command line) instead of communicating through a translator (JSON).

If we really want to avoid writing compilation databases, I think that clang-scan-deps -format=p1689 -- <clang_flags> -std=c++20 <input> -o <output> is much cleaner/generic/reusable than clang-scan-deps -format=p1689 --p1689-targeted-file-name=<input> --p1689-targeted-output=<output> -- <clang_flags> -std=c++20. I understand that FixedCompilationDatabase currently doesn't know what the input/output is unless you explicitly pass that in. What if we were able to construct some kind of CompilationDatabase by properly parsing the given complete command line into CompilerInvocation and poking at FrontendOptions?

You still need to know where to put the P1689 output and where the make-style dependency info for files read during the scan need to go. I see these as distinctly clang-scan-deps flags and should be *before* the --. Basically, it seems weird to me to have the clang flags end up telling the scanner where to put the -MF info (as it is now, but you want even fewer flags which I don't think is workable as the information just isn't there).

[tschuett]

I believe you are arguing for a JSON file. The complexity of flags that want to pass to clang-scan-deps is too high. You even get some benefit of caching if you create a JSON file with several targets.

[jansvoboda11]

In D137534#4024178, @ben.boeckel wrote:

In D137534#4024022, @jansvoboda11 wrote:

While I was not suggesting using compilation database instead of the approach taken by this patch, I appreciate your explanation. But it still leaves me wondering whether generating one compilation database per file-to-be-scanned would be a reasonable strategy? That might help us avoid increasing the complexity of the clang-scan-deps command-line interface.

My preferred solution is to just use clang directly, but this is where we ended up. A database file would still require -MF flags for the scan itself (unless it expects us to "lie" about the command line that we'll actually use…which is already true since we'll have to skip the @module_info file as it either won't exist or may be out-of-date). We also need to communicate:

• the file the scanning output should go to
• if -MF isn't "stolen" from the "lying" command line, we'll need those flags too

I guess I don't see the benefit of writing out all of these JSON files dealing with escaping and command line parsing when we can just communicate in the same language we do with the compiler anyways (the command line) instead of communicating through a translator (JSON).

That makes sense to me, thanks.

If we really want to avoid writing compilation databases, I think that clang-scan-deps -format=p1689 -- <clang_flags> -std=c++20 <input> -o <output> is much cleaner/generic/reusable than clang-scan-deps -format=p1689 --p1689-targeted-file-name=<input> --p1689-targeted-output=<output> -- <clang_flags> -std=c++20. I understand that FixedCompilationDatabase currently doesn't know what the input/output is unless you explicitly pass that in. What if we were able to construct some kind of CompilationDatabase by properly parsing the given complete command line into CompilerInvocation and poking at FrontendOptions?

You still need to know where to put the P1689 output and where the make-style dependency info for files read during the scan need to go. I see these as distinctly clang-scan-deps flags and should be *before* the --. Basically, it seems weird to me to have the clang flags end up telling the scanner where to put the -MF info (as it is now, but you want even fewer flags which I don't think is workable as the information just isn't there).

I'm getting confused by the semantics of --p1689-targeted-output=. In D137527, it's used to populate CompilerCommand::Output which is supposed to be the main compiler output (.o file). Is that correct? If that's not the case and this flag is supposed to only control where to put the JSON file with the dependency information, it might as well have more generic name, like -o or --output and apply to the rest of clang-scan-deps, no?

[ChuanqiXu]

I'm getting confused by the semantics of --p1689-targeted-output=. In D137527, it's used to populate CompilerCommand::Output which is supposed to be the main compiler output (.o file). Is that correct? If that's not the case and this flag is supposed to only control where to put the JSON file with the dependency information, it might as well have more generic name, like -o or --output and apply to the rest of clang-scan-deps, no?

Correct. The semantics of --p1689-targeted-output= should be the same with the main compiler output (.o file).

If we really want to avoid writing compilation databases, I think that clang-scan-deps -format=p1689 -- <clang_flags> -std=c++20 <input> -o <output> is much cleaner/generic/reusable than clang-scan-deps -format=p1689 --p1689-targeted-file-name=<input> --p1689-targeted-output=<output> -- <clang_flags> -std=c++20. I understand that FixedCompilationDatabase currently doesn't know what the input/output is unless you explicitly pass that in. What if we were able to construct some kind of CompilationDatabase by properly parsing the given complete command line into CompilerInvocation and poking at FrontendOptions?

I was worrying about the complexity and I wanted to reuse FixedCompilationDatabase. I'd like to see if we can implement it simply.

You still need to know where to put the P1689 output and where the make-style dependency info for files read during the scan need to go. I see these as distinctly clang-scan-deps flags and should be *before* the --. Basically, it seems weird to me to have the clang flags end up telling the scanner where to put the -MF info (as it is now, but you want even fewer flags which I don't think is workable as the information just isn't there).

Do you think it is OK to write: clang-scan-deps -format=p1689 -- clang++ -std=c++20 -MD -MT <target_name> -MF <dep_file> -c input_file <any_other_needed_opts> -o output_file?

[ChuanqiXu]

Address comments from @jansvoboda11: use the style of clang-scan-deps -format=p1689 -- <clang_flags> -std=c++20 <input> -o <output> instead of clang-scan-deps -format=p1689 --p1689-targeted-file-name=<input> --p1689-targeted-output=<output> -- <clang_flags> -std=c++20.

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

691–701

Here is the corresponding code: https://github.com/llvm/llvm-project/blob/0e11d65a58da32311b562ecea2b5ba9d4d655659/clang/lib/Frontend/CreateInvocationFromCommandLine.cpp#L39-L43. There is another FIXME.

Simply, we will lost the output file by using createInvocation and I feel it is not easy to fix. Then I feel it is not too bad to parse -o manually here if we document it clearly. I understand the current style will miss some cases like using -output=, -output instead of -o or missing -o. But I don't feel too bad for it.

[ChuanqiXu]

clang/tools/clang-scan-deps/ClangScanDeps.cpp

691–701

createInvocation will be helpful to recognize many cases, e.g., -MF in https://reviews.llvm.org/D139168. I feel it'll be worse if we don't use createInvocation .

[ben.boeckel]

In D137534#4024249, @jansvoboda11 wrote:

I'm getting confused by the semantics of --p1689-targeted-output=. In D137527, it's used to populate CompilerCommand::Output which is supposed to be the main compiler output (.o file). Is that correct? If that's not the case and this flag is supposed to only control where to put the JSON file with the dependency information, it might as well have more generic name, like -o or --output and apply to the rest of clang-scan-deps, no?

So the scanner has a few jobs:

• given this command line for this object file (we do not think in terms of source files because object files are unique; source files can be reused with different flags), please write out a P1689 file describing the module requirements and any module(s) provided (plural because Fortran supports multiple modules in a single source file and unity builds can also do this though I have no idea how likely unity module builds is ever going to be)
• the format supports doing this for a set of object files (but given the way it tangles the dep graph, is unlikely to be a perf win for incremental/developer builds; CI may prefer it, but that can be future work)
• because it is a rule that can itself read extra files that affect the scanning; this is the -MF-style output so that make/ninja can know "oh, frabnitz.h changed, it can affect the scan results in glom.ddi, so I will rescan")

Note that multi-arch builds are an open question (just like multi-arch .pcm files are I believe); the format could support it as two reports on provides/reqs, but the merged object file complicates things.

The object can be obtained from the -o on the command line, but the rest is "lying" if it is extracted from the clang command line and not given to clang-scan-deps directly. This paper may be useful for the overall strategy being used here: https://mathstuf.fedorapeople.org/fortran-modules/fortran-modules.html

[ChuanqiXu]

Note that multi-arch builds are an open question (just like multi-arch .pcm files are I believe); the format could support it as two reports on provides/reqs, but the merged object file complicates things.

BTW, we decide to forbid multiple-arch .pcm files in https://reviews.llvm.org/D137058 now since we don't know how to handle it properly.

[ChuanqiXu]

gentle ping @jansvoboda11 . sorry for the a little bit hurried ping since LLVM-16 is going to be branched in the 24th of January.

[jansvoboda11]

clang/test/ClangScanDeps/P1689.cppm

10

@ChuanqiXu Can you write up a better comment here?

clang/tools/clang-scan-deps/ClangScanDeps.cpp

691–701

This is again doing ad-hoc parsing of the command-line. clang-cl accepts output file path in the /o argument, which this code will not recognize. You should be able to use CompilerInvocation::CreateFromArgs() to avoid having the -fsyntax-only option injected.

[jansvoboda11]

In D137534#4025898, @ben.boeckel wrote:

• the format supports doing this for a set of object files (but given the way it tangles the dep graph, is unlikely to be a perf win for incremental/developer builds; CI may prefer it, but that can be future work)

Another thing to be aware of is that the scanner is tuned for scanning multiple TUs. Single clang-scan-deps invocation maintains a shared in-memory cache of the filesystem between its threads for all TUs it's given. This means invoking clang-scan-deps once for each TU is not as efficient as it could be. At Apple, we only use clang-scan-deps for the in-tree tests. In production, we actually wrap the C++ interface and expose a libclang API that is able to take advantage of caching to improve performance. To be honest, I'm surprised clang-scan-deps is being integrated into build systems as-is, especially without utilizing the cache. How's the performance looking for larger projects?

• because it is a rule that can itself read extra files that affect the scanning; this is the -MF-style output so that make/ninja can know "oh, frabnitz.h changed, it can affect the scan results in glom.ddi, so I will rescan")

I see. So the P1689 output is the primary scanner output, but you're also relying on emitting .d files to track the actual FS dependencies.

The object can be obtained from the -o on the command line, but the rest is "lying" if it is extracted from the clang command line and not given to clang-scan-deps directly.

I see your point. But since clang-scan-deps is built around reusing the same FS cache for scanning multiple TUs, you'd need to specify these arguments (that we currently extract from Clang command line) for all of those TUs. That's not very convenient, neither through the command line nor via a separate config file.

clang/test/ClangScanDeps/P1689.cppm

11

I'm fairly happy with the clang-scan-deps interface now (besides the performance aspect mentioned in another comment).

@ChuanqiXu what would happen if you run this Clang command line directly?

[ben.boeckel]

In D137534#4037064, @jansvoboda11 wrote:

Another thing to be aware of is that the scanner is tuned for scanning multiple TUs. Single clang-scan-deps invocation maintains a shared in-memory cache of the filesystem between its threads for all TUs it's given. This means invoking clang-scan-deps once for each TU is not as efficient as it could be. At Apple, we only use clang-scan-deps for the in-tree tests. In production, we actually wrap the C++ interface and expose a libclang API that is able to take advantage of caching to improve performance. To be honest, I'm surprised clang-scan-deps is being integrated into build systems as-is, especially without utilizing the cache. How's the performance looking for larger projects?

I originally investigated (and ended up lost) for something like GCC where P1689 information is extracted via -E -fdep-file=p1689.json -fdep-output=module.o -fdep-format=p1689 (which is "abusing" -E, but works), but using clang-scan-deps was where we ended up after a more-knowledgeable LLVM developer took over knowing what needed to be done.

• because it is a rule that can itself read extra files that affect the scanning; this is the -MF-style output so that make/ninja can know "oh, frabnitz.h changed, it can affect the scan results in glom.ddi, so I will rescan")

I see. So the P1689 output is the primary scanner output, but you're also relying on emitting .d files to track the actual FS dependencies.

Yes, that's exactly it. It'd be great if *every* command had -MF-style information for more accurate builds, but I'll roll that rock up another hill some other day.

The object can be obtained from the -o on the command line, but the rest is "lying" if it is extracted from the clang command line and not given to clang-scan-deps directly.

I see your point. But since clang-scan-deps is built around reusing the same FS cache for scanning multiple TUs, you'd need to specify these arguments (that we currently extract from Clang command line) for all of those TUs. That's not very convenient, neither through the command line nor via a separate config file.

While batch scanning is probably better for one-shot (basically, CI) builds, I suspect the excess work during development/incremental builds will cause that to "lose" over a long enough time span.

[ChuanqiXu]

Address comments:

• Add more comments in the test.
• Avoid ad-hoc parsing -o option manually.

[ChuanqiXu]

I originally investigated (and ended up lost) for something like GCC where P1689 information is extracted via -E -fdep-file=p1689.json -fdep-output=module.o -fdep-format=p1689 (which is "abusing" -E, but works), but using clang-scan-deps was where we ended up after a more-knowledgeable LLVM developer took over knowing what needed to be done.

Yeah, I believe the clang-scan-deps would be the better option and according to the talk of clang-scan-deps (https://llvm.org/devmtg/2019-04/slides/TechTalk-Lorenz-clang-scan-deps_Fast_dependency_scanning_for_explicit_modules.pdf), it is also the goal of clang-scan-deps to support c++20 modules.

clang/test/ClangScanDeps/P1689.cppm

10

I've updated the comments to tell it is required for non-exist and potentially out-of-date files.

11

what would happen if you run this Clang command line directly?

I remember this refers to %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/M.cppm -o %t/M.o previously.

And if we run this clang command line directly, it will compile the module-unit M.cppm into object files if all its dependent precompiled module files lives in the directory %t. The command line should be what will run actually.

[ChuanqiXu]

Oh, I forgot to submit the inline comments. My bad.

[ChuanqiXu]

@jansvoboda11 gentle ping~ (although I feel it may be hard to land this before the branching since I'll take a vacation in 19th until February, @h-vetinari said it should be OK to backport these patches to branched 16.x.)

[h-vetinari]

In D137534#4055283, @ChuanqiXu wrote:

@h-vetinari said it should be OK to backport these patches to branched 16.x.

That should be considered a comment from the peanut gallery, not a definite statement. What I had said in https://reviews.llvm.org/D137058 was:

[...] but I am not sure if we can land them in time. I guess it may be possible to backport these patches to 16.x since it is relatively important?

Might make sense tagging the release managers if things get tight - perhaps it's possible. E.g. ranges for LLVM 15 was also finished after the branch.

[ChuanqiXu]

In D137534#4055286, @h-vetinari wrote:

In D137534#4055283, @ChuanqiXu wrote:

@h-vetinari said it should be OK to backport these patches to branched 16.x.

That should be considered a comment from the peanut gallery, not a definite statement. What I had said in https://reviews.llvm.org/D137058 was:

[...] but I am not sure if we can land them in time. I guess it may be possible to backport these patches to 16.x since it is relatively important?

Might make sense tagging the release managers if things get tight - perhaps it's possible. E.g. ranges for LLVM 15 was also finished after the branch.

Sorry for the misleading and thanks for the quick clarification. So it looks like the status quo is a little bit worse than I imaged...

[h-vetinari]

Sorry for the misleading and thanks for the quick clarification. So it looks like the status quo is a little bit worse than I imaged...

I opened a thread on discourse for more visibility: https://discourse.llvm.org/t/land-c-modules-changes-required-for-cmake-before-llvm-16-branch/67717

[ChuanqiXu]

In D137534#4055323, @h-vetinari wrote:

Sorry for the misleading and thanks for the quick clarification. So it looks like the status quo is a little bit worse than I imaged...

I opened a thread on discourse for more visibility: https://discourse.llvm.org/t/land-c-modules-changes-required-for-cmake-before-llvm-16-branch/67717

Thanks for chime in!

[jansvoboda11]

LGTM.

clang/test/ClangScanDeps/P1689.cppm

 // RUN: rm -fr %t
 // RUN: mkdir -p %t
 // RUN: split-file %s %t
 //
 // RUN: sed "s|DIR|%/t|g" %t/P1689.json.in > %t/P1689.json
 // RUN: clang-scan-deps -compilation-database %t/P1689.json -format=p1689 | FileCheck %t/Checks.cpp -DPREFIX=%/t
 // RUN: clang-scan-deps --mode=preprocess-dependency-directives -compilation-database %t/P1689.json -format=p1689 | FileCheck %t/Checks.cpp -DPREFIX=%/t
+//
+// Check the separated dependency format. This is required by CMake for the case
+// that we have non-exist files in a fresh build and potentially out-of-date after that.
+// So the build system need to wrtie a compilation database just for scanning purposes,
+// which is not so good. So here is the per file mode for P1689.
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/M.cppm -o %t/M.o \
+// RUN:   | FileCheck %t/M.cppm -DPREFIX=%/t
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/Impl.cpp -o %t/Impl.o \
+// RUN:   | FileCheck %t/Impl.cpp -DPREFIX=%/t
+// 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:   | FileCheck %t/impl_part.cppm -DPREFIX=%/t
+// RUN: clang-scan-deps -format=p1689 \
+// RUN:   -- %clang++ -std=c++20 -c -fprebuilt-module-path=%t %t/interface_part.cppm -o %t/interface_part.o \
+// RUN:   | FileCheck %t/interface_part.cppm -DPREFIX=%/t
+// 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
 
 //--- P1689.json.in
 [
 {
   "directory": "DIR",
   "command": "clang++ -std=c++20 DIR/M.cppm -c -o DIR/M.o",
   "file": "DIR/M.cppm",
   "output": "DIR/M.o"

 
 
 //--- M.cppm
 export module M;
 export import :interface_part;
 import :impl_part;
 export void Hello();
 
+// CHECK: {
+// CHECK-NEXT:   "revision": 0,
+// CHECK-NEXT:   "rules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "primary-output": "[[PREFIX]]/M.o",
+// CHECK-NEXT:       "provides": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "is-interface": true,
+// CHECK-NEXT:           "logical-name": "M",
+// CHECK-NEXT:           "source-path": "[[PREFIX]]/M.cppm"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "requires": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "M:interface_part"
+// CHECK-NEXT:         },
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "M:impl_part"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ]
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "version": 1
+// CHECK-NEXT: }
+
 //--- Impl.cpp
 module;
 #include "header.mock"
 module M;
 void Hello() {
     std::cout << "Hello ";
 }
 
+// CHECK: {
+// CHECK-NEXT:   "revision": 0,
+// CHECK-NEXT:   "rules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "primary-output": "[[PREFIX]]/Impl.o",
+// CHECK-NEXT:       "requires": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "M"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ]
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "version": 1
+// CHECK-NEXT: }
+
 //--- impl_part.cppm
 module;
 #include "header.mock"
 module M:impl_part;
 import :interface_part;
 
 std::string W = "World.";
 void World() {
     std::cout << W << std::endl;
 }
 
+// CHECK: {
+// CHECK-NEXT:   "revision": 0,
+// CHECK-NEXT:   "rules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "primary-output": "[[PREFIX]]/impl_part.o",
+// CHECK-NEXT:       "provides": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "is-interface": false,
+// CHECK-NEXT:           "logical-name": "M:impl_part",
+// CHECK-NEXT:           "source-path": "[[PREFIX]]/impl_part.cppm"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ],
+// CHECK-NEXT:       "requires": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "M:interface_part"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ]
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// 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();
 
+// CHECK: {
+// CHECK-NEXT:   "revision": 0,
+// CHECK-NEXT:   "rules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "primary-output": "[[PREFIX]]/interface_part.o",
+// CHECK-NEXT:       "provides": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "is-interface": true,
+// CHECK-NEXT:           "logical-name": "M:interface_part",
+// CHECK-NEXT:           "source-path": "[[PREFIX]]/interface_part.cppm"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ]
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "version": 1
+// CHECK-NEXT: }
+
 //--- User.cpp
 import M;
 import third_party_module;
 int main() {
     Hello();
     World();
     return 0;
 }
 
+// CHECK: {
+// CHECK-NEXT:   "revision": 0,
+// CHECK-NEXT:   "rules": [
+// CHECK-NEXT:     {
+// CHECK-NEXT:       "primary-output": "[[PREFIX]]/User.o",
+// CHECK-NEXT:       "requires": [
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "M"
+// CHECK-NEXT:         },
+// CHECK-NEXT:         {
+// CHECK-NEXT:           "logical-name": "third_party_module"
+// CHECK-NEXT:         }
+// CHECK-NEXT:       ]
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ],
+// CHECK-NEXT:   "version": 1
+// CHECK-NEXT: }
+
 //--- Checks.cpp
 // CHECK: {
 // CHECK-NEXT:   "revision": 0,
 // CHECK-NEXT:   "rules": [
 // CHECK-NEXT:     {
 // CHECK-NEXT:       "primary-output": "[[PREFIX]]/Impl.o",
 // CHECK-NEXT:       "requires": [
 // CHECK-NEXT:         {

clang/tools/clang-scan-deps/ClangScanDeps.cpp

 //===- ClangScanDeps.cpp - Implementation of clang-scan-deps --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningService.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningTool.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
 #include "clang/Tooling/JSONCompilationDatabase.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileUtilities.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/JSON.h"
 #include "llvm/Support/Program.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
 #include <mutex>
 #include <optional>

 llvm::cl::opt<unsigned>
     NumThreads("j", llvm::cl::Optional,
                llvm::cl::desc("Number of worker threads to use (default: use "
                               "all concurrent threads)"),
                llvm::cl::init(0), llvm::cl::cat(DependencyScannerCategory));
 
 llvm::cl::opt<std::string>
     CompilationDB("compilation-database",
-                  llvm::cl::desc("Compilation database"), llvm::cl::Required,
+                  llvm::cl::desc("Compilation database"), llvm::cl::Optional,
                   llvm::cl::cat(DependencyScannerCategory));
 
+llvm::cl::opt<std::string> P1689TargettedCommand(
+    llvm::cl::Positional, llvm::cl::ZeroOrMore,
+    llvm::cl::desc("The command line flags for the target of which "
+                   "the dependencies are to be computed."));
+
 llvm::cl::opt<std::string> ModuleName(
     "module-name", llvm::cl::Optional,
     llvm::cl::desc("the module of which the dependencies are to be computed"),
     llvm::cl::cat(DependencyScannerCategory));
 
 llvm::cl::list<std::string> ModuleDepTargets(
     "dependency-target",
     llvm::cl::desc("The names of dependency targets for the dependency file"),
     llvm::cl::cat(DependencyScannerCategory));
 
 enum ResourceDirRecipeKind {
   RDRK_ModifyCompilerPath,
   RDRK_InvokeCompiler,
 };
 
 static llvm::cl::opt<ResourceDirRecipeKind> ResourceDirRecipe(
     "resource-dir-recipe",
     llvm::cl::desc("How to produce missing '-resource-dir' argument"),
     llvm::cl::values(
         clEnumValN(RDRK_ModifyCompilerPath, "modify-compiler-path",
                    "Construct the resource directory from the compiler path in "
                    "the compilation database. This assumes it's part of the "
                    "same toolchain as this clang-scan-deps. (default)"),
         clEnumValN(RDRK_InvokeCompiler, "invoke-compiler",
                    "Invoke the compiler with '-print-resource-dir' and use the "
                    "reported path as the resource directory. (deprecated)")),
     llvm::cl::init(RDRK_ModifyCompilerPath),
     llvm::cl::cat(DependencyScannerCategory));
 
 #ifndef NDEBUG
 static constexpr bool DoRoundTripDefault = true;
 #else
 static constexpr bool DoRoundTripDefault = false;
for (StringRef Arg : llvm::reverse(Args)) {
     if (Arg.consume_front("-fmodules-cache-path="))
       return std::string(Arg);
   }
   SmallString<128> Path;
   driver::Driver::getDefaultModuleCachePath(Path);
   return std::string(Path);
 }
 
-int main(int argc, const char **argv) {
+// getCompilationDataBase - If -compilation-database is set, load the
+// compilation database from the specified file. Otherwise if the we're
+// generating P1689 format, trying to generate the compilation database
+// form specified command line after the positional parameter "--".
+static std::unique_ptr<tooling::CompilationDatabase>
+getCompilationDataBase(int argc, const char **argv, std::string &ErrorMessage) {
   llvm::InitLLVM X(argc, argv);
   llvm::cl::HideUnrelatedOptions(DependencyScannerCategory);
   if (!llvm::cl::ParseCommandLineOptions(argc, argv))
-    return 1;
+    return nullptr;
 
-  std::string ErrorMessage;
-  std::unique_ptr<tooling::JSONCompilationDatabase> Compilations =
-      tooling::JSONCompilationDatabase::loadFromFile(
-          CompilationDB, ErrorMessage,
-          tooling::JSONCommandLineSyntax::AutoDetect);
+  if (!CompilationDB.empty())
+    return tooling::JSONCompilationDatabase::loadFromFile(
+
+  if (Format != ScanningOutputFormat::P1689) {
+    llvm::errs() << "the --compilation-database option: must be specified at "
+                    "least once!";
+    return nullptr;
+  }
+
+  // Trying to get the input file, the output file and the command line options
+  // from the positional parameter "--".
+  const char **DoubleDash = std::find(argv, argv + argc, StringRef("--"));
+  if (DoubleDash == argv + argc) {
+    llvm::errs() << "The command line arguments is required after '--' in "
+                    "P1689 per file mode.";
+    return nullptr;
+  }
+  std::vector<const char *> CommandLine(DoubleDash + 1, argv + argc);
+
+  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions);
+  driver::Driver TheDriver(CommandLine[0], llvm::sys::getDefaultTargetTriple(),
+                           *Diags);
+  std::unique_ptr<driver::Compilation> C(
+      TheDriver.BuildCompilation(CommandLine));
+  if (!C)
+    return nullptr;
+
+  auto Cmd = C->getJobs().begin();
+  auto CI = std::make_unique<CompilerInvocation>();
+  CompilerInvocation::CreateFromArgs(*CI, Cmd->getArguments(), *Diags,
+                                     CommandLine[0]);
+  if (!CI)
+    return nullptr;
+
+  FrontendOptions &FEOpts = CI->getFrontendOpts();
+  if (FEOpts.Inputs.size() != 1) {
+    llvm::errs() << "Only one input file is allowed in P1689 per file mode.";
+    return nullptr;
+  }
+
+  // There might be multiple jobs for a compilation. Extract the specified
+  // output filename from the last job.
+  auto LastCmd = C->getJobs().end();
+  LastCmd--;
+  if (LastCmd->getOutputFilenames().size() != 1) {
+    llvm::errs() << "The command line should provide exactly one output file "
+                    "in P1689 per file mode.\n";
+  }
+  StringRef OutputFile = LastCmd->getOutputFilenames().front();
+
+  class InplaceCompilationDatabase : public tooling::CompilationDatabase {
+  public:
+    InplaceCompilationDatabase(StringRef InputFile, StringRef OutputFile,
+                               ArrayRef<const char *> CommandLine)
+        : Command(".", InputFile, {}, OutputFile) {
+      for (auto *C : CommandLine)
+        Command.CommandLine.push_back(C);
+    }
+
+    std::vector<tooling::CompileCommand>
+    getCompileCommands(StringRef FilePath) const override {
+      if (FilePath != Command.Filename)
+        return {};
+      return {Command};
+    }
+
+    std::vector<std::string> getAllFiles() const override {
+      return {Command.Filename};
+    }
+
+    std::vector<tooling::CompileCommand>
+    getAllCompileCommands() const override {
+      return {Command};
+    }
+
+  private:
+    tooling::CompileCommand Command;
+  };
+
+  return std::make_unique<InplaceCompilationDatabase>(
+      FEOpts.Inputs[0].getFile(), OutputFile, CommandLine);
+}
+
+int main(int argc, const char **argv) {
+  std::string ErrorMessage;
+  std::unique_ptr<tooling::CompilationDatabase> Compilations =
+      getCompilationDataBase(argc, argv, ErrorMessage);
   if (!Compilations) {
-    llvm::errs() << "error: " << ErrorMessage << "\n";
+    llvm::errs() << ErrorMessage << "\n";
     return 1;
   }
 
   llvm::cl::PrintOptionValues();
 
   // The command options are rewritten to run Clang in preprocessor only mode.
   auto AdjustingCompilations =
       std::make_unique<tooling::ArgumentsAdjustingCompilations>(
           std::move(Compilations));
   ResourceDirectoryCache ResourceDirCache;
 
   AdjustingCompilations->appendArgumentsAdjuster(
       [&ResourceDirCache](const tooling::CommandLineArguments &Args,
                           StringRef FileName) {