[Driver] [Modules] Support -fmodule-output (1/2)

[ChuanqiXu]

Summary

Patches to support the one-phase compilation model for modules. This is the successor of D134267.

The behavior:
(1) If -o and -c is specified , the module file is in the same path within the same directory as the output the -o specified and with a new suffix .pcm.
(2) Otherwise, the module file is in the same path within the working directory directory with the name of the input file with a new suffix .pcm

For example,

Hello.cppm Use.cpp

A trivial one and the contents are ignored. When we run:

clang++ -std=c++20 -fmodule-output Hello.cppm -c

The directory would look like:

Hello.cppm  Hello.o  Hello.pcm Use.cpp

And if we run:

clang++ -std=c++20 -fmodule-output Hello.cppm -c -o output/Hello.o

Then the output directory may look like:

Hello.o  Hello.pcm

---

I'll add the ReleaseNotes and documentation after the series got accepted.

[dblaikie]

@ben.boeckel maybe this'd be the place to discuss the motivation for this feature (picking up from your comment here: https://reviews.llvm.org/D134267#3892629 )

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

[ChuanqiXu]

In D137058#3896999, @dblaikie wrote:

@ben.boeckel maybe this'd be the place to discuss the motivation for this feature (picking up from your comment here: https://reviews.llvm.org/D134267#3892629 )

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)

Although I agreed it would be better to wait for @ben.boeckel to answer the question, I still want to say "why we would offer something only if it is terribly bad". It looks like if it is normally bad, then we won't handle it and you need to workaround for it. I feel bad for the strategy.
Why can't we give some small help to make their lives easier.

[ben.boeckel]

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?

[dblaikie]

I realize I got this jumbled up and the thread about "why do we need to name things" is meant to be over in D137059 (sorry @ben.boeckel :/ I know this is all confusing to follow at the best of times) so I'll pick that up there.

But maybe a relevant question /here/ (maybe @iains has some context here, and @ben.boeckel ): What are GCC's (& any other implementations) command line interfaces for things like this? How're the command line flags spelled? To see about inspiration for this.

(judging from the discourse thread with @rsmith it seems like "the ability to generate a .o from a .pcm" is disfavored, he didn't outright say "we should remove that functionality" but pretty close to it - in favor of ".cppm -> {.o, .pcm}" and ".ccpm -> .o" + ".cppm -> .pcm" (without the ability to generate the .o from the .pcm) - in which case we could change the existing driver behavior sooner or later to address that third action (.cppm -> .pcm but it's only a minimal pcm). Maybe we should have two flags one that says "produce a .pcm" (which we already have --precompile for that) and another that says "produce a .o" (I guess that's the default, so maybe we want a way to opt /out/ of that behavior?))

Eh, sorry, just talking myself around in circles.
Currently we have:
.cppm -> .o (no extra flags)
.cppm -> .pcm (--precompile)
.cppm -> .pcm + .o (unsupported)

I'm not sure that --precompile is the best flag name to be inspired by (it's unqualified by any -f or other prefix, which usually feels a bit weird, and it's a very generic term, doesn't mention modules, etc) - so I can appreciate the -fsave-std-c++-module-file name here, but it does sound a bit verbose? Wouldn't mind hearing other people's thoughts on flag names, especially any prior art/other implementation choices we could look for for inspiration.

[iains]

In D137058#3906579, @dblaikie wrote:

I realize I got this jumbled up and the thread about "why do we need to name things" is meant to be over in D137059 (sorry @ben.boeckel :/ I know this is all confusing to follow at the best of times) so I'll pick that up there.

But maybe a relevant question /here/ (maybe @iains has some context here, and @ben.boeckel ): What are GCC's (& any other implementations) command line interfaces for things like this? How're the command line flags spelled? To see about inspiration for this.

.. the state I have is ...
As you note there are two parts to this:

1. the ability to emit a BMI and object from one invocation of the compiler (which can be done as in this series by the driver - or with alternate patches in the FE - which allows for the improvements we want in the BMI)
2. How to name the BMI.
   • • in my experimentation with GCC, for this part, it defers the decision until the module name is known, and then queries an interface to ask for the BMI filename for that named module. The interface can be internal (e.g. choosing a simple default like source.pcm) or it could get the mapping from some other place, like a build system. The FE side of the transaction does not need to know how the name was chosen (so it _could_ be specified from a command-line flag).
   • but, AFAIR, GCC does not currently have such a flag.

(judging from the discourse thread with @rsmith it seems like "the ability to generate a .o from a .pcm" is disfavored, he didn't outright say "we should remove that functionality" but pretty close to it - in favor of ".cppm -> {.o, .pcm}" and ".ccpm -> .o" + ".cppm -> .pcm" (without the ability to generate the .o from the .pcm) - in which case we could change the existing driver behavior sooner or later to address that third action (.cppm -> .pcm but it's only a minimal pcm).

This is the strategy my draft patches for the FE aims for (apologies, for repeatedly mentioning a patch set that is not yet up for review ...)

Maybe we should have two flags one that says "produce a .pcm" (which we already have --precompile for that) and another that says "produce a .o" (I guess that's the default, so maybe we want a way to opt /out/ of that behavior?))

Eh, sorry, just talking myself around in circles.
Currently we have:
.cppm -> .o (no extra flags)
.cppm -> .pcm (--precompile)
.cppm -> .pcm + .o (unsupported)

I'm not sure that --precompile is the best flag name to be inspired by (it's unqualified by any -f or other prefix, which usually feels a bit weird, and it's a very generic term, doesn't mention modules, etc) -

GCC has "-fmodule-only" for which I have a patch (also unpublished) that aliases that to --precompile in the driver.
(I think we could possibly see a reason to have -fobject-only for symmetry and to cover the cases you mention here)

