[clang-cl] Accept `cl`-style output arguments (`/Fo`, `-Fo`) for `--precompile` #121046

sharadhr · Dec 24, 2024

This PR resolves this issue, which can be summarised as such. Given the following foo.cxx:

export module foo;
export int from_foo()
{
  return 0;
}

Running the following outputs foo.pcm, but doesn’t output foo.bmi as requested by -Fo:

clang-cl.exe -std:c++20 --precompile -x c++-module -fmodule-output=foo.bmi -Fo"foo.bmi" -c foo.cxx

However, running the following outputs foo.bmi as requested:

clang++.exe -std=c++20 --precompile -x c++-module -fmodule-output=foo.bmi -o foo.bmi -c foo.cxx

This behaviour is then used by clang-scan-deps to output the dependency scanning JSON. In the former case since no .bmi is output, the corresponding value of the primary-output key here is "-".

This invocation is used by CMake to run clang-scan-deps and build the dependency tree.

llvmbot · Dec 24, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Sharadh Rajaraman (sharadhr)

Changes

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

1 Files Affected:

(modified) clang/lib/Driver/Driver.cpp (+2-2)

diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
index ecae475f75da00..b47e0443d6e9db 100644
--- a/clang/lib/Driver/Driver.cpp
+++ b/clang/lib/Driver/Driver.cpp
@@ -5442,7 +5442,7 @@ InputInfoList Driver::BuildJobsForAction(
   InputInfoList Result = BuildJobsForActionNoCache(
       C, A, TC, BoundArch, AtTopLevel, MultipleArchs, LinkingOutput,
       CachedResults, TargetDeviceOffloadKind);
-  CachedResults[ActionTC] = Result;
+   CachedResults[ActionTC] = Result;
   return Result;
 }
 
@@ -5890,7 +5890,7 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
   llvm::PrettyStackTraceString CrashInfo("Computing output path");
   // Output to a user requested destination?
   if (AtTopLevel && !isa<DsymutilJobAction>(JA) && !isa<VerifyJobAction>(JA)) {
-    if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+    if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o, options::OPT__SLASH_Fo, options::OPT__SLASH_Fo_COLON))
       return C.addResultFile(FinalOutput->getValue(), &JA);
   }

clang/lib/Driver/Driver.cpp

ChuanqiXu9 · Dec 25, 2024

The tests are failing and it looks relevent.

nikic · Jan 3, 2025

Why is this submitted against the release/19.x branch? Is this a backport? If so, please indicate which commit it backports.

sharadhr · Jan 5, 2025

Why is this submitted against the release/19.x branch? Is this a backport? If so, please indicate which commit it backports.

Not a backport but a new commit. I have now re-submitted it against main, but I'd like it back-ported to 19.x if possible.

ChuanqiXu9 · Jan 6, 2025

Why is this submitted against the release/19.x branch? Is this a backport? If so, please indicate which commit it backports.

Not a backport but a new commit. I have now re-submitted it against main, but I'd like it back-ported to 19.x if possible.

It is better to fix the test failures (or announce they are not relevant with your pr)

github-actions · Jan 12, 2025

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

sharadhr · Jan 13, 2025

I'm not sure how to fix the Driver/at_file.c and ClangScanDeps/target-filename.cpp tests. I'd love some help with these.

ChuanqiXu9 · Jan 16, 2025

@ChuanqiXu9 are you able to have a look?

Sorry, I can't test and take a deeper look on windows. From https://discourse.llvm.org/t/driver-volunteer-wanted-for-modules-support-in-driver-for-mac-ubuntu-and-windows/83768/13, I think you can CC people there or calling again there.

rnk · Feb 6, 2025

Thanks, the code seems correct, but can this be tested? cc @zmodem for clang-cl driver changes

sharadhr · Feb 7, 2025

Ready for review; all relevant tests passing.

ChuanqiXu9 · Feb 8, 2025

Ready for review; all relevant tests passing.

I think @rnk was saying you should add a test for this.

sharadhr · Feb 8, 2025

Fair point. Added a test. Additionally the CMake repository contains a bigger battery of tests that also all pass with this PR.

sharadhr · Feb 10, 2025

A quick bump. I'd like to get this reviewed hopefully in time for the Clang 20.x release. I think having it in 19.x is a lost battle, it's a bit too late for that.

clang/lib/Driver/Driver.cpp

zmodem · Feb 10, 2025

clang/lib/Driver/Driver.cpp

+    Arg *FinalOutput = nullptr;
+    if (IsCLNonPCH && HasAnyOutputArg) {
+      FinalOutput = C.getArgs().getLastArg(
+          options::OPT_o, options::OPT__SLASH_Fo, options::OPT__SLASH_Fo_COLON);
+    } else if (IsCLNonPCH) {
+      FinalOutput = C.getArgs().getLastArg(options::OPT__SLASH_Fo,
+                                           options::OPT__SLASH_Fo_COLON);
+    } else {
+      FinalOutput = C.getArgs().getLastArg(options::OPT_o);
+    }


