Page MenuHomePhabricator

[C++2a] [Module] Support extern C/C++ semantics
ClosedPublic

Authored by ChuanqiXu on Sep 21 2021, 7:21 PM.

Details

Summary

According to [[ https://eel.is/c++draft/module.unit#7.2.3 | [module.unit]p7.2.3]], a declaration within a linkage-specification should be attached to the global module.
This let user to forward declare types across modules. Here is an example: https://godbolt.org/z/vEWjdrdq9

Test Plan: check-all, https://godbolt.org/z/vEWjdrdq9

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Sep 21 2021, 7:21 PM
ChuanqiXu created this revision.
ChuanqiXu added a reviewer: Quuxplusone.
ChuanqiXu updated this revision to Diff 375767.Sep 28 2021, 7:52 PM

Update test.

Hello.

Just some minor nibbles, as I am a bit out of the loop on modules.

clang/include/clang/Basic/DiagnosticSemaKinds.td
10842–10845 ↗(On Diff #375767)
def **err_module_language_linkage_no_global** : Error <
  "The declaration %0**, which** appears within a linkage-specification**, is not "
  "attached to **the** global module.">;

My two cents since I think the last sentence did not seem to explain the problem very well.

clang/lib/Sema/SemaDecl.cpp
9353 ↗(On Diff #375767)
16437 ↗(On Diff #375767)

This block is of code is identical to the one above. Can you put this into a function?

clang/lib/Sema/SemaModule.cpp
71
clang/test/CXX/module/module.linkage_specification/Inputs/h1.h
7 ↗(On Diff #375767)

Please if possible make your editor set new lines at EOF.

Adding some more reviewers and subscribing the mailing lists.

clang/include/clang/Basic/DiagnosticSemaKinds.td
10842 ↗(On Diff #375767)

typo, should be: err_module_language_linkage_no_global

10842–10845 ↗(On Diff #375767)

Diagnostics in Clang are intentionally ungrammatical, so they should not have leading capitalization or most terminating punctuation. I agree that the diagnostic wording here doesn't really explain what's going wrong.

clang/lib/Sema/SemaDecl.cpp
9351 ↗(On Diff #375767)

I'm a bit confused here. [module.unit]p7 is describing what module a declaration attached to under which circumstances. I don't see a constraint there which should result in a diagnostic. My reading of https://eel.is/c++draft/module.unit#6 is that the global module always exists, so we should be able to attach declarations to it at any point. Am I misunderstanding?

clang/test/CXX/module/module.linkage_specification/Inputs/h1.h
7 ↗(On Diff #375767)

+1 -- please add a newline to the end of each of these files that.

aaron.ballman set the repository for this revision to rG LLVM Github Monorepo.Oct 8 2021, 5:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2021, 5:04 AM
ChuanqiXu updated this revision to Diff 378401.Oct 8 2021, 7:50 PM

Address the comments.

ChuanqiXu added inline comments.Oct 8 2021, 7:51 PM
clang/lib/Sema/SemaDecl.cpp
9351 ↗(On Diff #375767)

Yeah, after looking into this, I think you are right. We should create the global module when we need it but failed to find it.

Instead of moving functions and classes/enums into the global module after creating them, it would be better to push a module scope for the global module when entering a C or C++ language linkage specification and pop the module scope when leaving the linkage specification. There are lots of other kinds of declaration that can appear inside a linkage specification (for example, variables), and doing it that way would properly handle all of them, not only functions and classes.

clang/include/clang/Sema/Sema.h
2216

I think this should live on the ModuleScope instead of being separate. If we process multiple modules in a single compilation, each needs to have its own global module fragment so that we can restrict the visibility of the global module fragment to the module unit in which it was declared.

ChuanqiXu updated this revision to Diff 379907.Oct 14 2021, 8:32 PM

Address the comments from @rsmith. It looks much better now.

aaron.ballman added inline comments.Oct 29 2021, 6:05 AM
clang/lib/Sema/SemaDeclCXX.cpp
16155

Module * instead of auto * (the type is not spelled out in the initialization).

clang/lib/Sema/SemaModule.cpp
71–72

Module *

707–708

Please spell these out as well.

712–715

Can we use emplace_back() and construct in place rather than constructing piecemeal?

722–723

FWIW, this seems to be failing to compile according to the precommit CI.

FAILED: tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DCLANG_ROUND_TRIP_CC1_ARGS=ON -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/lib/Sema -I/var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema -I/var/lib/buildkite-agent/builds/llvm-project/clang/include -Itools/clang/include -Iinclude -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -gmlt -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o -MF tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o.d -o tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaModule.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema/SemaModule.cpp
/var/lib/buildkite-agent/builds/llvm-project/clang/lib/Sema/SemaModule.cpp:722:53: error: member reference type 'clang::Module *' is a pointer; did you mean to use '->'?
  assert(!ModuleScopes.empty() && getCurrentModule().isGlobalModule() &&
                                  ~~~~~~~~~~~~~~~~~~^
                                                    ->
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
     (static_cast <bool> (expr)                                         \
                          ^~~~
1 error generated.
clang/test/CXX/module/module.linkage_specification/p1.cpp
1 ↗(On Diff #379907)

Might as well switch all these to use -std=c++20.

ChuanqiXu updated this revision to Diff 383376.Oct 29 2021, 8:48 AM

Address comments.

ChuanqiXu marked 7 inline comments as done.Oct 29 2021, 8:49 AM
ChuanqiXu added inline comments.
clang/lib/Sema/SemaModule.cpp
722–723

Oh, my bad. I forgot to run tests under debug locally.

ChuanqiXu updated this revision to Diff 388125.Nov 18 2021, 1:18 AM

Change includes:

  • Add parent for newly created global module fragment.
  • Add a test.
  • Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

ChuanqiXu updated this revision to Diff 388151.Nov 18 2021, 3:54 AM

Format codes.

Change includes:

  • Add parent for newly created global module fragment.
  • Add a test.
  • Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

@ChuanqiXu -- I'm happy to take a look (all of your modules patches are still on my review queue), but I've been in WG14 meetings all week this week and will be on vacation all week next week. Given the complexity and importance of the patches, it may be a bit before I can give this a thorough review. (If you get approval from other trusted reviewers before I get to anything, please don't let me hold any of the patches up!)

Change includes:

  • Add parent for newly created global module fragment.
  • Add a test.
  • Rename test directory. Suddenly I found that the names of the tests in CXX directory are corresponds to the standard one by one. So my name before is not good.

@aaron.ballman @urnathan It looks like @rsmith is busy now. Could you please help to review this one? Many Thanks.

@ChuanqiXu -- I'm happy to take a look (all of your modules patches are still on my review queue), but I've been in WG14 meetings all week this week and will be on vacation all week next week. Given the complexity and importance of the patches, it may be a bit before I can give this a thorough review. (If you get approval from other trusted reviewers before I get to anything, please don't let me hold any of the patches up!)

Got it. Many thanks! It is great enough to know the review process is going on. I completely understand that we should be very careful about these patches.

rsmith accepted this revision.Dec 7 2021, 6:30 PM

Thanks, this looks great. A couple of minor suggestions.

clang/include/clang/Lex/ModuleMap.h
542–543

We generally refer to sections of the C++ standard by name not by URL.

clang/lib/Sema/SemaDeclCXX.cpp
16158

I think this should be ModulePrivate -- such a global module fragment should not be visible to importers of this module. (I think it probably doesn't matter in practice since visibility is generally monotonically increasing while compiling a module unit, but ModulePrivate seems more correct in principle.)

This revision is now accepted and ready to land.Dec 7 2021, 6:30 PM
ChuanqiXu updated this revision to Diff 392625.Dec 7 2021, 7:35 PM

Address comments.

This revision was landed with ongoing or failed builds.Dec 7 2021, 9:29 PM
This revision was automatically updated to reflect the committed changes.
llvm-project/clang/lib/Sema/SemaModule.cpp:715:70: warning: missing field 'OuterVisibleModules' initializer [-Wmissing-field-initializers]
                                                                    ^
llvm-project/clang/lib/Sema/SemaModule.cpp:715:70: warning: missing field 'OuterVisibleModules' initializer [-Wmissing-field-initializers]
                                                                    ^

I had tried to fix this in a NFC commit: 4168efe1b. Hope this works.
(My compiler locally didn't warn for it... I need to update it...)