This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Do not auto-enable header modules with -std=c++20
AbandonedPublic

Authored by ilya-biryukov on May 17 2022, 6:27 AM.

Details

Reviewers
sammccall
rsmith
Group Reviewers
Restricted Project
Summary

This only shows up if one passes module maps to Clang in -std=c++20
modes, e.g. for layering checks.

Diff Detail

Event Timeline

ilya-biryukov created this revision.May 17 2022, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:27 AM
ilya-biryukov requested review of this revision.May 17 2022, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 6:27 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
sammccall accepted this revision.May 17 2022, 7:03 AM

LG, but I think this is choosing a (new) public name for clang modules/header modules, so maybe Richard or others might want to weigh in?

clang/lib/Driver/ToolChains/Clang.cpp
3631

These interacting flags are complicated, and I think relying on how the default value is computed makes it harder to reason about.

Can we explicitly set -f{no}-header-modules whenever any version of modules is available?
i.e.

if (HaveModules) {
  if (HaveClangModules)
    push("-fheader-modules")
  else
    push ("-fno-header-modules")
}

(In the long run, it would be much clearer for -fheader-modules to default to false, and explicitly set it whenever it's needed, but this is a larger cleanup)

6542

this change looks correct but unrelated, can we split into a separate change?
(Mostly because either change might break stuff downstream, and they could be reverted separately)

This revision is now accepted and ready to land.May 17 2022, 7:03 AM
  • Revert gnu++20 fix
  • Always pass -fheader-modules/-fno-header-modules when modules are enabled
ilya-biryukov marked 2 inline comments as done.

LG, but I think this is choosing a (new) public name for clang modules/header modules, so maybe Richard or others might want to weigh in?

@rsmith could you confirm you are happy with the changes (and naming)?
And let us know if there are others we should loop in.

clang/lib/Driver/ToolChains/Clang.cpp
3631

Makes sense, I think it's clearer.
As for using false as a default, the mentioned cleanup is that we need to update all tests to specify -fheader-modules explicitly.
I'm also not sure if there are any guarantees for -cc1 that we have to adhere to. My assumption is that -cc1 has no stability guarantees, but I'm not 100% certain.

6542

Good point, will send it as a separate review.

Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather, I think we should be thinking about moving towards header modules simply always being something that Clang can do, with no flag to control that.

That said: this new flag is not actually turning off header modules -- we'll still treat headers listed in module maps as having modular semantics with this flag specified (especially if -fmodules-local-submodule-visibility is enabled, but some modules semantics such as checking for undeclared dependencies are applied regardless). Rather, this flag will cause us to parse header modules on the fly, as part of the same parsing action, rather than building and importing a separate AST for them. And I think that is a reasonable thing to have a flag for. Maybe -fheader-modules=parse (to parse them on the fly) versus -fheader-modules=import (to import them from a pcm file)? This would also leave the door open for being able to specify whether modular semantics are enabled for header modules or not, as a different value for the same flag.

MaskRay added a reviewer: Restricted Project.May 18 2022, 1:24 PM

Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather, I think we should be thinking about moving towards header modules simply always being something that Clang can do, with no flag to control that.

It feels unclear that using Clang's legacy module maps functionality _is_ a reasonable forward-looking way to provide this functionality for the C++20 standard. It feels to me like the models are fairly distinct and we might be better off disentangling C++20 modules from module-maps entirely (e.g. that we may continue to support Clang modules in C++20 for compatibility, but not as a way to support any C++20 standard features).

Has there been some previous discussion on this topic that concluded that we do want module maps for C++20 modules support?

ilya-biryukov planned changes to this revision.May 19 2022, 8:16 AM
ilya-biryukov marked 2 inline comments as done.

Richard, thanks for course correcting. I was under impression that header modules are not in the standard, my mistake.

It looks like this particular change actually breaks standard compatibility as we also use the same parsing action and don't build a module separately on import.

[module.import]p5
A module-import-declaration that specifies a header-name H imports a synthesized header unit, which is a
translation unit formed by applying phases 1 to 7 of translation (5.2) to the source file or header nominated
by H, which shall not contain a module-declaration.

We are definitely allowed to use the same parsing action for #include .

[cpp.include]p7
If the header identified by the header-name denotes an importable header (10.3), it is implementation-defined
whether the #include preprocessing directive is instead replaced by an import directive (15.5) of the
form

So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?

To take a step back, the original problem was to allow running layering checks without requiring .pcm files even in -std=c++20.
Are there alternatives I might be missing?

So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?

Per cpp.includep7, "If the header identified by the header-name denotes an importable header ([module.import]), it is implementation-defined whether the #include preprocessing directive is instead replaced by an import directive ([cpp.import]) of the form
import header-name ; new-line"

For Clang, I would expect the implementation-defined behavior to derive from the existence of a module map that nominates a header as being importable.

So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?

Per cpp.includep7, "If the header identified by the header-name denotes an importable header ([module.import]), it is implementation-defined whether the #include preprocessing directive is instead replaced by an import directive ([cpp.import]) of the form
import header-name ; new-line"

For Clang, I would expect the implementation-defined behavior to derive from the existence of a module map that nominates a header as being importable.

Sure, that's what Clang does now. It's totally reasonable.
It seems we can get away with our use-case by passing -fno-modules flag as we don't really use C++20 modules. It should still provide the layering check.

ilya-biryukov abandoned this revision.May 20 2022, 8:41 AM

It looks like this particular change actually breaks standard compatibility as we also use the same parsing action and don't build a module separately on import.

[module.import]p5
A module-import-declaration that specifies a header-name H imports a synthesized header unit, which is a
translation unit formed by applying phases 1 to 7 of translation (5.2) to the source file or header nominated
by H, which shall not contain a module-declaration.

For the record, the intention is that (with local submodule visibility enabled) we behave "as if" we preprocessed separately, even if we do so using the same parsing action: we store various parts of the preprocessor state (most notably, the defined macros map) per-module, so that when we switch modules, we get the same preprocessing behavior as if we'd started from a clean slate. Fundamentally, Clang's model is that a single ASTContext and a single preprocessing action can cover multiple translation units, and that includes the possibility that a single preprocessor or Sema object can process multiple translation units, and we make this work by tracking which macros and declarations and similar should be visible where, based on which modules are currently imported. This is intended to be a conforming strategy for implementing C++20 header units.