@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Sharadh Rajaraman (sharadhr)

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

The tests are failing and it looks relevent.

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

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.

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)

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

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.

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

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.

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?

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

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.

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

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

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

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.

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.

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)) {
  ...
}

@mathstuf in #140825 (comment) , xmake dev said he can fix the problem by /clang:-o. I am wondering if you can did similar thing in CMake if here is an argument? (Semantics of /Fo in clang-cl)

xmake dev said he can fix the problem by /clang:-o

We tried this, but it didn't work when generating multiple output files. In particular, a scan-with-pch test still fails. Perhaps @Arthapz needs to widen the test scenarios for xmake.

CMake Error at R:/cmake/Tests/RunCMake/RunCMake.cmake:289 (message):
examples/scan-with-pch-build - FAILED:

Result is [1], not [0].

Command was:

command> "R:/cmake/out/build/RelWithDebInfo/bin/cmake.exe" "--build" "." "--config" "Debug"

Actual stdout:

actual-stdout> [1/5] Building CXX object CMakeFiles\simple.dir\cmake_pch.cxx.obj
actual-stdout> FAILED: CMakeFiles/simple.dir/cmake_pch.cxx.obj
actual-stdout> R:\llvm-project\build\x64-release\bin\clang-cl.exe  /nologo -TP   /DWIN32 /D_WINDOWS /EHsc /Ob0 /Od /RTC1 -std:c++20 -MDd -Zi /YcR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/cmake_pch.hxx /FpR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/./cmake_pch.cxx.pch /FIR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/cmake_pch.hxx /showIncludes -clang:-oCMakeFiles\simple.dir\cmake_pch.cxx.obj /FdCMakeFiles\simple.dir\ -c -- R:\cmake\out\build\RelWithDebInfo\Tests\RunCMake\CXXModules\examples\scan-with-pch-build\CMakeFiles\simple.dir\cmake_pch.cxx
actual-stdout> clang-cl: error: cannot specify -o when generating multiple output files
actual-stdout> ninja: build stopped: subcommand failed.

xmake dev said he can fix the problem by /clang:-o

We tried this, but it didn't work when generating multiple output files. In particular, a scan-with-pch test still fails. Perhaps @Arthapz needs to widen the test scenarios for xmake.

CMake Error at R:/cmake/Tests/RunCMake/RunCMake.cmake:289 (message):
examples/scan-with-pch-build - FAILED:

Result is [1], not [0].

Command was:

command> "R:/cmake/out/build/RelWithDebInfo/bin/cmake.exe" "--build" "." "--config" "Debug"

Actual stdout:

actual-stdout> [1/5] Building CXX object CMakeFiles\simple.dir\cmake_pch.cxx.obj
actual-stdout> FAILED: CMakeFiles/simple.dir/cmake_pch.cxx.obj
actual-stdout> R:\llvm-project\build\x64-release\bin\clang-cl.exe  /nologo -TP   /DWIN32 /D_WINDOWS /EHsc /Ob0 /Od /RTC1 -std:c++20 -MDd -Zi /YcR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/cmake_pch.hxx /FpR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/./cmake_pch.cxx.pch /FIR:/cmake/out/build/RelWithDebInfo/Tests/RunCMake/CXXModules/examples/scan-with-pch-build/CMakeFiles/simple.dir/cmake_pch.hxx /showIncludes -clang:-oCMakeFiles\simple.dir\cmake_pch.cxx.obj /FdCMakeFiles\simple.dir\ -c -- R:\cmake\out\build\RelWithDebInfo\Tests\RunCMake\CXXModules\examples\scan-with-pch-build\CMakeFiles\simple.dir\cmake_pch.cxx
actual-stdout> clang-cl: error: cannot specify -o when generating multiple output files
actual-stdout> ninja: build stopped: subcommand failed.

well i'm still experimenting,
i'm not using /clang: at scanning step (and i don't remove -Fo flag), only at build step
we use scanning only for deps, we don't use primary-output, objectfile and bmifile path are generated by xmake
pch with module fail but for an other reason

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

FYI, branching for LLVM v21 is less than two weeks away

I think this patch needs to go back to the drawing board.

I agree. /Fo is strictly for object files—that is, .obj as a result of a compilation—that is, /c, whereas -o is meant for 'any output'.

My understanding is that @Arthapz made /clang:-o work with upstream, even without this patch. CMake needs to rework its understanding of the Clang compilation model and perhaps rework its invocation of BMI compilation and clang-scan-deps.

CC @mathstuf @bradking

I don't really care how things are spelled as long as they're possible. Don't take what CMake does for clang-cl and modules as gospel as it is very much not declared as supported at all. We can rework how CMake constructs command lines for clang-cl and modules as needed.

If this is consensus, then I will rework this PR from scratch to expose /ifcOutput (or any other appropriate flag, just not /Fo or -o) with clang-cl.