I'm having trouble following the logic here. Can you add a comment explaining what's going on? I.e. when is -o and/or /Fo supposed to be considered?

I've updated the OP message with the reasoning for this PR.

This logic in particular is to resolve lit test cases that failed when /Yp was passed to clang-cl.exe, i.e. PCHs were generated. I believe this is because /Yp uses /Fo for the PCH output, and therefore this invocation needs to use /o (or -o). This PR originally just wrapped lines 6111 and 6112 around an if (IsCLMode()) { ... }, which failed many PCH test cases.

HasAnyOutputArg was written specifically to resolve llvm-project\clang\test\ClangScanDeps\target-filename.cpp failing; more particularly, the clangcl.o test cases. Admittedly it feels like a weird hack and I'm open to suggestions.

I do hope there's some scope for a response, as I'm not sure how to proceed.

You can ping the reviewer @zmodem . The suggested style is no more ping in a week.

The logic has to be clear to any reader. "It makes the lit tests pass" is not a good explanation.

I think you need to figure out how these flags are supposed to work together, and then translate that into code.

Given the comment below, I believe it is incorrect to pass both -fmodule-output and the combination of --precompile and -o (or -Fo); this is a CMake bug. I therefore think we can simplify this logic.

Additionally, --precompile + -o and -fmodule-output then need to be marked as mutually-exclusive command-line options.

zmodem · Feb 10, 2025

clang/test/Driver/cl-cxx20-modules.cppm

@@ -14,3 +14,11 @@

 //--- test.pcm
 // CPP20WARNING-NOT: clang-cl: warning: argument unused during compilation: '/std:c++20' [-Wunused-command-line-argument]
+
+// test whether the following outputs %Hello.bmi
+// RUN: %clang_cl /std:c++20 --precompile -x c++-module -fmodule-output=%t/Hello.bmi -Fo"%t/Hello.bmi" -c %t/Hello.cppm -### 2>&1 | FileCheck %s


This is just one invocation, but the logic in GetNamedOutputPath() seems to cover many cases.

Also, is it -fmodule-output or /Fo that sets the output filename (or both?)

(Nit: you'll want to put -- before %t/Hello.cppm, otherwise it may be interpreted as a command-line option e.g. if the path starts with /Users.)

This is just one invocation, but the logic in GetNamedOutputPath() seems to cover many cases.

Agreed; once again; open to suggestions for more test cases.

Also, is it -fmodule-output or /Fo that sets the output filename (or both?)

From the Clang user guide, it appears both are required, and CMake in particular uses the 'one-phase compilation' method.

From the doc:

To generate a BMI for an importable module unit, use either the --precompile or -fmodule-output command line options.

Your test uses both, with the same filename passed to two different flags. This seems wrong to me.

Coming back after a long while...

Both arguments were provided to exercise CMake's command-line invocation which given this information might be incorrect.

@ChuanqiXu9 are you able to provide more information?

Sorry for ignoring this. --precompile will be meaningless if it is followed by a -c. So the command line in cmake's thread actually use -fmodule-output.

Please don't worry too much about what CMake's command-line invocations linked above are doing. CMake doesn't yet support modules with clang-cl, and is waiting on the work here. The command lines I posted over there are showing what unmerged changes to CMake are doing in its test suite as part of analyzing why not everything is working. This PR should focus on the functionality in clang-cl and its test should focus on the documented and intended usage. Once this is merged then CMake can be taught to do the right thing.

ChuanqiXu9

@zmodem given how important this it, I hope we can make this for clang21. Thanks!

zmodem · May 15, 2025

@zmodem given how important this it, I hope we can make this for clang21. Thanks!

I'm just a reviewer here, and I still find the patch very confusing.

And taking a step back, the whole point of clang-cl is to provide a cl.exe compatible interface to clang. I presume cl.exe does not use --precompile -x c++-module to produce BMI files. Shouldn't we try to match what they're doing?

mathstuf · May 15, 2025

That was discussed on Discourse. If/when clang-cl starts writing and reading MSVC-compatible BMIs, it can use the same flags. Until then, it would be very weird to use -ifcOutput for clang-cl when it is not using the IFC format at all.

zmodem · May 15, 2025

Sorry, I don't remember seeing that discussion, and I'm probably missing lots of modules context. As a counter argument, clang-cl uses /Yc and /Fp to generate PCH files, to match MSVC's interface although the file format is different.

More importantly, it feels like we're being very unintentional about the interface design here. Why should clang-cl's /Fo name a BMI file if that's not what MSVC does, and the document clearly says that the flag "Specifies an object (.obj) file name or directory to be used instead of the default."?

If the idea is to punt on the design, maybe the solution is just to use regular clang flags for this?

ChuanqiXu9 · May 15, 2025

@zmodem given how important this it, I hope we can make this for clang21. Thanks!

I'm just a reviewer here, and I still find the patch very confusing.

Not asking for an approval. It is just a ping : )

