[clang][Modules] Respect -fno-cxx-modules as a driver flag

[cyndyishida]

The mentioned flag is already both a cc1 & driver flag; however, whether it is respected was tied to either:

1. Whether it was passed as a cc1 option (Xclang)
2. or -fmodules accompanying it

This poses a consistency problem where std=c++20 enables the modules feature, independent of other module settings.
This patch resolves this issue by checking for the presence unconditionally & passing it down to cc1 when applicable.

[llvmbot]

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Cyndy Ishida (cyndyishida)

Changes

The mentioned flag is already both a cc1 & driver flag; however, whether it is respected was tied to either:

1. Whether it was passed as a cc1 option (Xclang)
2. or -fmodules accompanying it

This poses a consistency problem where std=c++20 enables the modules feature, independent of other module settings.
This patch resolves this issue by checking for the presence unconditionally & passing it down to cc1 when applicable.

---

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

2 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Clang.cpp (+7-4)
• (modified) clang/test/Driver/modules.mm (+3)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 7d0c142ecd061..9d882dbfd0c65 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3881,17 +3881,17 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
                                  const ArgList &Args, const InputInfo &Input,
                                  const InputInfo &Output, bool HaveStd20,
                                  ArgStringList &CmdArgs) {
-  bool IsCXX = types::isCXX(Input.getType());
-  bool HaveStdCXXModules = IsCXX && HaveStd20;
+  const bool IsCXX = types::isCXX(Input.getType());
+  const bool HaveStdCXXModules = IsCXX && HaveStd20;
   bool HaveModules = HaveStdCXXModules;
 
   // -fmodules enables the use of precompiled modules (off by default).
   // Users can pass -fno-cxx-modules to turn off modules support for
   // C++/Objective-C++ programs.
+  const bool AllowedInCXX = Args.hasFlag(options::OPT_fcxx_modules,
+                                         options::OPT_fno_cxx_modules, true);
   bool HaveClangModules = false;
   if (Args.hasFlag(options::OPT_fmodules, options::OPT_fno_modules, false)) {
-    bool AllowedInCXX = Args.hasFlag(options::OPT_fcxx_modules,
-                                     options::OPT_fno_cxx_modules, true);
     if (AllowedInCXX || !IsCXX) {
       CmdArgs.push_back("-fmodules");
       HaveClangModules = true;
@@ -3900,6 +3900,9 @@ static bool RenderModulesOptions(Compilation &C, const Driver &D,
 
   HaveModules |= HaveClangModules;
 
+  if (HaveModules && !AllowedInCXX)
+    CmdArgs.push_back("-fno-cxx-modules");
+
   // -fmodule-maps enables implicit reading of module map files. By default,
   // this is enabled if we are using Clang's flavor of precompiled modules.
   if (Args.hasFlag(options::OPT_fimplicit_module_maps,
diff --git a/clang/test/Driver/modules.mm b/clang/test/Driver/modules.mm
index d1536c73a1f3b..f0b06697a37ce 100644
--- a/clang/test/Driver/modules.mm
+++ b/clang/test/Driver/modules.mm
@@ -3,6 +3,9 @@
 // RUN: %clang -fmodules -fno-cxx-modules -### %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MODULES %s
 // CHECK-NO-MODULES-NOT: -fmodules
 
+// RUN: %clang -std=c++20 -fno-cxx-modules -### %s 2>&1 | FileCheck -check-prefix=CHECK-NO-CPP-20-MODULES %s
+// CHECK-NO-CPP-20-MODULES: -fno-cxx-modules
+
 // RUN: %clang -fmodules -### %s 2>&1 | FileCheck -check-prefix=CHECK-HAS-MODULES %s
 // RUN: %clang -fmodules -fno-cxx-modules -fcxx-modules -### %s 2>&1 | FileCheck -check-prefix=CHECK-HAS-MODULES %s
 // CHECK-HAS-MODULES: -fmodules

[cyndyishida]

This PR would also resolve: #57432 , but I'm not sure if its still relevant for the affected client code

[ChuanqiXu9]

The bad side of -fcxx-modules is, I remember, in MacOS, it has a different meaning to enable clang modules feature instead of C++20 modules.

If we want, I think we can introduce a new language options for C++20 modules only. If it enabled, the C++20 modules are enabled. If it is disabled, the C++20 modules will be disabled.

[cyndyishida]

If we want, I think we can introduce a new language options for C++20 modules only. If it enabled, the C++20 modules are enabled. If it is disabled, the C++20 modules will be disabled.

Thanks for offering this suggestion, but I'm not sure if that's necessary yet. If the lack of distinction actively causes adoption issues for either clang modules or c++20 modules, then I'd say yes, but I'm not aware of this. In an ideal world, I'd hope these two definitions of modules can be enabled & intermixed without issues, especially if header units end up being represented with modulemaps (or something like it).

FWIW, I actually noticed the issue this PR fixes when trying to clean up some of the Darwin-toolchain compiler divergence around this area.

[Bigcheese]

This looks good to me. I think having -fno-cxx-modules mean no modules (Clang or standard) when in C++ mode makes sense, and matches how we've been using it.