[C++20][Modules] Do not allow non-inline external definitions in header units.

[iains]

Summary

[module.import/6] last sentence:
A header unit shall not contain a definition of a non-inline function or
variable whose name has external linkage.

[iains]

this came up during discussion of other header unit constraints, we had not implemented it yet.
found a test case where we'd accidentally broken this rule too.

[ChuanqiXu]

LGTM generally.

clang/lib/Sema/SemaDecl.cpp

13076–13077

getLangOpts().CPlusPlus20 is redundant. It is also good to define a helper interface withinHeaderUnit in Sema (not required).

15238–15239

ditto

[iains]

clang/lib/Sema/SemaDecl.cpp

13076–13077

I do not mind making this change - but note that the constraint is specific to C++20 and there are some people who want to remove it (the constraint). I guess you meant getCurrentModule() ?

[ChuanqiXu]

clang/lib/Sema/SemaDecl.cpp

13076–13077

I do not mind making this change - but note that the constraint is specific to C++20 and there are some people who want to remove it (the constraint).

Got it. Let's make it when it comes true. It won't be a big deal.

I guess you meant getCurrentModule() ?

It looks a little bit better to me to not access ModuleScopes directly.

[iains]

rebased, addressed comments.

[iains]

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

clang/lib/Sema/SemaDecl.cpp

13076–13077

revised now (I added the helper).

[ChuanqiXu]

In D140261#4004542, @iains wrote:

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

[iains]

In D140261#4006653, @ChuanqiXu wrote:

In D140261#4004542, @iains wrote:

OK so this is what I plan to land assuming testing goes OK.
I suspect that this might cause some user code to flag errors - there are quite a number of ODR violations "in the wild".

I forgot we need to mention such changes in https://clang.llvm.org/docs/ReleaseNotes.html#potentially-breaking-changes.

It will not break any existing compilation (we accept headers with non-inline external definitions as textual includes, that has not changed). What I mean is that such headers will not be accepted as Header Units - and I am sure some users will consider that to be a bug (even though the problem is that the header is not ODR safe).
So it's not a change that breaks existing code - because no-one has Header Units in existing code.

[ChuanqiXu]

So it's not a change that breaks existing code - because no-one has Header Units in existing code.

We can't assume this. Since we already said people can try to use header units in clang15. We can't assume there is nobody using header units just because they don't tell us. In fact, I've seen some bug reports about header units recently. So there are people (try to) using header units at least. So I think we should better to mark it in the potential breaking section.

[iains]

rebase, added release notes

[iains]

how does that look?

[ChuanqiXu]

Yeah, it looks good to me.

clang/docs/ReleaseNotes.rst

- Clang's resource dir used to include the full clang version. It will now
   ``clang -print-resource-dir``, just like before.
 
 - To match GCC, ``__ppc64__`` is no longer defined on PowerPC64 targets. Use
   ``__powerpc64__`` instead.
 
 - ``-p`` is rejected for all targets which are not AIX or OpenBSD. ``-p`` led
   to an ``-Wunused-command-line-argument`` warning in previous releases.
 
+- Clang now diagnoses non-inline externally-visible definitions in C++
+  standard header units as per ``[module.import/6]``.  Previously, in Clang-15,
+  these definitions were allowed.  Note that such definitions are ODR
+  violations if the header is included more than once.
+
 What's New in Clang |release|?
 ==============================
 Some of the major new features and improvements to Clang are listed
 here. Generic improvements to Clang as a whole or to its underlying
 infrastructure are described first, followed by language-specific
 sections with improvements to Clang's support for those languages.
 
 Major New Features

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_private_module_fragment_redefined : Error<
   "private module fragment redefined">;
 def err_private_module_fragment_not_module_interface : Error<
   "private module fragment in module implementation unit">;
 def note_not_module_interface_add_export : Note<
   "add 'export' here if this is intended to be a module interface unit">;
 def err_invalid_module_name : Error<
   "%0 is %select{an invalid|a reserved}1 name for a module">;
+def err_extern_def_in_header_unit : Error<
+  "non-inline external definitions are not permitted in C++ header units">;
 
 def ext_equivalent_internal_linkage_decl_in_modules : ExtWarn<
   "ambiguous use of internal linkage declaration %0 defined in multiple modules">,
   InGroup<DiagGroup<"modules-ambiguous-internal-linkage">>;
 def note_equivalent_internal_linkage_decl : Note<
   "declared here%select{ in module '%1'|}0">;
 
 def note_redefinition_modules_same_file : Note<

clang/include/clang/Sema/Sema.h

Module *getCurrentModule() const {
     return ModuleScopes.empty() ? nullptr : ModuleScopes.back().Module;
   }
 
   /// Is the module scope we are an interface?
   bool currentModuleIsInterface() const {
     return ModuleScopes.empty() ? false : ModuleScopes.back().ModuleInterface;
   }
 