And taking a step back, the whole point of clang-cl is to provide a cl.exe compatible interface to clang. I presume cl.exe does not use --precompile -x c++-module to produce BMI files. Shouldn't we try to match what they're doing?

ChuanqiXu9 · May 15, 2025

Sorry, I don't remember seeing that discussion, and I'm probably missing lots of modules context. As a counter argument, clang-cl uses /Yc and /Fp to generate PCH files, to match MSVC's interface although the file format is different.

More importantly, it feels like we're being very unintentional about the interface design here. Why should clang-cl's /Fo name a BMI file if that's not what MSVC does, and the document clearly says that the flag "Specifies an object (.obj) file name or directory to be used instead of the default."?

If the idea is to punt on the design, maybe the solution is just to use regular clang flags for this?

although I don't use MSVC. I think '/Fo' is basically '-o'. Its semantic is to specify the path to the output file. And with '--precompile' the output file is BMI. And with '-c', the output is obj files. And with '-emit-llvm', the output is llvm ir. So I feel it is fine.

But of course, all of these are my thoughts and imagination. I am never a clang-cl user ever. So if now clang-cl can accept '-o', I don't mind to use that if @mathstuf agrees on this. Since most end users won't write this in my mind. The build system are the major users here.

zmodem · May 15, 2025

I think '/Fo' is basically '-o'. Its semantic is to specify the path to the output file.

No, /Fo is for object files, /Fe for the executable, /Fp for pch file etc.

So if now clang-cl can accept '-o' [...]

It works in general in clang-cl. I guess we'd have to double check if it works in the modules use case. And -fmodule-output is exposed in clang-cl as well, so it sounds like there are plenty of flags already :)

mathstuf · May 15, 2025

Is -Fo even relevant here? The BMI output name comes from -ifcOutput (or -fmodule-output=). Consumption of external modules doesn't need an object, so it would only use -fmodule-output=.

Sorry, I don't remember seeing that discussion

See this thread: https://discourse.llvm.org/t/clang-cl-exe-support-for-c-modules/72257

ChuanqiXu9 · May 16, 2025

Is -Fo even relevant here? The BMI output name comes from -ifcOutput (or -fmodule-output=). Consumption of external modules doesn't need an object, so it would only use -fmodule-output=.

Sorry, I don't remember seeing that discussion

See this thread: https://discourse.llvm.org/t/clang-cl-exe-support-for-c-modules/72257

But if we want to generate BMI only instead of obj files, we should use --precompile -o

sharadhr · May 16, 2025

No, /Fo is for object files, /Fe for the executable, /Fp for pch file etc.

Good point. I think trying to hack /Fo here is odd and it might just be better to use another flag. It was decided in Discourse that using -ifcOutput wouldn't be appropriate since Clang doesn't output MSVC-compatible IFC files, but given the complexity here (and your counter-point for PCH files) I think using -ifcOutput wouldn't be so bad here.

Is -Fo even relevant here? The BMI output name comes from -ifcOutput (or -fmodule-output=). Consumption of external modules doesn't need an object, so it would only use -fmodule-output=.

As mentioned above, the Clang C++ Standard Modules document suggests using --precompile -o for two-phase compilation, and -fmodule-output for single-phase compilation. Additionally it mentions this too:

The one-phase compilation model is simpler for build systems to implement while the two-phase compilation has the potential to compile faster due to higher parallelism.

But CMake seems to prefer the latter.

Additionally, in the CMake MR, it was determined that /clang:-o cannot be used to substitute for /Fo: (or -Fo). Even Driver.cpp makes a difference between /Fo and -o behaviour.

sharadhr · May 16, 2025

From @zmodem's comment above, MSVC documentation, and Clang-cl documentation, I believe it is now incorrect to use /Fo for the BMI output, since /Fo is for object files—for instance, foo.obj or bar.o, and definitely not for BMIs.

It might be better to use /ifcOutput even if Clang-cl doesn't output MSVC-compatible IFC files.

sharadhr · May 17, 2025

I believe the issue can be distilled further.

On the main branch, this command outputs Hello.bmi, assuming Hello.cppm exists and has export module Hello;:

clang.exe --precompile -x c++-module -o Hello.bmi -c Hello.cppm

This command does not:

clang-cl.exe --precompile -x c++-module -o Hello.bmi -c Hello.cppm

This command also does not:

clang-cl.exe --precompile -x c++-module /o Hello.bmi -c Hello.cppm

