Page MenuHomePhabricator

[Modules] Don't support clang module and c++20 module at the same time
AbandonedPublic

Authored by ChuanqiXu on Mon, Nov 8, 3:30 AM.

Details

Summary

Background: https://lists.llvm.org/pipermail/cfe-dev/2021-November/069237.html
Simply, we think it is weird to support c++20 module and clang module at the same time.
Further more, there was no way to judge if the compiler was running in clang module instead of c++20 module. Both in clang module and c++20 module, LangOpts::Modules would be true. So it makes it hard to implement c++20 semantics.

Main change in this patch:

  • Emit an error if the driver/frontend detect that the input implies that it want to use clang module and c++20 module at the same time.
  • Add variable LangOpts::ClangModules. LangOpts::ClangModules would be true only if the user specified -fmodules.
  • Remove variable LangOpts::Modules. Add member function LangOpts::hasModules(). Replace all the uses of LangOpts::Modules with LangOpts::hasModules(). LangOpts::hasModules() would return true in case clang modules or c++20 modules are enabled.

And the thing confuses me is the wanted behavior of clang/Modules/ms-enums.cpp. See the inline comments for details.

Another thing Aaron proposed in the mailing list is to rename '-fmodules' to '-fclang-modules'. It sounds good. But this patch didn't contain this change since I think change the command line option may be a break change and need more discussion. Especially clang modules are used broadly.

Test-Plan: check-all (except clang/Modules/ms-enums.cpp)

Diff Detail

Unit TestsFailed

TimeTest
60 msx64 debian > Clang.Modules::ms-enums.cpp
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/ms-enums.cpp.tmp

Event Timeline

ChuanqiXu created this revision.Mon, Nov 8, 3:30 AM
ChuanqiXu requested review of this revision.Mon, Nov 8, 3:30 AM

This sounds like an enum.

enum class ModuleKind {
C++20,
Clang,
Unsupported
}
ChuanqiXu added inline comments.Mon, Nov 8, 3:42 AM
clang/test/Modules/ms-enums.cpp
1

This test couldn't pass. If we input -std=c++20 -fmodules -fno-cxx-modules, it would output

missing '#include "A.h"'; 'fwd_enum' must be declared before it is used

Instead of the message it wanted.

The secret is in: https://github.com/llvm/llvm-project/blob/99023627010bbfefb71e25a2b4d056de1cbd354e/clang/lib/Lex/PPDirectives.cpp#L788-L793. It tells that even if the compiler couldn't find the header, in the case C++20 modules is required, the compiler shouldn't emit an error for inclusion directly. So the test would fail after we disable the use of C++20 modules.

And I am not sure what we should do here. Should we change the expected error message? In that way the test shouldn't be under Modules. Or should we delete it? It looks not good. And I am wondering what's the intention of this file. It looks like it want to test module maps. But it is not standard c++20 module.

BTW, I have tried to rewrite this test into c++20 module. It looks like:

// A.cpp
export module A;
export enum fwd_enum;
// B.cpp
export module B;
export import A;
// ms-enums.cpp
import B;
...

But it wouldn't emit errors. So I am really not sure what should we do here.

ChuanqiXu updated this revision to Diff 385440.Mon, Nov 8, 3:46 AM
ChuanqiXu set the repository for this revision to rG LLVM Github Monorepo.
ChuanqiXu added a project: Restricted Project.

This sounds like an enum.

enum class ModuleKind {
C++20,
Clang,
Unsupported
}

In fact, there is another value ModuleTS. But it looks not bad. And the variable CPlusPlusModules and ClangModules comes from options directly (See Options.td for details). So it looks like we couldn't eliminate CPlusPlusModules and ClangModules after we introduce this enum. So the overall complexity didn't get decreased to me.

ChuanqiXu edited the summary of this revision. (Show Details)Mon, Nov 8, 3:53 AM
tschuett added a comment.EditedMon, Nov 8, 6:19 AM

It would enforce that there is exactly one module mode live.

ChuanqiXu updated this revision to Diff 385680.Mon, Nov 8, 7:13 PM

Update part in clang-tools-extra

Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 8, 7:13 PM

It would enforce that there is exactly one module mode live.

Yeah. But my consideration is that it would enforce another variable and it couldn't eliminate the two existing variables. So it looks redundant to me. We have made this check in driver and frontend part. So it looks sufficient to me.
I admit that the current method has limitations. For example, it is possible that both ClangModules and CPlusPlusModules could be wrote as true in Lexer part and Sema part (Maybe we could add an assertion in hasModules()). And your suggestion could solve this. But I have another question about the implementation, where should we put this variable and when should we fill the value for it? I failed to use a new variable in LangOpts and initialize it in constructor of LangOpts since both ClangModules and CPlusPlusModules is zero.
And even in this suggestion, user could still access ClangModules and CPlusPlusModules. And they could rewrite them too. So I am wondering if it would be more confusing.