so I can appreciate the -fsave-std-c++-module-file name here, but it does sound a bit verbose? Wouldn't mind hearing other people's thoughts on flag names, especially any prior art/other implementation choices we could look for for inspiration.

What was the objection to "-fc++-module-filename[=]" ?

As noted above, if my memory is not too faulty, GCC does not have this specific [name the BMI] flag yet (I think this is the case because GCC essentially uses a simple in-process P1184 interface as the fallback when there's no external build system), then this flag name is up for grabs - and what is chosen here could, presumably, be implemented for GCC too.

Actually, in some ways clang does something pretty similar in that there's the in-process module-mapper - which is where the filenames probably should be decided, we just tend to bypass it with specific command line input for output filenames (which does not work where there are two names needed, except by providing some simplified fallback - like the "just change the extension" approach).

edited: to fix some bad formatting of the replies.

[dblaikie]

What was the objection to "-fc++-module-filename[=]" ?

I guess it reads a bit awkwardly when you aren't providing the filename/want the default filename?

GCC has "-fmodule-only"

Hmm, I don't mind that too much (& as you say, '-fobject-only' - though that flag is maybe too vague?) - but it does mean we'd need a separate flag to name the .pcm output file, because that flag ('-fmodule-only') wouldn't be present in all cases where the pcm is generated, only when it's pcm-but-no-object. Maybe less "exclusionary" flag names and more explicit (like '-fbuild-the-binary-module' and '-fbuild-the-object'). I guess most C++ GCC options ( https://gcc.gnu.org/onlinedocs/gcc/Option-Summary.html ) don't include "C++" in them, but the modules ones mostly include "module".

We could get out in front, planning for the world in which BMIs are really only the interface (& whatever else we want to carry for optimizing /use/ of that interface, but not complete enough to be usable for generating the modules object) and use -fmodule-interface[=] (skipping the 'binary' part) and -fmodule-implementation or -fmodule-object (which could go either way - default on or off, but provide -fno-module-object to do the "generate PCM only")?

Only other minor thing might be singular V plural - some of Clang's flags are -fmodules and others are -fmodule - any sense of what's likely to work better? (be nice to unify/standardize on one or the other, I can see having variation there might make for frustrating usability trying to remember which is which)

[ChuanqiXu]

so I can appreciate the -fsave-std-c++-module-file name here, but it does sound a bit verbose?

My thought is the option is mainly used by build systems so that the users wouldn't type it frequently. In this case, I feel it might not be bad to make it a little bit redundant. Someone's redundancy is another's readability : )

Hmm, I don't mind that too much (& as you say, '-fobject-only' - though that flag is maybe too vague?) - but it does mean we'd need a separate flag to name the .pcm output file, because that flag ('-fmodule-only') wouldn't be present in all cases where the pcm is generated, only when it's pcm-but-no-object. Maybe less "exclusionary" flag names and more explicit (like '-fbuild-the-binary-module' and '-fbuild-the-object'). I guess most C++ GCC options ( https://gcc.gnu.org/onlinedocs/gcc/Option-Summary.html ) don't include "C++" in them, but the modules ones mostly include "module".

If this paragraph only talks about the GCC?

We could get out in front, planning for the world in which BMIs are really only the interface (& whatever else we want to carry for optimizing /use/ of that interface, but not complete enough to be usable for generating the modules object) and use -fmodule-interface[=] (skipping the 'binary' part) and -fmodule-implementation or -fmodule-object (which could go either way - default on or off, but provide -fno-module-object to do the "generate PCM only")?

For clang, I would like to add c++ in the option name since users are really easy to get confused with the existing clang modules. I've met many such cases. And for the proposal it self, I feel it is complex and hard to understand.

GCC has "-fmodule-only" for which I have a patch (also unpublished) that aliases that to --precompile in the driver.

This idea sounds good to me. But I would like to suggest to change the name to -fc++-module-interface-only. Due to the same reason above. The name -fmodule-only is not clear especially in the context of clang now.

Only other minor thing might be singular V plural - some of Clang's flags are -fmodules and others are -fmodule - any sense of what's likely to work better? (be nice to unify/standardize on one or the other, I can see having variation there might make for frustrating usability trying to remember which is which)

Yeah, sometimes it is frustrating. But I think we can't unify the existing options. It'd cause many breaking changes. And for module and modules, I prefer module a little more in practice.

[ChuanqiXu]

@dblaikie @ben.boeckel gentle ping~

[dblaikie]

clang/lib/Driver/Driver.cpp

5627–5628

Do we do that for -o today? (eg: if you try to -o and specify the input file name, such that the output would overwrite the input, what happens? I'm not sure - but I guess it's OK to do whatever that is for this case too)

clang/test/Driver/save-std-c++-module-file.cpp

1–11 ↗

(On Diff #471912)

Usually driver tests only invoke the driver and observe the output using -### to see what subcommands the driver would run. Could this test be phrased in that way? (testing whatever command line is expected to be invoked, rather than the actual behavior of the frontend)

[ben.boeckel]

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

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5627–5628

Do we do that for -o today? (eg: if you try to -o and specify the input file name, such that the output would overwrite the input, what happens? I'm not sure - but I guess it's OK to do whatever that is for this case too)

In this case, the input file will be overwrite and no warning shows. So it looks like we didn't take special treatment here. So I remove the FIXME.

clang/test/Driver/save-std-c++-module-file.cpp

1–11 ↗

(On Diff #471912)

Good point! Done.

[dblaikie]

Couple of minor fixes for the test, but otherwise seems fine.

clang/test/Driver/save-std-c++-module-file.cpp

1–4 ↗

(On Diff #475663)

Is this needed? Maybe we don't need to split the file, if it's just the one file anyway?

6 ↗

(On Diff #475663)

Is the prefix needed? I'd expect we'd usually regex match away the actual directory with {{.*}} in tests like this?

[ChuanqiXu]

Rename the option into -fmodule-output according to the discussion result from: https://gcc.gnu.org/pipermail/gcc/2022-December/240239.html

[iains]

thanks all for the patience on this one - and for the collaborative discussion - I do think the outcome is going to be an easier to remember option name ;)

[ChuanqiXu]

In D137058#3983687, @iains wrote:

thanks all for the patience on this one - and for the collaborative discussion - I do think the outcome is going to be an easier to remember option name ;)

Yeah. Thanks for your suggestion too. Otherwise I guess we won't get back : )

[dblaikie]

Please update the patch description/commit message to reflect the new naming.

clang/test/Driver/save-std-c++-module-file.cpp

8–9 ↗

(On Diff #481503)

Not sure I understand the need for two tests here - they both specify an absolute path to a .o file & CHECK that the absolute path matches the .pcm output file path, so they don't seem to be testing distinct scenarios?

14–15 ↗

(On Diff #481503)

Might be worth fleshing out the CHECKs a bit more -

If this patch implements:

Support .cppm -> .pcm + .o in one phase.

Then I'd expect the test to demonstrate that - show that there are two commands run by the driver and one outputs the .o file (so include checking for the .o file output file name, and whatever flags tell the frontend that object code is to be emitted) and one that outputs the .pcm file (including checking the .pcm file output file name, and whatever flags tell the frontend that a pcm should be emitted).

1–4 ↗

(On Diff #475663)

Ping on this ^

[h-vetinari]

clang/test/Driver/save-std-c++-module-file.cpp

1 ↗

(On Diff #481503)

The filename of this test still reflects the old option name and should presumably be changed.

[ChuanqiXu]

Address comments.

[ChuanqiXu]

clang/test/Driver/save-std-c++-module-file.cpp

8–9 ↗

(On Diff #481503)

The above one doesn't specify the path for the .o file. So in the first test the .pcm file will be in the same directory with the input source file. And in the second command line, the .pcm file will be in the same directory with the .o file.

1–4 ↗

(On Diff #475663)

Sorry for missing it. Thanks for clarification.

6 ↗

(On Diff #475663)

Yeah, generally we would use {{.*}}. And -DPREFIX is more precise for what we want to test. So I feel it wouldn't be bad.

[dblaikie]

(I'd probably still reduce the test down to one example, using -o and skipping the extra OUTPUT_PATH details, only checking the last part of the path is as specified (or perhaps checking that it matches the .o file?))

Also, could you consider the questions @urnathan asked in the cross-project/flag naming thread about the semantics of this flag - I imagine the right behavior is "whatever we do for .o files", but it'd be good to check/consider/test them (they may not need automated testing - if the behaviour is implemented with the same utilities for .o files as for .pcm files then I don't mind just a statement that that's the case and some manual testing has been done to verify that all works)?

clang/test/Driver/save-std-c++-module-file.cpp

8–9 ↗

(On Diff #481503)

Ah, maybe just testing the explicit -o version is adequate? We aren't doing anything interesting here except "whatever the .o path is" - so testing just "-o" seems sufficient to test the one path through this code. The pcm code doesn't have a "-o" and "implicit -o" codepath, just "do whatever the .o path is" so one test seems fine to me?

6 ↗

(On Diff #475663)

I think the extra precision probably isn't necessary/sufficiently valuable & we could use {{.*}} here?

[tahonermann]

clang/lib/Driver/Driver.cpp

5627–5628

Basing the location of the module output on the presence of -o sounds confusing to me. Likewise, defaulting to writing next to the input file as opposed to the current directory (where a .o file is written by default) sounds wrong. I think this option should be handled similarly to -o; it should accept a path and:

• If an absolute path is provided, write to that location.
• If a relative path is provided, write to that location relative to the current working directory.

Leave it up to the user or build system to ensure that the .o and .pcm file locations coincide if that is what they want. In general, I don't expect colocation of .o and .pcm files to be what is desired.

[dblaikie]

clang/lib/Driver/Driver.cpp

5627–5628

@tahonermann there's precedent for this with Split DWARF (.dwo files go next to the .o file) & I'd argued for possibly only providing this behavior, letting consumers move files elsewhere if they needed to, but got voted down there.

I do think this is a reasonable default, though. Users and build systems have the option to pass a path to place the .pcm somewhere else (in the follow-up patch to this one).

5630–5634

It'd be nice if we didn't have to recompute this/lookup OPT_o here - any way we can use the object file output path here (that's already handled OPT_o or using the base input name, etc)?

[ChuanqiXu]

• Reduce the test down to one example, using -o and skipping the extra OUTPUT_PATH details - as the comment required.

[ChuanqiXu]

(I'd probably still reduce the test down to one example, using -o and skipping the extra OUTPUT_PATH details, only checking the last part of the path is as specified (or perhaps checking that it matches the .o file?))

Done.

Also, could you consider the questions @urnathan asked in the cross-project/flag naming thread about the semantics of this flag - I imagine the right behavior is "whatever we do for .o files", but it'd be good to check/consider/test them (they may not need automated testing - if the behaviour is implemented with the same utilities for .o files as for .pcm files then I don't mind just a statement that that's the case and some manual testing has been done to verify that all works)?

Yeah, I've replied in that thread. And it looks like we can't test the behaviors if we can only check the output of -### in this patch. I've made some manual testing locally and let's document the behaviors later.

clang/lib/Driver/Driver.cpp

5627–5628

@tahonermann here is another patch which implements the behavior you described: https://reviews.llvm.org/D137059

5630–5634

I didn't understand this a lot. We don't compute anything here and we just use the object file output path here if -o is provided and we replace the suffix then. I feel this is simple enough.

[tahonermann]

clang/lib/Driver/Driver.cpp

5627–5628

I'm ok with a default that consistently writes the PCM relative to the location of the .o file (if not overridden with an absolute path). What I'm not ok with is having the default be next to the .o file if -o is specified and next to the input file if -o is not specified. I don't think writing the PCM relative to the input file is a good default in any case. If -o is not specified, then I think it should be written relative to the current working directory; which matches where the .o file will be written.

[tahonermann]

clang/lib/Driver/Driver.cpp

5627–5628

Summary of the consensus of the Clang Modules WG:

1. If -fmodule-output= is provided, the PCM is written to the indicated file (relative to the current working directory for a relative path).
2. If -fmodule-output is provided, the PCM is written relative to the location of the .o file (the current working directory by default and the location indicated by -o otherwise).

[ChuanqiXu]

Address comments:

• if -o is not provided, emit the module file to the working directory instead of the directory of the input file.

clang/lib/Driver/Driver.cpp

5627–5628

Done.

[dblaikie]

clang/lib/Driver/Driver.cpp

5630–5634

Computing the path to write to is what I'm referring to - and the fact that this had a bug (was relative to the source path instead of the CWD)/divergence from the -o path logic is the sort of thing I want to avoid.

I'm not sure there's an easy way to do this - but looks like the logic to name the .o file output is in Driver::GetNamedOutputPath and gets stored in the clang::driver::Compilation.... which I guess is where we are, but we're going through here with a PrecompileJobAction insntead of the compile job action, I suppose.

Could we keep these two codepaths closer together?

It'd be really nice if we could reuse that in some way.

Hmm, actually, why doesn't this fall out of the existing algorithm without modification?

Ah, I see, since the precompile action isn't "at top level" it gets a temporary file name - so if we change only that, it seems to fall out naturally:

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index c7efe60b2335..db878cbfff46 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
   }
 
   // Output to a temporary file?
-  if ((!AtTopLevel && !isSaveTempsEnabled() &&
+  if (((!AtTopLevel && !isSaveTempsEnabled() &&
        !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
-      CCGenDiagnostics) {
+      CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
     StringRef Name = llvm::sys::path::filename(BaseInput);
     std::pair<StringRef, StringRef> Split = Name.split('.');
     const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());

Without the need to reimplement/duplicate the -o handling logic?

[dblaikie]

clang/lib/Driver/Driver.cpp

5630–5634

Oh, I should say, this patch didn't actually have the flag support, but it'd be something like this ^ but with the command line argument test as well (so "other stuff that's already there && !(TY_ModuleFile && hasArg fmodule-output)")

[ChuanqiXu]

Address comments:

• Reuse the logic to compute object file for module file when -o is not specified.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5630–5634

To be honest, I prefer the previous patch. I feel it has higher readability. But this is a problem about taste and it doesn't have standard answer. Someone's readability is redundancy for others : )

[dblaikie]

clang/lib/Driver/Driver.cpp

5630–5634

I think there's real functionality we're at risk of missing by having separate implementations.

For instance - how does this interact with Apple's multiarch support (eg: clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target x86_64_apple_darwin) - from my testing, without specifying an output file, you get something semi-usable/not simply incorrect: test-i386.pcm and test-x86_64.pcm. But if you try to name the output file you get foo.pcm and then another foo.pcm that overwrites the previous one. I think this could be taken care of if the suffix handling code was delegated down towards the else block that starts with SmallString<128> Output(getDefaultImageName());

But maybe solving that ^ problem could come out of a more general solution to the next problem:

What if you specify multiple source files on the command line without -c? Without -o you get test1.pcm and test2.pcm, but with -o foo you get foo.pcm overwriting foo.pcm. Perhaps if the output file specified isn't a .o file, we should ignore the -o and use the input-filename based naming? I guess we could reject this situation outright, and require the user to run multiple separate compilations. Though keeping features composable is nice.

Perhaps this needs a bit more consideration of some of these cases?

[ChuanqiXu]

• when -fmodule-output and -o are both specified, generate the module file into the directory of the output file and name the module file with the name of the input file with module file's suffixes. (Previously, the name of the module will be the same with the output file). The change tries to avoid rewrite the module file when we specify multiple input files in one command line.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5630–5634

Oh, thanks for finding this. It is pretty bad that I didn't image we can specify multiple input module units in one command line.

What if you specify multiple source files on the command line without -c? Without -o you get test1.pcm and test2.pcm, but with -o foo you get foo.pcm overwriting foo.pcm. Perhaps if the output file specified isn't a .o file, we should ignore the -o and use the input-filename based naming? I guess we could reject this situation outright, and require the user to run multiple separate compilations. Though keeping features composable is nice.

Now the name of the module file will be the same with the input file no matter if we specified -o or not. With -o specified, the module files will be generated into the same directory with the output file. Without -o specified, the module files will be generated in the working directory. It'll still be problematic if the user specify two inputs with the same name in two different directory, the module file of the latter will overwrite the former one. But I guess we don't need to handle such cases?

For instance - how does this interact with Apple's multiarch support (eg: clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target x86_64_apple_darwin) - from my testing, without specifying an output file, you get something semi-usable/not simply incorrect: test-i386.pcm and test-x86_64.pcm. But if you try to name the output file you get foo.pcm and then another foo.pcm that overwrites the previous one. I think this could be taken care of if the suffix handling code was delegated down towards the else block that starts with SmallString<128> Output(getDefaultImageName());

In the new implementation, I image we'll generate test-i386.pcm and test-x86_64.pcm even if we specified -o with -fmodule-output. But the weird thing is that when I tried to reproduce your example, the compiler told me the other archs are ignored. I'm not sure if I set something wrong or I must do it in Apple machine.

BTW, I am not sure if I misunderstand you, but the else block that starts with SmallString<128> Output(getDefaultImageName()); handles about IMAGE types, which looks irreverent to me.

[tahonermann]

clang/lib/Driver/Driver.cpp

5630–5634

how does this interact with Apple's multiarch support

Good question. What does split dwarf do in this case? Are differently named outputs generated or is a multi-arch dwarf file produced (assuming they exist)?

Rejecting the command line if it specifies multiple -arch options with -fmodule-output= seems fair to me (unless/until support for multi-arch .pcm files is added). How is this handled with -split_dwarf_output?

I'm not sure if I set something wrong or I must do it in Apple machine.

I don't recall for sure, but I think Apple Clang is needed. We noticed differences between community Clang and Apple Clang while I was at Coverity, but I don't recall the details.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5630–5634

How is this handled with -split_dwarf_output?

Although I am not familiar with split-dwarf, according to https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/BackendUtil.cpp#L1076-L1087, it looks like -split_dwarf_output wouldn't handle the multiple -arch options. But I am not sure if the reason is the dwarf information is arch-independent (or not). Maybe we need to ask @dblaikie to make sure.

Rejecting the command line if it specifies multiple -arch options with -fmodule-output= seems fair to me (unless/until support for multi-arch .pcm files is added).

Yeah, it sounds good to me too. It is simpler and I feel like it wouldn't hurt so much if we don't support -fmodule-output with multiple arch.

[dblaikie]

(pulling this out from an inline comment, because it's hard to quote/smaller than it needs to be for more complex discussions):

Oh, thanks for finding this. It is pretty bad that I didn't image we can specify multiple input module units in one command line.

What if you specify multiple source files on the command line without -c? Without -o you get test1.pcm and test2.pcm, but with -o foo you get foo.pcm overwriting foo.pcm. Perhaps if the output file specified isn't a .o file, we should ignore the -o and use the input-filename based naming? I guess we could reject this situation outright, and require the user to run multiple separate compilations. Though keeping features composable is nice.

Now the name of the module file will be the same with the input file no matter if we specified -o or not. With -o specified, the module files will be generated into the same directory with the output file. Without -o specified, the module files will be generated in the working directory. It'll still be problematic if the user specify two inputs with the same name in two different directory, the module file of the latter will overwrite the former one. But I guess we don't need to handle such cases?

For instance - how does this interact with Apple's multiarch support (eg: clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target x86_64_apple_darwin) - from my testing, without specifying an output file, you get something semi-usable/not simply incorrect: test-i386.pcm and test-x86_64.pcm. But if you try to name the output file you get foo.pcm and then another foo.pcm that overwrites the previous one. I think this could be taken care of if the suffix handling code was delegated down towards the else block that starts with SmallString<128> Output(getDefaultImageName());

In the new implementation, I image we'll generate test-i386.pcm and test-x86_64.pcm even if we specified -o with -fmodule-output. But the weird thing is that when I tried to reproduce your example, the compiler told me the other archs are ignored. I'm not sure if I set something wrong or I must do it in Apple machine.

BTW, I am not sure if I misunderstand you, but the else block that starts with SmallString<128> Output(getDefaultImageName()); handles about IMAGE types, which looks irreverent to me.

how does this interact with Apple's multiarch support

Good question. What does split dwarf do in this case? Are differently named outputs generated or is a multi-arch dwarf file produced (assuming they exist)?

Split DWARF isn't supported on Darwin/MacOS/whatever it's called - the MachO debug info distribution model looks more like, but isn't quite, Split DWARF already (it keeps the debug info in .o files, doesn't link them into the final executable - and you can make a separate debug info archive with dsymutil).

Rejecting the command line if it specifies multiple -arch options with -fmodule-output= seems fair to me (unless/until support for multi-arch .pcm files is added).

Yeah, trying to see if

I'm not sure if I set something wrong or I must do it in Apple machine.

I don't recall for sure, but I think Apple Clang is needed. We noticed differences between community Clang and Apple Clang while I was at Coverity, but I don't recall the details.

I don't think this needs Apple Clang, but it does need a Darwin triple. I was reproducing/examining the behavior with clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -### -target x86_64-apple-darwin.

[ChuanqiXu]

Address comments: Reject the command line if it specifies -fmodule-output and multiple arch.

[tahonermann]

I don't know the driver code very well, but as best I can tell, this appears to match the design agreed to at the last Clang Modules WG meeting.

clang/test/Driver/module-output.cppm

6

19

27

[ChuanqiXu]

Address comments.

[ChuanqiXu]

@dblaikie would you like to take a look at this?

[h-vetinari]

Without undue haste, I just wanted to comment from the peanut gallery that it would be amazing if the patches that are necessary for CMake + Clang to use C++ modules would make the cut-off for LLVM 16 that's coming up around the 24th of January.

[ChuanqiXu]

In D137058#4033825, @h-vetinari wrote:

Without undue haste, I just wanted to comment from the peanut gallery that it would be amazing if the patches that are necessary for CMake + Clang to use C++ modules would make the cut-off for LLVM 16 that's coming up around the 24th of January.

Thanks for the note. I'll land this patch in the next week if no further comments come in. BTW, the series of clang-scan-deps patch (https://reviews.llvm.org/D139168) is also necessary 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?

[h-vetinari]

BTW, the series of clang-scan-deps patch (https://reviews.llvm.org/D139168) is also necessary [...]

Yes, I meant both those series.

[...] 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.

[dblaikie]

In D137058#4030331, @ChuanqiXu wrote:

@dblaikie would you like to take a look at this?

Yep, this is still on my radar - sorry for the delays. I'll get to it.

[dblaikie]

clang/lib/Driver/Driver.cpp

5814–5816

These conditions might be shared with the previous condition that tests the opposite?

5817

Is there some way we can avoid this (-fmodule-output -o ...) being a special case? It'd be good if we could use a single common implementation regardless of whether -o is used (ie: Figure out the output file name (whether it's implicitly determined by clang, in the absence of -o, or explicitly provided by the user through -o) and then replace the suffix with pcm)?

[ChuanqiXu]

Address comments: merge the conditions.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5817

I guess we can't do it or we can't do it easily. Otherwise the .pcm file will always lives in the same directory with the input file.

[dblaikie]

clang/lib/Driver/Driver.cpp

5817

I guess we can't do it or we can't do it easily. Otherwise the .pcm file will always lives in the same directory with the input file.

I don't follow/understand. What I mean is, I'd like it, if possible, this was implemented by something like "find the path for the .o file output, then replace the extension with .pcm".

I worry atht code like this that recomputes something similar to the object output path risks getting out of sync with the actual object path.

[tahonermann]

clang/lib/Driver/Driver.cpp

5817

That is certainly a valid concern and I agree it would be better if the shared output path is computed exactly once. If that would require significant change, then tests to ensure consistent behavior would be the next best thing. I'm not sure what infrastructure is in place for validation of output file locations though.

[dblaikie]

clang/lib/Driver/Driver.cpp

5817

Well, I guess the Split DWARF file naming isn't fundamentally better - it looks at the OPT_O argument directly too:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250

Perhaps we could at least reuse this same logic - make it a reusable function in some way? (for instance, it checks -c too, which seems relevant - we wouldn't want to name the .pcm after the linked executable name, right?

[ChuanqiXu]

Address comments:

• Extract the logic to compute the output path of -fmodule-output to a reusable function.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5817

Perhaps we could at least reuse this same logic - make it a reusable function in some way?

Done. It looks better now.

(for instance, it checks -c too, which seems relevant - we wouldn't want to name the .pcm after the linked executable name, right?

Oh, right. Although the previous conclusion is that if -o is specified, the .pcm file should be in the same dir with the output. But it is indeed weird that the .pcm file are related to the linked executable if we thought it deeper.

[dblaikie]

clang/lib/Driver/Driver.cpp

5817

Ah, I was hoping it could reuse the same code, rather than duplicate it - any chance it could be refactored into a common implementation between Split DWARF and modules?

[h-vetinari]

clang/lib/Driver/Driver.cpp

5639–5640

5817

Ah, I was hoping it could reuse the same code, rather than duplicate it - any chance it could be refactored into a common implementation between Split DWARF and modules?

Could we uncouple this clean-up from landing the patches before the LLVM 16 branch? A trivial refactor might even still be backportable, but the whole series will be more challenging.

[ChuanqiXu]

Address comments: fix a typo.

[ChuanqiXu]

clang/lib/Driver/Driver.cpp

5817

Ah, I was hoping it could reuse the same code, rather than duplicate it - any chance it could be refactored into a common implementation between Split DWARF and modules?

I feel it is an overkill to merge these 2 logics. It will become harder to read and modify the logics after we merge them. I feel better with the current implementation since it has fewer code dependencies and its complexity is low.

[dblaikie]

I really don't think this is the right thing to do - the Split DWARF code, for instance, has support for GPU bundling that's missing in the module file naming code, which seems likely to be broken & within reason handle-able today by reusing the Split DWARF code, similarly with the multi-arch bundling for MachO. But I've tried to explain these things in several ways, and haven't managed to connect.

Carry on.

[ChuanqiXu]

In D137058#4050188, @dblaikie wrote:

I really don't think this is the right thing to do - the Split DWARF code, for instance, has support for GPU bundling that's missing in the module file naming code, which seems likely to be broken & within reason handle-able today by reusing the Split DWARF code, similarly with the multi-arch bundling for MachO. But I've tried to explain these things in several ways, and haven't managed to connect.

Carry on.

Thanks for your patient reviewing! We can merge the logics with Split DWARF code someday when we find it necessary.

[Jake-Egan]

Hi, this new test fails on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
Could you take a look?

[ChuanqiXu]

In D137058#4056647, @Jake-Egan wrote:

Hi, this new test fails on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
Could you take a look?

I added // REQUIRES: x86-registered-target. Is it still failing?

[Jake-Egan]

In D137058#4057424, @ChuanqiXu wrote:

In D137058#4056647, @Jake-Egan wrote:

Hi, this new test fails on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
Could you take a look?

I added // REQUIRES: x86-registered-target. Is it still failing?

It is still failing yes, I think it should restrict system-aix instead, like you did on windows.

[dblaikie]

In D137058#4055275, @ChuanqiXu wrote:

In D137058#4050188, @dblaikie wrote:

I really don't think this is the right thing to do - the Split DWARF code, for instance, has support for GPU bundling that's missing in the module file naming code, which seems likely to be broken & within reason handle-able today by reusing the Split DWARF code, similarly with the multi-arch bundling for MachO. But I've tried to explain these things in several ways, and haven't managed to connect.

Carry on.

Thanks for your patient reviewing! We can merge the logics with Split DWARF code someday when we find it necessary.

My concern is that when it becomes necessary it won't be apparent - someone will fix (or introduce) a bug in one codepath, unaware of the other similar codepath. Unifying them before that happens is valuable.

[ChuanqiXu]

In D137058#4058836, @Jake-Egan wrote:

In D137058#4057424, @ChuanqiXu wrote:

In D137058#4056647, @Jake-Egan wrote:

Hi, this new test fails on AIX https://lab.llvm.org/buildbot/#/builders/214/builds/5351/steps/6/logs/FAIL__Clang__module-output_cppm
Could you take a look?

I added // REQUIRES: x86-registered-target. Is it still failing?

It is still failing yes, I think it should restrict system-aix instead, like you did on windows.

Got it. Could you help to add this? Since I can't test it properly.

My concern is that when it becomes necessary it won't be apparent - someone will fix (or introduce) a bug in one codepath, unaware of the other similar codepath. Unifying them before that happens is valuable.

Understood. I just felt that the tools::SplitDebugName and GetModuleOutputPath have more different points than common points.

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_modules_validate_once_requires_timestamp : Error<
   "option '-fmodules-validate-once-per-build-session' requires "
   "'-fbuild-session-timestamp=<seconds since Epoch>' or '-fbuild-session-file=<file>'">;
 
 def err_test_module_file_extension_format : Error<
   "-ftest-module-file-extension argument '%0' is not of the required form "
   "'blockname:major:minor:hashed:user info'">;
 
+def err_drv_module_output_with_multiple_arch : Error<
+  "option '-fmodule-output' can't be used with multiple arch options">;
+
 def err_drv_extract_api_wrong_kind : Error<
   "header file '%0' input '%1' does not match the type of prior input "
   "in api extraction; use '-x %2' to override">;
 
 def warn_slash_u_filename : Warning<"'/U%0' treated as the '/U' option">,
   InGroup<DiagGroup<"slash-u-filename">>;
 def note_use_dashdash : Note<
   "use '--' to treat subsequent arguments as filenames">;

clang/include/clang/Driver/Options.td

 def fprebuilt_module_path : Joined<["-"], "fprebuilt-module-path=">, Group<i_Group>,
   Flags<[NoXarchOption, CC1Option]>, MetaVarName<"<directory>">,
   HelpText<"Specify the prebuilt module path">;
 defm prebuilt_implicit_modules : BoolFOption<"prebuilt-implicit-modules",
   HeaderSearchOpts<"EnablePrebuiltImplicitModules">, DefaultFalse,
   PosFlag<SetTrue, [], "Look up implicit modules in the prebuilt module path">,
   NegFlag<SetFalse>, BothFlags<[NoXarchOption, CC1Option]>>;
 
+def fmodule_output : Flag<["-"], "fmodule-output">, Flags<[NoXarchOption, CC1Option]>,
+  HelpText<"Save intermediate module file results when compiling a standard C++ module unit.">;
+
 def fmodules_prune_interval : Joined<["-"], "fmodules-prune-interval=">, Group<i_Group>,
   Flags<[CC1Option]>, MetaVarName<"<seconds>">,
   HelpText<"Specify the interval (in seconds) between attempts to prune the module cache">,
   MarshallingInfoInt<HeaderSearchOpts<"ModuleCachePruneInterval">, "7 * 24 * 60 * 60">;
 def fmodules_prune_after : Joined<["-"], "fmodules-prune-after=">, Group<i_Group>,
   Flags<[CC1Option]>, MetaVarName<"<seconds>">,
   HelpText<"Specify the interval (in seconds) after which a module file will be considered unused">,
   MarshallingInfoInt<HeaderSearchOpts<"ModuleCachePruneAfter">, "31 * 24 * 60 * 60">;

clang/lib/Driver/Driver.cpp

if (MultipleArchs && !BoundArch.empty()) {
                               Twine(Prefix) + "-" + BoundArch + "." + Suffix);
     } else {
       TmpName = GetTemporaryPath(Prefix, Suffix);
     }
   }
   return C.addTempFile(C.getArgs().MakeArgString(TmpName));
 }
 
+// Calculate the output path of the module file when compiling a module unit
+// with the `-fmodule-output` option specified. The behavior is:
+// - If the output object file of the module unit is specified, the output path
+//   of the module file should be the same with the output object file except
+//   the corresponding suffix. This requires both `-o` and `-c` are specified.
+// - Otherwise, the output path of the module file will be the same with the
+//   input with the corresponding suffix.
+static const char *GetModuleOutputPath(Compilation &C, const JobAction &JA,
+                                       const char *BaseInput) {
+  assert(isa<PrecompileJobAction>(JA) && JA.getType() == types::TY_ModuleFile &&
+         C.getArgs().hasArg(options::OPT_fmodule_output));
+
+  SmallString<64> OutputPath;
+  Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o);
+  if (FinalOutput && C.getArgs().hasArg(options::OPT_c))
+    OutputPath = FinalOutput->getValue();
+  else
+    OutputPath = BaseInput;
+
+  const char *Extension = types::getTypeTempSuffix(JA.getType());
+  llvm::sys::path::replace_extension(OutputPath, Extension);
+  return C.addResultFile(C.getArgs().MakeArgString(OutputPath.c_str()), &JA);
+}
+
 const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
                                        const char *BaseInput,
                                        StringRef OrigBoundArch, bool AtTopLevel,
                                        bool MultipleArchs,
                                        StringRef OffloadingPrefix) const {
   std::string BoundArch = OrigBoundArch.str();
   if (is_style_windows(llvm::sys::path::Style::native)) {
     // BoundArch may contains ':', which is invalid in file names on Windows,
const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
   }
 
   if (IsDXCMode() && !C.getArgs().hasArg(options::OPT_o))
     return "-";
 
   // Is this the assembly listing for /FA?
   if (JA.getType() == types::TY_PP_Asm &&
       (C.getArgs().hasArg(options::OPT__SLASH_FA) ||
        C.getArgs().hasArg(options::OPT__SLASH_Fa))) {
     // Use /Fa and the input filename to determine the asm file name.
     StringRef BaseName = llvm::sys::path::filename(BaseInput);
     StringRef FaValue = C.getArgs().getLastArgValue(options::OPT__SLASH_Fa);
     return C.addResultFile(
         MakeCLOutputFilename(C.getArgs(), FaValue, BaseName, JA.getType()),
         &JA);
   }
 
+  if (MultipleArchs && C.getArgs().hasArg(options::OPT_fmodule_output))
+    Diag(clang::diag::err_drv_module_output_with_multiple_arch);
+
+  // If we're emitting a module output with the specified option
+  // `-fmodule-output`.
+  if (!AtTopLevel && isa<PrecompileJobAction>(JA) &&
+      JA.getType() == types::TY_ModuleFile &&
+      C.getArgs().hasArg(options::OPT_fmodule_output))
+    return GetModuleOutputPath(C, JA, BaseInput);
+
   // Output to a temporary file?
   if ((!AtTopLevel && !isSaveTempsEnabled() &&
        !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
       CCGenDiagnostics) {
     StringRef Name = llvm::sys::path::filename(BaseInput);
     std::pair<StringRef, StringRef> Split = Name.split('.');
     const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
     return CreateTempFile(C, Split.first, Suffix, MultipleArchs, BoundArch);
const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
   // As an annoying special case, PCH generation doesn't strip the pathname.
   if (JA.getType() == types::TY_PCH && !IsCLMode()) {
     llvm::sys::path::remove_filename(BasePath);
     if (BasePath.empty())
       BasePath = NamedOutput;
     else
       llvm::sys::path::append(BasePath, NamedOutput);
     return C.addResultFile(C.getArgs().MakeArgString(BasePath.c_str()), &JA);
-  } else {
-    return C.addResultFile(NamedOutput, &JA);
   }
+
+  return C.addResultFile(NamedOutput, &JA);
 }
 
 std::string Driver::GetFilePath(StringRef Name, const ToolChain &TC) const {
   // Search for Name in a list of paths.
   auto SearchPaths = [&](const llvm::SmallVectorImpl<std::string> &P)
       -> std::optional<std::string> {
     // Respect a limited subset of the '-Bprefix' functionality in GCC by
     // attempting to use this prefix when looking for file paths.
     for (const auto &Dir : P) {
       if (Dir.empty())
         continue;
       SmallString<128> P(Dir[0] == '=' ? SysRoot + Dir.substr(1) : Dir);
       llvm::sys::path::append(P, Name);
       if (llvm::sys::fs::exists(Twine(P)))
         return std::string(P);

clang/test/Driver/lit.local.cfg

 from lit.llvm import llvm_config
 
-config.suffixes = ['.c', '.cpp', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
+config.suffixes = ['.c', '.cpp', '.cppm', '.h', '.m', '.mm', '.S', '.s', '.f90', '.F90', '.f95',
                    '.cu', '.rs', '.cl', '.clcpp', '.hip', '.hipi', '.hlsl']
 config.substitutions = list(config.substitutions)
 config.substitutions.insert(0,
     ('%clang_cc1',
      """*** Do not use 'clang -cc1' in Driver tests. ***""") )
 
 # Remove harmful environmental variables for clang Driver tests.
 # Some might be useful for other tests so they are only removed here.

clang/test/Driver/module-output.cppm

+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: split-file %s %t
+//
+// Tests that the .pcm file will be generated in the same directory with the specified
+// output and the name of the .pcm file should be the same with the input file.
+// RUN: %clang -std=c++20 %t/Hello.cppm -fmodule-output -c -o %t/output/Hello.o \
+// RUN:   -### 2>&1 | FileCheck %t/Hello.cppm
+//
+// Tests that the output file will be generated in the input directory if the output
+// file is not the corresponding object file.
+// RUN: %clang -std=c++20 %t/Hello.cppm %t/AnotherModule.cppm -fmodule-output -o \
+// RUN:   %t/output/a.out -### 2>&1 | FileCheck  %t/AnotherModule.cppm
+//
+// Tests that clang will reject the command line if it specifies -fmodule-output with
+// multiple archs.
+// RUN: %clang %t/Hello.cppm -fmodule-output -arch i386 -arch x86_64 -### -target \
+// RUN:   x86_64-apple-darwin 2>&1 | FileCheck %t/Hello.cppm -check-prefix=MULTIPLE-ARCH
+
+//--- Hello.cppm
+export module Hello;
+
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/output/Hello.o" "-x" "pcm" "{{.*}}/output/Hello.pcm"
+
+// MULTIPLE-ARCH: option '-fmodule-output' can't be used with multiple arch options
+
+//--- AnotherModule.cppm
+export module AnotherModule;
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello.pcm" "-x" "c++" "{{.*}}/Hello.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "Hello.cppm" {{.*}}"-o" "{{.*}}/Hello-{{.*}}.o" "-x" "pcm" "{{.*}}/Hello.pcm"
+// CHECK: "-emit-module-interface" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule.pcm" "-x" "c++" "{{.*}}/AnotherModule.cppm"
+// CHECK: "-emit-obj" {{.*}}"-main-file-name" "AnotherModule.cppm" {{.*}}"-o" "{{.*}}/AnotherModule-{{.*}}.o" "-x" "pcm" "{{.*}}/AnotherModule.pcm"