+  /// Is the module scope we are in a C++ Header Unit?
+  bool currentModuleIsHeaderUnit() const {
+    return ModuleScopes.empty() ? false
+                                : ModuleScopes.back().Module->isHeaderUnit();
+  }
+
   /// Get the module owning an entity.
   Module *getOwningModule(const Decl *Entity) {
     return Entity->getOwningModule();
   }
 
   bool isModuleDirectlyImported(const Module *M) {
     return DirectModuleImports.contains(M);
   }

clang/lib/Sema/SemaDecl.cpp

if (!VDecl->getType()->isDependentType()) {
 
     // The variable can not have an abstract class type.
     if (RequireNonAbstractType(VDecl->getLocation(), VDecl->getType(),
                                diag::err_abstract_type_in_decl,
                                AbstractVariableType))
       VDecl->setInvalidDecl();
   }
 
+  // C++ [module.import/6] external definitions are not permitted in header
+  // units.
+  if (getLangOpts().CPlusPlusModules && currentModuleIsHeaderUnit() &&
+      VDecl->getFormalLinkage() == Linkage::ExternalLinkage &&
+      !VDecl->isInline()) {
+    Diag(VDecl->getLocation(), diag::err_extern_def_in_header_unit);
+    VDecl->setInvalidDecl();
+  }
+
   // If adding the initializer will turn this declaration into a definition,
   // and we already have a definition for this variable, diagnose or otherwise
   // handle the situation.
   if (VarDecl *Def = VDecl->getDefinition())
     if (Def != VDecl &&
         (!VDecl->isStaticDataMember() || VDecl->isOutOfLine()) &&
         !VDecl->isThisDeclarationADemotedDefinition() &&
         checkVarDeclRedefinition(Def, VDecl))
for (auto *Param : FD->parameters()) {
     // If this has an identifier, add it to the scope stack.
     if (Param->getIdentifier() && FnBodyScope) {
       CheckShadow(FnBodyScope, Param);
 
       PushOnScopeChains(Param, FnBodyScope);
     }
   }
 
+  // C++ [module.import/6] external definitions are not permitted in header
+  // units.
+  if (getLangOpts().CPlusPlusModules && currentModuleIsHeaderUnit() &&
+      FD->getFormalLinkage() == Linkage::ExternalLinkage && !FD->isInlined()) {
+    Diag(FD->getLocation(), diag::err_extern_def_in_header_unit);
+    FD->setInvalidDecl();
+  }
+
   // Ensure that the function's exception specification is instantiated.
   if (const FunctionProtoType *FPT = FD->getType()->getAs<FunctionProtoType>())
     ResolveExceptionSpec(D->getLocation(), FPT);
 
   // dllimport cannot be applied to non-inline function definitions.
   if (FD->hasAttr<DLLImportAttr>() && !FD->isInlined() &&
       !FD->isTemplateInstantiation()) {
     assert(!FD->hasAttr<DLLExportAttr>());

clang/test/CXX/module/module.import/p6.cpp

+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -std=c++20 -x c++-header %t/bad-header-unit.h \
+// RUN:  -emit-header-unit -o %t/bad-header-unit.pcm -verify
+
+//--- bad-header-unit.h
+
+inline int ok_foo () { return 0;}
+
+static int ok_bar ();
+
+int ok_decl ();
+
+int bad_def () { return 2;}  // expected-error {{non-inline external definitions are not permitted in C++ header units}}
+
+inline int ok_inline_var = 1;
+
+static int ok_static_var;
+
+int ok_var_decl;
+
+int bad_var_definition = 3;  // expected-error {{non-inline external definitions are not permitted in C++ header units}}
+

clang/test/CodeGenCXX/module-initializer-header.cppm

 // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
 // RUN: rm -rf %t
 // RUN: split-file %s %t
 //
 // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple -xc++-user-header -emit-header-unit %t/header.h -o %t/header.pcm
 // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple -fmodule-file=%t/header.pcm %t/M.cppm -S -emit-llvm -o - | FileCheck %t/M.cppm
 // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple -fmodule-file=%t/header.pcm %t/Use.cpp -S -emit-llvm -o - | FileCheck %t/Use.cpp
 //
 //--- header.h
 int foo();
-int i = foo();
+static int i = foo();
 
 //--- M.cppm
 module;
 import "header.h";
 export module M;
 
-// CHECK: @i = {{.*}}global i32 0
+// CHECK: @_ZL1i = {{.*}}global i32 0
 // CHECK: void @__cxx_global_var_init()
 // CHECK-NEXT: entry:
 // CHECK-NEXT:  %call = call noundef{{.*}} i32 @_Z3foov()
-// CHECK-NEXT:  store i32 %call, ptr @i  
+// CHECK-NEXT:  store i32 %call, ptr @_ZL1i
 
 //--- Use.cpp
 import "header.h";
 
-// CHECK: @i = {{.*}}global i32 0
+// CHECK: @_ZL1i = {{.*}}global i32 0
 // CHECK: void @__cxx_global_var_init()
 // CHECK-NEXT: entry:
 // CHECK-NEXT:  %call = call noundef{{.*}} i32 @_Z3foov()
-// CHECK-NEXT:  store i32 %call, ptr @i  
+// CHECK-NEXT:  store i32 %call, ptr @_ZL1i