And the only lines where the driver behaviour appears to diverge, is the if block in Driver.cpp, commented as below:

// Output to a user requested destination?
if (AtTopLevel && !isa<DsymutilJobAction>(JA) && !isa<VerifyJobAction>(JA)) {
  ...
}

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Dec 24, 2024

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from d8108f0 to 350a517 Compare December 24, 2024 09:31

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from 350a517 to e1708db Compare December 24, 2024 09:32

carlocab reviewed Dec 24, 2024

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

ChuanqiXu9 self-requested a review December 25, 2024 01:36

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from e1708db to bc9b68d Compare January 5, 2025 14:51

sharadhr requested review from daniel-grumberg, cyndyishida, aaupov, maksfb, rafaelauler, ayermolo, dcci, lanza, bcardosolopes, tbaederr and Endilll as code owners January 5, 2025 14:51

sharadhr changed the base branch from release/19.x to main January 5, 2025 14:52

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from 7487d82 to ff22639 Compare January 12, 2025 21:44

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch 3 times, most recently from a839072 to 1f40c12 Compare January 12, 2025 23:56

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch 2 times, most recently from d030b5a to b6bda7b Compare February 7, 2025 21:25

zmodem reviewed Feb 10, 2025

View reviewed changes

sharadhr added 2 commits April 16, 2025 20:45

Accept /Fo and -Fo in -fmodule-output when running under CL mode

d6c2b9b

Add a test for the -fmodule-output BMI output file

59dcf4d

sharadhr changed the title ~~[clang-cl] Accept cl-style output arguments (/Fo, -Fo) for --fmodule-output~~ [clang-cl] Accept cl-style output arguments (/Fo, -Fo) for --precompile Apr 16, 2025

ChuanqiXu9 reviewed May 15, 2025

View reviewed changes

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from 91cf98b to 8653855 Compare May 15, 2025 10:42

SLASH_Fo and SLASH_Fo_COLON are aliases

d2e56db

sharadhr force-pushed the clang-cl-accept-Fo-fmodule-output branch from 8653855 to d2e56db Compare May 15, 2025 10:50

Remove -fmodule-output from test

8c2edef

Formatting fixes

f139bf1

Add -- in test

2f7aad6

Search code, repositories, users, issues, pull requests...

[clang-cl] Accept cl-style output arguments (/Fo, -Fo) for --precompile #121046

Are you sure you want to change the base?

[clang-cl] Accept cl-style output arguments (/Fo, -Fo) for --precompile #121046

Conversation

sharadhr commented Dec 24, 2024 • edited Loading

llvmbot commented Dec 24, 2024 • edited Loading

ChuanqiXu9 commented Dec 25, 2024

nikic commented Jan 3, 2025

sharadhr commented Jan 5, 2025 • edited Loading

ChuanqiXu9 commented Jan 6, 2025

github-actions bot commented Jan 12, 2025 • edited Loading

sharadhr commented Jan 13, 2025

ChuanqiXu9 commented Jan 16, 2025

rnk commented Feb 6, 2025

sharadhr commented Feb 7, 2025

ChuanqiXu9 commented Feb 8, 2025

sharadhr commented Feb 8, 2025 • edited Loading

sharadhr commented Feb 10, 2025

Choose a reason for hiding this comment

sharadhr Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharadhr Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharadhr Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChuanqiXu9 left a comment • edited Loading

Choose a reason for hiding this comment

zmodem commented May 15, 2025

mathstuf commented May 15, 2025

zmodem commented May 15, 2025

ChuanqiXu9 commented May 15, 2025

ChuanqiXu9 commented May 15, 2025

zmodem commented May 15, 2025

mathstuf commented May 15, 2025

ChuanqiXu9 commented May 16, 2025

sharadhr commented May 16, 2025 • edited Loading

sharadhr commented May 16, 2025 • edited Loading

sharadhr commented May 17, 2025

[clang-cl] Accept `cl`-style output arguments (`/Fo`, `-Fo`) for `--precompile` #121046

[clang-cl] Accept `cl`-style output arguments (`/Fo`, `-Fo`) for `--precompile` #121046

sharadhr commented Dec 24, 2024 •

edited

Loading

llvmbot commented Dec 24, 2024 •

edited

Loading

sharadhr commented Jan 5, 2025 •

edited

Loading

github-actions bot commented Jan 12, 2025 •

edited

Loading

sharadhr commented Feb 8, 2025 •

edited

Loading

sharadhr Feb 10, 2025 •

edited

Loading

sharadhr Apr 16, 2025 •

edited

Loading

sharadhr Feb 10, 2025 •

edited

Loading

ChuanqiXu9 left a comment •

edited

Loading

sharadhr commented May 16, 2025 •

edited

Loading

sharadhr commented May 16, 2025 •

edited

Loading