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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

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.


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

https://reviews.llvm.org/D137534

___
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)

[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)

[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)

[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] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/4)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

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


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

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 --  -std=c++20  -o 
> ` is much cleaner/generic/reusable than `clang-scan-deps 
> -format=p1689 --p1689-targeted-file-name= 
> --p1689-targeted-output= --  -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).


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

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 
P  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?


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/test/ClangScanDeps/P1689.cppm:9
+//
+// Check the seperated dependency format.
+// RUN: clang-scan-deps -format=p1689 --p1689-targeted-file-name=%t/M.cppm 
--p1689-targeted-output=%t/M.o \

jansvoboda11 wrote:
> What does "separated" mean in this context?
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.


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

https://reviews.llvm.org/D137534

___
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] D137059: [Driver] [Modules] Introduce -fsave-std-c++-module-file= to specify the path of the module file (2/2)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D137059#3934448 , @dblaikie wrote:

> I'm still curious what about the details of other compilers - I think from 
> the sounds of it, @iains suggested GCC doesn't support this yet so we'll need 
> to pick/name the flag ourselves & he's happy to implement whatever we pick? I 
> guess Microsoft's flag naming is sufficiently differently styled as to offer 
> no useful inspiration? Though wouldn't hurt to know what they name it.
>
> Any other examples you had in mind, Ben?

GCC supports naming the output file by asking the "module mapper" where a 
module with a given name lives (also used for finding imported modules). MSVC 
uses the `-ifcOutput` flag to specify where to write any exported module data 
to. See this CMake code which handles the "module mapping" for the various 
compilers: 
https://gitlab.kitware.com/cmake/cmake/-/blob/master/Source/cmCxxModuleMapper.cxx


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

https://reviews.llvm.org/D137059

___
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

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)

[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)