ChuanqiXu added inline comments.Mon, Nov 8, 7:23 PM
clang/lib/Driver/ToolChains/Clang.cpp
3445–3447

From our discussion, fmodules shouldn't be suppressed by fno-cxx-modules. And the tests shows that is wouldn't be a problem to relax this.

jansvoboda11 added inline comments.
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
7

Please undo the whitespace changes in ClangScanDeps tests.

9

Can you explain why -fcxx-modules is removed? We want to explicitly enable Clang modules for C++ inputs here. That's off by default in our downstream repo and we'd like to keep this upstream to prevent conflicts. (it's benign upstream)

ChuanqiXu added inline comments.Tue, Nov 9, 3:12 AM
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
7

It looks like that it's formatted by clang-format surprisingly. I would undo this manually.

9

According to the discussion in the link, I think it is in consensus that we decide to not support clang modules and c++20 modules at the same time. (I know many people may not have visited it. If any one disagree with the higher idea, I think it would be better to reply in that thread).
So the original test case would fail.

We want to explicitly enable Clang modules for C++ inputs here.

I am not sure if I missed any thing. Personally, it looks like the test now would test clang modules for c++ inputs. Since it has -fmodule option so that clang module is enabled and the input is C++ (from its suffix). Do I misunderstand you?

jansvoboda11 added inline comments.Tue, Nov 9, 3:24 AM
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
7

Thanks.

9

According to the discussion in the link, I think it is in consensus that we decide to not support clang modules and c++20 modules at the same time. (I know many people may not have visited it. If any one disagree with the higher idea, I think it would be better to reply in that thread).
So the original test case would fail.

What's the error message? As I say, we're enabling Clang modules for C++ input here, not C++20 modules.

We want to explicitly enable Clang modules for C++ inputs here.

I am not sure if I missed any thing. Personally, it looks like the test now would test clang modules for c++ inputs. Since it has -fmodule option so that clang module is enabled and the input is C++ (from its suffix). Do I misunderstand you?

Right. What I'm saying is that in our downstream repo, -fmodules is not enough to enable Clang modules for C++ inputs, you need to do it via -fcxx-modules. And we'd like to keep it upstream, even though it's benign here, to avoid conflicts between the repos.

MyDeveloperDay added inline comments.
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
7

Just as a drive by assuming you are using a fairly recent clang-format, then clang-format should ONLY clang-format .json files if you have added the following code into your .clang-format

BasedOnStyle: LLVM
ColumnLimit: 0
AlwaysBreakTemplateDeclarations: No
Language: Cpp
---
Language: Json
BasedOnStyle: LLVM

Doing so would render like this

$ clang-format cdb.json
[
  {
    "directory": "DIR",
    "command": "clang -E DIR/header-search-pruning.cpp -Ibegin -I1 -Ia -I3 -I4 -
I5 -I6 -Ib -I8 -Iend DEFINES -fmodules -fcxx-modules -fmodules-cache-path=DIR/mo
dule-cache -fimplicit-modules -fmodule-map-file=DIR/module.modulemap",
    "file": "DIR/header-search-pruning.cpp"
  }
]
ChuanqiXu added inline comments.Tue, Nov 9, 3:54 AM
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
7

Oh, thanks for your kindly guide. But I have updated this manually hhh. BTW, the clang-format I am using is:

git diff -U0 --no-color --relative HEAD^ | clang/tools/clang-format/clang-format-diff.py -p1 -i

This one is recommended by the authority documentation so I think many others would use this too.

9

What's the error message? As I say, we're enabling Clang modules for C++ input here, not C++20 modules.

The error message is added in this patch. After this patch landed, the original intentional behavior would be -fmodules to enable clang module and -fcxx-modules to enable C++20 modules.

Right. What I'm saying is that in our downstream repo, -fmodules is not enough to enable Clang modules for C++ inputs, you need to do it via -fcxx-modules. And we'd like to keep it upstream, even though it's benign here, to avoid conflicts between the repos.

Got it. But it conflicts with the idea that disable clang module and c++20 module at the same time. Personally, I think it would be better to edit the code in downstream. @aaron.ballman sorry if I ping you too frequently. But I think now we need input from you.

ChuanqiXu updated this revision to Diff 385765.Tue, Nov 9, 3:57 AM
ChuanqiXu added a reviewer: jansvoboda11.

Undo unnecessary style change in *.json files

jansvoboda11 added inline comments.Tue, Nov 9, 6:34 AM
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
9

Ah, I understand now, thanks for explaining.

I'm worried about changing the behavior of a driver flag, we generally don't break existing driver options. Have you considered keeping the -fmodules and -fcxx-modules semantics intact and instead add new -fno-c++20-modules flag or something like that?

aaron.ballman added inline comments.Tue, Nov 9, 8:06 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
162–163

I think we can get away with err_drv_argument_not_allowed_with for this diagnostic -- this also will tell the user *which* options are in conflict.

clang/include/clang/Basic/DiagnosticFrontendKinds.td
244

I think it's reasonable to give a diagnostic from the driver when the user does this, but in the FE for a cc1 invocation, I'm a bit less sympathetic to giving people nice diagnostics. Do we need this diagnostic, or can we pick one option to be the "winner" in this case?

clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
9

I'm worried about changing the behavior of a driver flag, we generally don't break existing driver options. Have you considered keeping the -fmodules and -fcxx-modules semantics intact and instead add new -fno-c++20-modules flag or something like that?

The goal is not "let users disable C++20 modules", it's "ensure the user cannot enable two different kinds of modules at the same time". What I think we want to avoid is "C++20 mode, but with Clang modules instead of standard modules" or "C++20 mode, but with both clang and standard modules", etc. I think the support matrix that makes sense to me is:

C++20 mode: you get standard modules, no other module schemes are allowed
Pre-C++20 modes: you can opt into Clang modules or you can opt into C++20 modules
Non-C++ modes: you can opt into Clang modules (maybe we want to also support C++20 modules in C, but I'm less certain)

However, I'm not certain if other people agree. I'm basing this on the belief that we don't want modules features to have different semantics within the same program. While this does make it harder for people with a C++17 code base using Clang modules to upgrade to C++20, I suggest that users shouldn't enable -std=c++20 until their code base is ready to make that leap and that includes dealing with the incompatible modules features.

@dblaikie -- I've seen you responding to some of the WG21 discussions around modules. Do you have opinions here?

dblaikie added inline comments.Tue, Nov 9, 9:03 AM
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
9

Mixed feelings - it's still a pretty exploratory space.

Backwards compatibility's pretty important - though, yeah, seems some folks on the C++ committee have different ideas about that & insist C++20 presents a pretty hard break in terms of how libraries vend interfaces, etc. Sorry, that's a bit of an aside/only vaguely connected.

I'd /guess/ Google would want some way to combine C++20 and Clang Header Modules if/when we get to the rollout - and Apple's probably in that situation too?

I'm honestly probably a bit too out of the loop to make good calls here - I've weighed in on a few conversations to try to help move them forward here and there where I thought I could add value, but probably not informed enough really.

How far are Clang Header Modules from C++20 header units? Far enough off that we can't just treat things that are Clang Header Modules pre-C++20 as C++20 header units when building in C++20 mode?

Bigcheese added inline comments.
clang/test/ClangScanDeps/Inputs/header-search-pruning/cdb.json
9

I'd /guess/ Google would want some way to combine C++20 and Clang Header Modules if/when we get to the rollout - and Apple's probably in that situation too?

Very much so. We would like Clang modules to just be modeled as C++20 Header Units, which was a large reason for Header Units existing in the first place.

Specifically module maps are how Clang implements the "set of implementation defined headers" requirement for C++20 Header Units.

rsmith requested changes to this revision.Tue, Nov 9, 11:30 AM

The ideal situation here would be to have a single semantic model that is a superset of the various combinations of behaviors we want. Further entrenching Clang modules and C++20 modules as being different things would be a backwards step, and would be harmful for users of Clang modules who want to incrementally adopt C++20 modules. We shouldn't have a separate language option flag for "clang modules" -- that should be a different kind of module, not a different compilation mode; if a clang header module needs different treatment from a C++20 header unit, we should base that on the kind of the current clang::Module, not on a language option.

In terms of what the various LangOptions flags mean:

LangOptions::Modules corresponds to the expectation that modular headers will be imported rather than entered textually.
LangOptions::ModulesTS enables C++ Modules TS language syntax. This corresponds to -fmodules-ts. We should probably deprecate it.
LangOptions::CPlusPlusModules enables C++20 modules syntax. This is enabled by -std=c++20.
LangOptions::ModulesLocalVisibility indicates that in this compilation, visibility is not monotonically increasing. This corresponds to -fmodules-local-submodule-visibility. This is the case when we might compile multiple source files or header units into a single AST, and we want to reset the visibility state at the end of each such unit. For example, this happens when building a Clang module containing multiple headers. This really should be enabled automatically when appropriate, but for now it's enabled by default with CPlusPlusModules and ModulesTS.

The only combination that's incompatible here is ModulesTS plus CPlusPlusModules, because they have overlapping but different syntax.

This revision now requires changes to proceed.Tue, Nov 9, 11:30 AM
ChuanqiXu abandoned this revision.Tue, Nov 9, 5:59 PM

Oh, OK. If this is not the direction the community want, we might not go on this direction.