From 074f6fd61d382ff6bf108472ea701d214b02f64b Mon Sep 17 00:00:00 2001 From: Mitch Phillips <31459023+hctim@users.noreply.github.com> Date: Mon, 27 Mar 2023 04:33:53 -0700 Subject: [PATCH] Revert "[C++20][Modules] Introduce an implementation module." This reverts commit c6e9823724ef6bdfee262289ee34d162db436af0. Reason: Broke the ASan buildbots, see https://reviews.llvm.org/D126959 (the original phabricator review) for more info. --- clang/include/clang/Basic/Module.h | 28 ++--------- clang/include/clang/Lex/ModuleMap.h | 12 ----- clang/include/clang/Sema/Sema.h | 4 -- clang/lib/AST/Decl.cpp | 1 - clang/lib/CodeGen/CGDeclCXX.cpp | 6 +-- clang/lib/CodeGen/CodeGenModule.cpp | 2 - clang/lib/Frontend/FrontendActions.cpp | 2 - clang/lib/Lex/ModuleMap.cpp | 42 +++------------- clang/lib/Sema/SemaDecl.cpp | 20 +++----- clang/lib/Sema/SemaModule.cpp | 59 +++++++++-------------- clang/lib/Serialization/ASTWriter.cpp | 2 +- clang/test/CXX/module/basic/basic.def.odr/p4.cppm | 10 ++-- clang/test/CXX/module/basic/basic.link/p2.cppm | 10 ++-- clang/test/CodeGenCXX/module-intializer.cpp | 8 +-- 14 files changed, 59 insertions(+), 147 deletions(-) diff --git a/clang/include/clang/Basic/Module.h b/clang/include/clang/Basic/Module.h index c0c99eb..387ce4d 100644 --- a/clang/include/clang/Basic/Module.h +++ b/clang/include/clang/Basic/Module.h @@ -103,22 +103,16 @@ public: /// The location of the module definition. SourceLocation DefinitionLoc; - // FIXME: Consider if reducing the size of this enum (having Partition and - // Named modules only) then representing interface/implementation separately - // is more efficient. enum ModuleKind { /// This is a module that was defined by a module map and built out /// of header files. ModuleMapModule, - /// This is a C++ 20 header unit. - ModuleHeaderUnit, - /// This is a C++20 module interface unit. ModuleInterfaceUnit, - /// This is a C++20 module implementation unit. - ModuleImplementationUnit, + /// This is a C++ 20 header unit. + ModuleHeaderUnit, /// This is a C++ 20 module partition interface. ModulePartitionInterface, @@ -175,16 +169,9 @@ public: /// Does this Module scope describe part of the purview of a standard named /// C++ module? bool isModulePurview() const { - switch (Kind) { - case ModuleInterfaceUnit: - case ModuleImplementationUnit: - case ModulePartitionInterface: - case ModulePartitionImplementation: - case PrivateModuleFragment: - return true; - default: - return false; - } + return Kind == ModuleInterfaceUnit || Kind == ModulePartitionInterface || + Kind == ModulePartitionImplementation || + Kind == PrivateModuleFragment; } /// Does this Module scope describe a fragment of the global module within @@ -574,11 +561,6 @@ public: Kind == ModulePartitionImplementation; } - /// Is this a module implementation. - bool isModuleImplementation() const { - return Kind == ModuleImplementationUnit; - } - /// Is this module a header unit. bool isHeaderUnit() const { return Kind == ModuleHeaderUnit; } // Is this a C++20 module interface or a partition. diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h index f155c60..a0ddd13 100644 --- a/clang/include/clang/Lex/ModuleMap.h +++ b/clang/include/clang/Lex/ModuleMap.h @@ -560,11 +560,6 @@ public: Module *createPrivateModuleFragmentForInterfaceUnit(Module *Parent, SourceLocation Loc); - /// Create a new C++ module with the specified kind, and reparent any pending - /// global module fragment(s) to it. - Module *createModuleUnitWithKind(SourceLocation Loc, StringRef Name, - Module::ModuleKind Kind); - /// Create a new module for a C++ module interface unit. /// The module must not already exist, and will be configured for the current /// compilation. @@ -574,13 +569,6 @@ public: /// \returns The newly-created module. Module *createModuleForInterfaceUnit(SourceLocation Loc, StringRef Name); - /// Create a new module for a C++ module implementation unit. - /// The interface module for this implementation (implicitly imported) must - /// exist and be loaded and present in the modules map. - /// - /// \returns The newly-created module. - Module *createModuleForImplementationUnit(SourceLocation Loc, StringRef Name); - /// Create a C++20 header unit. Module *createHeaderUnit(SourceLocation Loc, StringRef Name, Module::Header H); diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 9f7d58a..92ca377 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -2274,10 +2274,6 @@ private: }; /// The modules we're currently parsing. llvm::SmallVector ModuleScopes; - - /// For an interface unit, this is the implicitly imported interface unit. - clang::Module *ThePrimaryInterface = nullptr; - /// The explicit global module fragment of the current translation unit. /// The explicit Global Module Fragment, as specified in C++ /// [module.global.frag]. diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp index cd78604..56042e5 100644 --- a/clang/lib/AST/Decl.cpp +++ b/clang/lib/AST/Decl.cpp @@ -1600,7 +1600,6 @@ Module *Decl::getOwningModuleForLinkage(bool IgnoreLinkage) const { return nullptr; case Module::ModuleInterfaceUnit: - case Module::ModuleImplementationUnit: case Module::ModulePartitionInterface: case Module::ModulePartitionImplementation: return M; diff --git a/clang/lib/CodeGen/CGDeclCXX.cpp b/clang/lib/CodeGen/CGDeclCXX.cpp index 9d7284c..0d0b570 100644 --- a/clang/lib/CodeGen/CGDeclCXX.cpp +++ b/clang/lib/CodeGen/CGDeclCXX.cpp @@ -880,11 +880,9 @@ CodeGenModule::EmitCXXGlobalInitFunc() { // Include the filename in the symbol name. Including "sub_" matches gcc // and makes sure these symbols appear lexicographically behind the symbols - // with priority emitted above. Module implementation units behave the same - // way as a non-modular TU with imports. + // with priority emitted above. llvm::Function *Fn; - if (CXX20ModuleInits && getContext().getNamedModuleForCodeGen() && - !getContext().getNamedModuleForCodeGen()->isModuleImplementation()) { + if (CXX20ModuleInits && getContext().getNamedModuleForCodeGen()) { SmallString<256> InitFnName; llvm::raw_svector_ostream Out(InitFnName); cast(getCXXABI().getMangleContext()) diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp index bd1ee2a..0e33e96 100644 --- a/clang/lib/CodeGen/CodeGenModule.cpp +++ b/clang/lib/CodeGen/CodeGenModule.cpp @@ -548,8 +548,6 @@ void CodeGenModule::Release() { GlobalTopLevelStmtBlockInFlight = {nullptr, nullptr}; } - // Module implementations are initialized the same way as a regular TU that - // imports one or more modules. if (CXX20ModuleInits && Primary && Primary->isInterfaceOrPartition()) EmitCXXModuleInitFunc(Primary); else diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp index 05d9fc8..2aae41f 100644 --- a/clang/lib/Frontend/FrontendActions.cpp +++ b/clang/lib/Frontend/FrontendActions.cpp @@ -759,8 +759,6 @@ static StringRef ModuleKindName(Module::ModuleKind MK) { return "Module Map Module"; case Module::ModuleInterfaceUnit: return "Interface Unit"; - case Module::ModuleImplementationUnit: - return "Implementation Unit"; case Module::ModulePartitionInterface: return "Partition Interface"; case Module::ModulePartitionImplementation: diff --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp index f2b2d0b..8dead93 100644 --- a/clang/lib/Lex/ModuleMap.cpp +++ b/clang/lib/Lex/ModuleMap.cpp @@ -888,30 +888,23 @@ ModuleMap::createPrivateModuleFragmentForInterfaceUnit(Module *Parent, return Result; } -Module *ModuleMap::createModuleUnitWithKind(SourceLocation Loc, StringRef Name, - Module::ModuleKind Kind) { +Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc, + StringRef Name) { + assert(LangOpts.CurrentModule == Name && "module name mismatch"); + assert(!Modules[Name] && "redefining existing module"); + auto *Result = new Module(Name, Loc, nullptr, /*IsFramework*/ false, /*IsExplicit*/ false, NumCreatedModules++); - Result->Kind = Kind; + Result->Kind = Module::ModuleInterfaceUnit; + Modules[Name] = SourceModule = Result; - // Reparent any current global module fragment as a submodule of this module. + // Reparent the current global module fragment as a submodule of this module. for (auto &Submodule : PendingSubmodules) { Submodule->setParent(Result); Submodule.release(); // now owned by parent } PendingSubmodules.clear(); - return Result; -} - -Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc, - StringRef Name) { - assert(LangOpts.CurrentModule == Name && "module name mismatch"); - assert(!Modules[Name] && "redefining existing module"); - - auto *Result = - createModuleUnitWithKind(Loc, Name, Module::ModuleInterfaceUnit); - Modules[Name] = SourceModule = Result; // Mark the main source file as being within the newly-created module so that // declarations and macros are properly visibility-restricted to it. @@ -922,25 +915,6 @@ Module *ModuleMap::createModuleForInterfaceUnit(SourceLocation Loc, return Result; } -Module *ModuleMap::createModuleForImplementationUnit(SourceLocation Loc, - StringRef Name) { - assert(LangOpts.CurrentModule == Name && "module name mismatch"); - // The interface for this implementation must exist and be loaded. - assert(Modules[Name] && Modules[Name]->Kind == Module::ModuleInterfaceUnit && - "creating implementation module without an interface"); - - auto *Result = - createModuleUnitWithKind(Loc, Name, Module::ModuleImplementationUnit); - SourceModule = Result; - - // Mark the main source file as being within the newly-created module so that - // declarations and macros are properly visibility-restricted to it. - auto *MainFile = SourceMgr.getFileEntryForID(SourceMgr.getMainFileID()); - assert(MainFile && "no input file for module implementation"); - - return Result; -} - Module *ModuleMap::createHeaderUnit(SourceLocation Loc, StringRef Name, Module::Header H) { assert(LangOpts.CurrentModule == Name && "module name mismatch"); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index dd001db..6403439 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -1661,19 +1661,13 @@ bool Sema::CheckRedeclarationModuleOwnership(NamedDecl *New, NamedDecl *Old) { if (NewM == OldM) return false; - if (NewM && OldM) { - // A module implementation unit has visibility of the decls in its - // implicitly imported interface. - if (NewM->isModuleImplementation() && OldM == ThePrimaryInterface) - return false; - - // Partitions are part of the module, but a partition could import another - // module, so verify that the PMIs agree. - if ((NewM->isModulePartition() || OldM->isModulePartition()) && - NewM->getPrimaryModuleInterfaceName() == - OldM->getPrimaryModuleInterfaceName()) - return false; - } + // Partitions are part of the module, but a partition could import another + // module, so verify that the PMIs agree. + if (NewM && OldM && + (NewM->isModulePartition() || OldM->isModulePartition()) && + NewM->getPrimaryModuleInterfaceName() == + OldM->getPrimaryModuleInterfaceName()) + return false; bool NewIsModuleInterface = NewM && NewM->isModulePurview(); bool OldIsModuleInterface = OldM && OldM->isModulePurview(); diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp index c02b9d2..8c120d2 100644 --- a/clang/lib/Sema/SemaModule.cpp +++ b/clang/lib/Sema/SemaModule.cpp @@ -298,8 +298,8 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc, const_cast(getLangOpts()).CurrentModule = ModuleName; auto &Map = PP.getHeaderSearchInfo().getModuleMap(); - Module *Mod; // The module we are creating. - Module *Interface = nullptr; // The interface for an implementation. + Module *Mod; + switch (MDK) { case ModuleDeclKind::Interface: case ModuleDeclKind::PartitionInterface: { @@ -336,19 +336,18 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc, // we're building if `LangOpts.CurrentModule` equals to 'ModuleName'. // Change the value for `LangOpts.CurrentModule` temporarily to make the // module loader work properly. - const_cast(getLangOpts()).CurrentModule = ""; - Interface = getModuleLoader().loadModule(ModuleLoc, {ModuleNameLoc}, - Module::AllVisible, - /*IsInclusionDirective=*/false); + const_cast(getLangOpts()).CurrentModule = ""; + Mod = getModuleLoader().loadModule(ModuleLoc, {ModuleNameLoc}, + Module::AllVisible, + /*IsInclusionDirective=*/false); const_cast(getLangOpts()).CurrentModule = ModuleName; - if (!Interface) { + if (!Mod) { Diag(ModuleLoc, diag::err_module_not_defined) << ModuleName; // Create an empty module interface unit for error recovery. Mod = Map.createModuleForInterfaceUnit(ModuleLoc, ModuleName); - } else { - Mod = Map.createModuleForImplementationUnit(ModuleLoc, ModuleName); } + } break; case ModuleDeclKind::PartitionImplementation: @@ -387,31 +386,19 @@ Sema::ActOnModuleDecl(SourceLocation StartLoc, SourceLocation ModuleLoc, // statements, so imports are allowed. ImportState = ModuleImportState::ImportAllowed; - getASTContext().setNamedModuleForCodeGen(Mod); - - // We already potentially made an implicit import (in the case of a module - // implementation unit importing its interface). Make this module visible - // and return the import decl to be added to the current TU. - if (Interface) { - - VisibleModules.setVisible(Interface, ModuleLoc); - - // Make the import decl for the interface in the impl module. - ImportDecl *Import = ImportDecl::Create(Context, CurContext, ModuleLoc, - Interface, Path[0].second); - CurContext->addDecl(Import); - - // Sequence initialization of the imported module before that of the current - // module, if any. - Context.addModuleInitializer(ModuleScopes.back().Module, Import); - Mod->Imports.insert(Interface); // As if we imported it. - // Also save this as a shortcut to checking for decls in the interface - ThePrimaryInterface = Interface; - // If we made an implicit import of the module interface, then return the - // imported module decl. + // For an implementation, We already made an implicit import (its interface). + // Make and return the import decl to be added to the current TU. + if (MDK == ModuleDeclKind::Implementation) { + // Make the import decl for the interface. + ImportDecl *Import = + ImportDecl::Create(Context, CurContext, ModuleLoc, Mod, Path[0].second); + // and return it to be added. return ConvertDeclToDeclGroup(Import); } + getASTContext().setNamedModuleForCodeGen(Mod); + + // FIXME: Create a ModuleDecl. return nullptr; } @@ -437,17 +424,19 @@ Sema::ActOnPrivateModuleFragmentDecl(SourceLocation ModuleLoc, Diag(ModuleScopes.back().BeginLoc, diag::note_previous_definition); return nullptr; - case Module::ModuleImplementationUnit: + case Module::ModuleInterfaceUnit: + break; + } + + if (!ModuleScopes.back().ModuleInterface) { Diag(PrivateLoc, diag::err_private_module_fragment_not_module_interface); Diag(ModuleScopes.back().BeginLoc, diag::note_not_module_interface_add_export) << FixItHint::CreateInsertion(ModuleScopes.back().BeginLoc, "export "); return nullptr; - - case Module::ModuleInterfaceUnit: - break; } + // FIXME: Check this isn't a module interface partition. // FIXME: Check that this translation unit does not import any partitions; // such imports would violate [basic.link]/2's "shall be the only module unit" // restriction. diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index d89ee4e..f5691e9 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -2719,7 +2719,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) { Abbrev->Add(BitCodeAbbrevOp(SUBMODULE_DEFINITION)); Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // ID Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Parent - Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 4)); // Kind + Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // Kind Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsFramework Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsExplicit Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // IsSystem diff --git a/clang/test/CXX/module/basic/basic.def.odr/p4.cppm b/clang/test/CXX/module/basic/basic.def.odr/p4.cppm index 487dbde..1542e53 100644 --- a/clang/test/CXX/module/basic/basic.def.odr/p4.cppm +++ b/clang/test/CXX/module/basic/basic.def.odr/p4.cppm @@ -143,6 +143,9 @@ void use() { (void)&inline_var_exported; (void)&const_var_exported; + // CHECK: define {{.*}}@_ZL26used_static_module_linkagev + used_static_module_linkage(); + // CHECK: define linkonce_odr {{.*}}@_ZW6Module26used_inline_module_linkagev used_inline_module_linkage(); @@ -151,12 +154,8 @@ void use() { (void)&extern_var_module_linkage; (void)&inline_var_module_linkage; - - // FIXME: Issue #61427 Internal-linkage declarations in the interface TU - // should not be not visible here. (void)&static_var_module_linkage; // FIXME: Should not be visible here. - - (void)&const_var_module_linkage; // FIXME: will be visible after P2788R0 + (void)&const_var_module_linkage; } //--- user.cpp @@ -177,6 +176,5 @@ void use() { (void)&inline_var_exported; (void)&const_var_exported; - // Internal-linkage declarations are not visible here. // Module-linkage declarations are not visible here. } diff --git a/clang/test/CXX/module/basic/basic.link/p2.cppm b/clang/test/CXX/module/basic/basic.link/p2.cppm index 19761fb..e04412e 100644 --- a/clang/test/CXX/module/basic/basic.link/p2.cppm +++ b/clang/test/CXX/module/basic/basic.link/p2.cppm @@ -39,21 +39,19 @@ void use() { } //--- M.cpp - +// expected-no-diagnostics module M; +// FIXME: Use of internal linkage entities should be rejected. void use_from_module_impl() { external_linkage_fn(); module_linkage_fn(); - internal_linkage_fn(); // expected-error {{no matching function for call to 'internal_linkage_fn'}} + internal_linkage_fn(); (void)external_linkage_class{}; (void)module_linkage_class{}; + (void)internal_linkage_class{}; (void)external_linkage_var; (void)module_linkage_var; - - // FIXME: Issue #61427 Internal-linkage declarations in the interface TU - // should not be not visible here. - (void)internal_linkage_class{}; (void)internal_linkage_var; } diff --git a/clang/test/CodeGenCXX/module-intializer.cpp b/clang/test/CodeGenCXX/module-intializer.cpp index d365d18..e514940 100644 --- a/clang/test/CodeGenCXX/module-intializer.cpp +++ b/clang/test/CodeGenCXX/module-intializer.cpp @@ -18,17 +18,17 @@ // RUN: -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-P // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.cpp \ -// RUN: -fmodule-file=N=N.pcm -fmodule-file=O=O.pcm -fmodule-file=M:Part=M-part.pcm \ +// RUN: -fmodule-file=N.pcm -fmodule-file=O.pcm -fmodule-file=M-part.pcm \ // RUN: -emit-module-interface -o M.pcm // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.pcm -S -emit-llvm \ // RUN: -o - | FileCheck %s --check-prefix=CHECK-M // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 useM.cpp \ -// RUN: -fmodule-file=M=M.pcm -S -emit-llvm -o - \ +// RUN: -fmodule-file=M.pcm -S -emit-llvm -o - \ // RUN: | FileCheck %s --check-prefix=CHECK-USE // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M-impl.cpp \ -// RUN: -fmodule-file=M=M.pcm -S -emit-llvm -o - \ +// RUN: -fmodule-file=M.pcm -S -emit-llvm -o - \ // RUN: | FileCheck %s --check-prefix=CHECK-IMPL // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 N.cpp -S -emit-llvm \ @@ -41,7 +41,7 @@ // RUN: -o - | FileCheck %s --check-prefix=CHECK-P // RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 M.cpp \ -// RUN: -fmodule-file=N.pcm -fmodule-file=O=O.pcm -fmodule-file=M:Part=M-part.pcm \ +// RUN: -fmodule-file=N.pcm -fmodule-file=O.pcm -fmodule-file=M-part.pcm \ // RUN: -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-M //--- N-h.h -- 2.7.4