[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] D137534: [C++20] [Modules] [ClangScanDeps] Allow clang-scan-deps to without specified compilation database in P1689 (3/3)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

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 .


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197
+llvm::cl::opt P1689TargetedCommand(
+"p1689-targeted-command", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689, the targeted command of which "

ben.boeckel wrote:
> ChuanqiXu wrote:
> > ben.boeckel wrote:
> > > ChuanqiXu wrote:
> > > > ben.boeckel wrote:
> > > > > 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)?
> > > > Yeah, it can. Both `-` and `--` are accepted. I've updated the test to 
> > > > disambiguate.
> > > 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
> > > ```
> > 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?
> 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?
Ah, it is the same as the compilation database parsing.


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197
+llvm::cl::opt P1689TargetedCommand(
+"p1689-targeted-command", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689, the targeted command of which "

ChuanqiXu wrote:
> ben.boeckel wrote:
> > ChuanqiXu wrote:
> > > ben.boeckel wrote:
> > > > 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)?
> > > Yeah, it can. Both `-` and `--` are accepted. I've updated the test to 
> > > disambiguate.
> > 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
> > ```
> 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?
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?


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197
+llvm::cl::opt P1689TargetedCommand(
+"p1689-targeted-command", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689, the targeted command of which "

ChuanqiXu wrote:
> ben.boeckel wrote:
> > 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)?
> Yeah, it can. Both `-` and `--` are accepted. I've updated the test to 
> disambiguate.
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
```


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

https://reviews.llvm.org/D137534

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

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

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added inline comments.



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:192
+"p1689-targeted-directory", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689, the targeted directory of which "
+   "the dependencies are to be computed."),

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



Comment at: clang/tools/clang-scan-deps/ClangScanDeps.cpp:197
+llvm::cl::opt P1689TargetedCommand(
+"p1689-targeted-command", llvm::cl::Optional,
+llvm::cl::desc("Only supported for P1689, the targeted command of which "

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)?


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

https://reviews.llvm.org/D137534

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

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

I'm not too concerned with the spelling of the flag myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

FWIW, I was able to make CMake's module test suite pass with this patch on top 
of you MyP1689 branch on your Github fork. I also added some diffs to help 
clean up output files. I'll try and get it to work with the replacement patches 
as well, but I just want this to be a working checkpoint.


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

https://reviews.llvm.org/D134267

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

[PATCH] D137059: [Driver] [Modules] Introduce -fsave-std-c++-module-file= to specify the path of the module file (2/2)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D137059#3904256 , @ChuanqiXu wrote:

> In my mind, it is OK for CMake to support one-phase compilation model in the 
> short term. And  the fact that clang also supports the 2-phase compilation 
> wouldn't affect CMake. Do I understand right?  I mean, if the 2-phase 
> compilation wouldn't affect CMake, CMake should be able to ignore it. My 
> thought is that there are many more build systems in the world. And I know 
> many of them would handle the dependency them self fully instead of translate 
> the dependency to other build scripts like `make` and `ninja`. And I think it 
> should be good for the compiler to remain different possibilities for 
> different build systems (or tools).

Indeed. Even if everything supports 2-phase, I suspect there are cases where 
1-phase might still be better. But again, this needs real world numbers and 
testing to actually perform (more because of build graph shapes than individual 
TU timings).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137059

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D134267#3904257 , @ChuanqiXu wrote:

> BTW, this patch should be discarded. And now we're pursuing D137059 
>  and D137058 
> , where there is no modules cache. I know 
> it is a little bit frustrating to change the demo all the time... but let's 
> turn for that direction now.

Sorry, Phab is obtuse/unfamiliar for me right now; the "Abandoned" state isn't 
prevalent enough and all the pages look the same…


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

https://reviews.llvm.org/D134267

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

I tried applying this patch to your MyP1689 branch (fixing conflicts with 
attempts there), but it seems that something isn't being plumbed properly:

  clang-15: error: unknown argument: 
'-fc++-module-file-output=CMakeFiles/export_bmi_and_interfaces.dir/importable.pcm'
  clang-15: error: unable to create default module cache path 
"/home/boeckb/.cache/clang/ModuleCache": No such file or direc
  tory

There's no way to disable the "module cache path" either (it fails here because 
I symlinked it to `/dev/null` to avoid things "working" behind my back).


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

https://reviews.llvm.org/D134267

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

[PATCH] D137059: [Driver] [Modules] Introduce -fsave-std-c++-module-file= to specify the path of the module file (2/2)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

There is another motivating factor for 1-phase: the build graph is far simpler. 
With 2-phase, CMake will have to write out rules to perform:

- source -> .bmi
- .bmi -> .withbmi.o
- source -> .o

because we do not know if a BMI is needed or not. If it isn't we use the 
latter. If it is, we use the former. Note that this also means we need 2 
different `.o` filenames (as neither `make` nor `ninja` doesn't support 
multiple rules making the same output). This also means that the collator needs 
to generate a response file for the linker to direct which `.o` file to use for 
each TU based on the contents.

Also with 2-phase, it is an open question of whether it actually helps with 
distributed builds (or anywhere process execution and I/O are expensive 
compared to some minimal work unit such as, say, Windows compiling from a 
network drive). Since this is not a bright line, giving the option to say "I 
know that split BMI is better for me in this instance" and "please combine 
here" would be handy (depending on actual real-world perf results on real-world 
projects). Yes, this is a chicken-and-egg cycle :) .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137059

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

[PATCH] D137058: [Driver] [Modules] Support -fsave-std-c++-module-file (1/2)

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D137058#3896999 , @dblaikie wrote:

> Could you expound a bit on why "write the .pcm to the same path as the .o" 
> isn't sufficient? (like Split DWARF does this, for instance - not sure we 
> have lots of other things to extrapolate from (admittedly there is a flag for 
> specifying the filename for Split DWARF too, but it's not a terribly official 
> thing (doesn't have a '-f', '-g', etc prefix))

CMake doesn't support split dwarf (officially; it works, but CMake is just 
ignorant of it), so adapting to existing behavior would be more important 
there. For modules, however, explicit control is just much nicer to provide. It 
means that if heuristics change, CMake doesn't need patches to adapt to the 
logic. I would also vastly prefer that the `.pcm` filename match the *module* 
name, not the source file name. I'll also note that the example here is not 
suitable in the case of `Hello.cxx` and `Hello.cpp` existing at the same time 
(this is why CMake uses `Hello.cxx.o`). So some more control over the `.pcm` 
filename through more than source basename + `.pcm` is required in the general 
case (it is mostly mitigated if it is "replace `.o` with `.pcm`", but this is 
just unnecessary logic to ask anything interacting with Clang to deal with.

It also helps analyzers know that something else is happening explicitly and 
distributed builds to know what to bring back. Plus the other compilers offer 
controls over it; why does Clang have to be different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137058

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

[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Ben Boeckel via Phabricator via cfe-commits]

ben.boeckel added a comment.

In D134267#3876071 , @dblaikie wrote:

> I'm getting a bit exhausted with all the words involved here & not sure how 
> to simplify/clarify this.
>
> If @ben.boeckel has particular use cases, it might be easier for him to be 
> here discussing them so we can discuss the tradeoffs directly rather than 
> through intermediaries.

Sorry, I've been lax on actually keeping up-to-date on all of these Clang 
threads.

The current state is that CMake's test suite for C++ modules does not work with 
clang because it doesn't provide the control CMake needs to do its explicit 
builds. @ChuanqiXu's MyP1689 branch is close except that the `.pcm` output path 
can't seem to be controlled closely enough for reliable builds. Namely, I would 
like:

- the ability to disable the module cache (completely); CMake doesn't need nor 
want it
- the ability to say put the generated `.pcm` that this TU will generate goes 
*here*

The latter is currently a juggle of flags and restrictions that I haven't been 
able to figure out. Without something like `-fmodule-output-path=`, convincing 
Clang to output its `.pcm` file to where CMake wants it to be is some 
combination of `-fmodules-cache-path=` and `-fmodule-name=` while hoping that 
the internal path computation inside of Clang makes what CMake wants. It's just 
far easier to have something like `-o` that gives the answer straight away.

For the former, I use `-x c++module` as well so that I don't have to care about 
any extension-sniffing behaviors. I also need to give a bogus 
`-fmodule-cache-path=` for module consumers because as soon as `import` is 
seen, it wants to run off to look at the cache instead of noticing that there's 
a `-fmodule-file=` for everything it needs already and the cache is 100% unused.


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

https://reviews.llvm.org/D134267

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