https://github.com/vsapsai updated https://github.com/llvm/llvm-project/pull/214345
>From 8c2b6d55cef2cd526e2ae6087e476a2af8f4504f Mon Sep 17 00:00:00 2001 From: Volodymyr Sapsai <[email protected]> Date: Tue, 4 Aug 2026 15:51:27 -0700 Subject: [PATCH 1/3] [modules] Avoid redefinition errors when access modular header as non-modular. Despite mechanisms to enforce a single inclusion (e.g., header guards, pragma once) treating the same header as modular and non-modular can defeat these mechanisms. Primarily it is caused by the fact that a module has a separate preprocessor state that's not shared with the includer. In practice it causes redefinition errors where both definitions are in the same file, at the same location. And it is very confusing and unactionable to clang users. In most cases both definitions are identical and the error doesn't provide any value. Don't reject such redefinitions immediately and re-use the same mechanism when we detect a definition with the same name as in a hidden submodule, i.e., check the definitions are identical and use one of them. rdar://183938264 --- clang/include/clang/Sema/Sema.h | 8 + clang/lib/Sema/SemaDecl.cpp | 10 +- clang/lib/Sema/SemaDeclObjC.cpp | 6 +- clang/lib/Sema/SemaModule.cpp | 22 +++ clang/lib/Sema/SemaTemplate.cpp | 6 +- .../test/Modules/reentered-header-duplicate.m | 180 ++++++++++++++++++ 6 files changed, 225 insertions(+), 7 deletions(-) create mode 100644 clang/test/Modules/reentered-header-duplicate.m diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 778c1a2f5c427..d9b2c7c280d2e 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -15708,6 +15708,14 @@ class Sema final : public SemaBase { return hasAcceptableDefinition(D, &Hidden, Kind); } + /// Determine if a definition at \p NewLoc coincides with \p PrevD. + /// + /// Including the same header as a non-modular and a modular allows to process + /// its content twice despite guards against multiple inclusions. To handle + /// such cases this method allows to detect if a currently-parsed decl is at + /// the same location as an existing decl imported from a module. + bool isFromSameSingleIncludeHeader(const Decl *PrevD, SourceLocation NewLoc); + /// Try to parse the conditional expression attached to an effect attribute /// (e.g. 'nonblocking'). (c.f. Sema::ActOnNoexceptSpec). Return an empty /// optional on error. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index ae8c26808a164..9424cd5cabf0a 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -5020,7 +5020,8 @@ void Sema::notePreviousDefinition(const NamedDecl *Old, SourceLocation New) { } bool Sema::checkVarDeclRedefinition(VarDecl *Old, VarDecl *New) { - if (!hasVisibleDefinition(Old) && + if ((!hasVisibleDefinition(Old) || + isFromSameSingleIncludeHeader(Old, New->getLocation())) && (New->getFormalLinkage() == Linkage::Internal || New->isInline() || isa<VarTemplateSpecializationDecl>(New) || New->getDescribedVarTemplate() || @@ -16246,7 +16247,9 @@ Sema::CheckForFunctionRedefinition(FunctionDecl *FD, return; bool DefinitionVisible = false; - if (SkipBody && isRedefinitionAllowedFor(Definition, DefinitionVisible) && + if (SkipBody && + (isRedefinitionAllowedFor(Definition, DefinitionVisible) || + isFromSameSingleIncludeHeader(Definition, FD->getLocation())) && (Definition->getFormalLinkage() == Linkage::Internal || Definition->isInlined() || Definition->getDescribedFunctionTemplate() || !Definition->getTemplateParameterLists().empty())) { @@ -18725,7 +18728,8 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc, bool HiddenDefVisible = false; if (SkipBody && (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || - getLangOpts().C23)) { + getLangOpts().C23 || + isFromSameSingleIncludeHeader(Def, NameLoc))) { // There is a definition of this tag, but it is not visible. // We explicitly make use of C++'s one definition rule here, // and assume that this definition is identical to the hidden diff --git a/clang/lib/Sema/SemaDeclObjC.cpp b/clang/lib/Sema/SemaDeclObjC.cpp index 04ba528098af4..4a6fc658575d9 100644 --- a/clang/lib/Sema/SemaDeclObjC.cpp +++ b/clang/lib/Sema/SemaDeclObjC.cpp @@ -1054,7 +1054,8 @@ ObjCInterfaceDecl *SemaObjC::ActOnStartClassInterface( if (PrevIDecl) { // Class already seen. Was it a definition? if (ObjCInterfaceDecl *Def = PrevIDecl->getDefinition()) { - if (SkipBody && !SemaRef.hasVisibleDefinition(Def)) { + if (SkipBody && (!SemaRef.hasVisibleDefinition(Def) || + SemaRef.isFromSameSingleIncludeHeader(Def, ClassLoc))) { SkipBody->CheckSameAsPrevious = true; SkipBody->New = IDecl; SkipBody->Previous = Def; @@ -1235,7 +1236,8 @@ ObjCProtocolDecl *SemaObjC::ActOnStartProtocolInterface( ProtocolLoc, AtProtoInterfaceLoc, /*PrevDecl=*/Def); - if (SkipBody && !SemaRef.hasVisibleDefinition(Def)) { + if (SkipBody && (!SemaRef.hasVisibleDefinition(Def) || + SemaRef.isFromSameSingleIncludeHeader(Def, ProtocolLoc))) { SkipBody->CheckSameAsPrevious = true; SkipBody->New = PDecl; SkipBody->Previous = Def; diff --git a/clang/lib/Sema/SemaModule.cpp b/clang/lib/Sema/SemaModule.cpp index caa61a99a6914..2705fee9c0168 100644 --- a/clang/lib/Sema/SemaModule.cpp +++ b/clang/lib/Sema/SemaModule.cpp @@ -1597,3 +1597,25 @@ void Sema::checkReferenceToTULocalFromOtherTU( PendingCheckReferenceForTULocal.push_back( std::make_pair(FD, PointOfInstantiation)); } + +bool Sema::isFromSameSingleIncludeHeader(const Decl *PrevD, + SourceLocation NewLoc) { + if (!PrevD->isFromASTFile()) + return false; + SourceLocation PrevLoc = PrevD->getLocation(); + if (!PrevLoc.isValid() || !NewLoc.isValid()) + return false; + SourceManager &SM = getSourceManager(); + auto [PrevFileID, PrevOffset] = SM.getDecomposedExpansionLoc(PrevLoc); + auto [NewFileID, NewOffset] = SM.getDecomposedExpansionLoc(NewLoc); + if (PrevOffset != NewOffset) + return false; + OptionalFileEntryRef PrevFileRef = SM.getFileEntryRefForID(PrevFileID), + NewFileRef = SM.getFileEntryRefForID(NewFileID); + if (*PrevFileRef != *NewFileRef) + return false; + const HeaderFileInfo *HFI = + getPreprocessor().getHeaderSearchInfo().getExistingFileInfo(*PrevFileRef); + return (HFI->isPragmaOnce || HFI->isImport || + HFI->LazyControllingMacro.isValid()); +} diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp index 643392833759d..9e0fdfd909faa 100644 --- a/clang/lib/Sema/SemaTemplate.cpp +++ b/clang/lib/Sema/SemaTemplate.cpp @@ -2169,7 +2169,8 @@ DeclResult Sema::CheckClassTemplate( NamedDecl *Hidden = nullptr; bool HiddenDefVisible = false; if (SkipBody && - isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible)) { + (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || + isFromSameSingleIncludeHeader(Def, NameLoc))) { SkipBody->ShouldSkip = true; SkipBody->Previous = Def; if (!HiddenDefVisible && Hidden) { @@ -9079,7 +9080,8 @@ DeclResult Sema::ActOnClassTemplateSpecialization( NamedDecl *Hidden = nullptr; bool HiddenDefVisible = false; if (Def && SkipBody && - isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible)) { + (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || + isFromSameSingleIncludeHeader(Def, TemplateNameLoc))) { SkipBody->ShouldSkip = true; SkipBody->Previous = Def; if (!HiddenDefVisible && Hidden) diff --git a/clang/test/Modules/reentered-header-duplicate.m b/clang/test/Modules/reentered-header-duplicate.m new file mode 100644 index 0000000000000..a349517c4bb64 --- /dev/null +++ b/clang/test/Modules/reentered-header-duplicate.m @@ -0,0 +1,180 @@ +// Check handling definitions from a file that is accessed both as non-modular and modular. + +// RUN: rm -rf %t +// RUN: split-file %s %t + +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-c -I %t/headers-c/sub %t/test.c -verify +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-c -I %t/headers-c/sub %t/test.c -verify \ +// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache + +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-objc -I %t/headers-objc/sub %t/test.m -verify +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-objc -I %t/headers-objc/sub %t/test.m -verify \ +// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache + +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-cxx -I %t/headers-cxx/sub %t/test.cpp -verify +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-cxx -I %t/headers-cxx/sub %t/test.cpp -verify \ +// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache + +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-unguarded -I %t/headers-unguarded/sub %t/unguarded.c -verify +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-unguarded -I %t/headers-unguarded/sub %t/unguarded.c -verify \ +// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache + +//--- headers-c/top.h +#ifndef TOP_H +#define TOP_H + +#include <sub/sub.h> + +#endif + +//--- headers-c/sub/sub.h +#ifndef SUB_H +#define SUB_H + +#include <top.h> +typedef int TestTypedef; + +struct TestStruct { + int a; + int b: 3; +}; + +union TestUnion { + int x; + float y; +}; + +struct WithAnonymous { + struct { + char p; + }; + struct { + int z; + } nested; +}; + +#define CUSTOM_STRUCT(name) struct name##Struct + +CUSTOM_STRUCT(MacroBased) { + int m; +}; + +enum Seasons { + kSeasonWinter = 0, + kSeasonSpring, +}; + +inline int square(int x) { + return x * x; +} + +#endif + +//--- headers-c/module.modulemap +module top_c { + header "top.h" + export * +} + +//--- test.c +// expected-no-diagnostics +// Access 'sub/sub.h' in non-modular way. +#include <sub.h> + + +//--- headers-objc/top.h +#import <sub/sub.h> + +//--- headers-objc/sub/sub.h +#import <top.h> + +@protocol TestProto +- (void)testProtocolMethod; +@end + +__attribute__((objc_root_class)) +@interface TestClass { + int _a; +} +- (void)testMethod:(float)b; +@end + +//--- headers-objc/module.modulemap +module top_objc { + header "top.h" + export * +} + +//--- test.m +// expected-no-diagnostics +// Access 'sub/sub.h' in non-modular way. +#import <sub.h> + + +//--- headers-cxx/top.h +#pragma once +#import <sub/sub.h> + +//--- headers-cxx/sub/sub.h +#pragma once +#include <top.h> + +inline int GlobalVar = 3; + +template <class T> class GenericClass { + T field; +}; + +template <bool B, class T, class F> struct condition { using type = F; }; +template <class T, class F> struct condition<true, T, F> { using type = T; }; + +template <typename T> void printGeneric(const T &val) { + // empty +} + +template <> void printGeneric<int>(const int &val) { + // still empty +} + +//--- headers-cxx/module.modulemap +module top_cxx { + header "top.h" + export * +} + +//--- test.cpp +// expected-no-diagnostics +// Access 'sub/sub.h' in non-modular way. +#import <sub.h> + + +//--- headers-unguarded/top.h +#ifndef TOP_H +#define TOP_H +#include <sub/sub.h> +#endif + +//--- headers-unguarded/sub/sub.h +#include <top.h> + +struct UnguardedStruct { + float x; + int y; +}; + +//--- headers-unguarded/module.modulemap +module top_unguarded { + header "top.h" + export * +} + +//--- unguarded.c +// Access 'sub/sub.h' in non-modular way. +#include <sub.h> +// [email protected]:* {{redefinition of 'UnguardedStruct'}} +// [email protected]:* {{sub.h' included multiple times, additional include site}} +// [email protected]:* {{sub.h' included multiple times, additional include site}} +// [email protected]:* {{unguarded header; consider using #ifdef guards or #pragma once}} +#if __has_feature(modules) +// [email protected]:* {{top_unguarded defined here}} +#endif >From 020057424d9788c05bf4a1b88c9fcace33e5de5c Mon Sep 17 00:00:00 2001 From: Volodymyr Sapsai <[email protected]> Date: Thu, 20 Aug 2026 13:02:00 -0700 Subject: [PATCH 2/3] `isFromSameSingleIncludeHeader` should be an alternative to `hasVisibleDefinition`, not to `isRedefinitionAllowedFor`. --- clang/include/clang/Sema/Sema.h | 9 +++++---- clang/lib/Sema/SemaDecl.cpp | 22 ++++++++++++---------- clang/lib/Sema/SemaTemplate.cpp | 7 +++---- 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index d9b2c7c280d2e..5d29f993315d7 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -15683,11 +15683,12 @@ class Sema final : public SemaBase { /// Determine if \p D has a definition which allows we redefine it in current /// TU. \p Suggested is the definition that should be made visible to expose /// the definition. - bool isRedefinitionAllowedFor(NamedDecl *D, NamedDecl **Suggested, - bool &Visible); - bool isRedefinitionAllowedFor(const NamedDecl *D, bool &Visible) { + bool isRedefinitionAllowedFor(NamedDecl *D, SourceLocation NewDefinitionLoc, + NamedDecl **Suggested, bool &Visible); + bool isRedefinitionAllowedFor(const NamedDecl *D, SourceLocation NewLoc, + bool &Visible) { NamedDecl *Hidden; - return isRedefinitionAllowedFor(const_cast<NamedDecl *>(D), &Hidden, + return isRedefinitionAllowedFor(const_cast<NamedDecl *>(D), NewLoc, &Hidden, Visible); } diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 9424cd5cabf0a..d70be1c63a87f 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -16248,8 +16248,8 @@ Sema::CheckForFunctionRedefinition(FunctionDecl *FD, bool DefinitionVisible = false; if (SkipBody && - (isRedefinitionAllowedFor(Definition, DefinitionVisible) || - isFromSameSingleIncludeHeader(Definition, FD->getLocation())) && + isRedefinitionAllowedFor(Definition, FD->getLocation(), + DefinitionVisible) && (Definition->getFormalLinkage() == Linkage::Internal || Definition->isInlined() || Definition->getDescribedFunctionTemplate() || !Definition->getTemplateParameterLists().empty())) { @@ -18726,10 +18726,9 @@ Sema::ActOnTag(Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc, // check in C11 6.2.7/1 (or 6.1.2.6/1 in C89). NamedDecl *Hidden = nullptr; bool HiddenDefVisible = false; - if (SkipBody && - (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || - getLangOpts().C23 || - isFromSameSingleIncludeHeader(Def, NameLoc))) { + if (SkipBody && (isRedefinitionAllowedFor(Def, NameLoc, &Hidden, + HiddenDefVisible) || + getLangOpts().C23)) { // There is a definition of this tag, but it is not visible. // We explicitly make use of C++'s one definition rule here, // and assume that this definition is identical to the hidden @@ -21428,8 +21427,9 @@ bool Sema::shouldIgnoreInHostDeviceCheck(FunctionDecl *Callee) { CUDA().IdentifyTarget(Callee) == CUDAFunctionTarget::Global; } -bool Sema::isRedefinitionAllowedFor(NamedDecl *D, NamedDecl **Suggested, - bool &Visible) { +bool Sema::isRedefinitionAllowedFor(NamedDecl *D, + SourceLocation NewDefinitionLoc, + NamedDecl **Suggested, bool &Visible) { Visible = hasVisibleDefinition(D, Suggested); // Accoding to [basic.def.odr]p16, it is not allowed to have duplicated definition // for declaratins which is attached to named modules. @@ -21439,6 +21439,8 @@ bool Sema::isRedefinitionAllowedFor(NamedDecl *D, NamedDecl **Suggested, D->isInNamedModule()) return false; // The redefinition of D in the **current** TU is allowed if D is invisible or - // D is defined in the global module of other module units. - return D->isInAnotherModuleUnit() || !Visible; + // D is defined in the global module of other module units or D is defined in + // the same header in a different module. + return D->isInAnotherModuleUnit() || !Visible || + isFromSameSingleIncludeHeader(D, NewDefinitionLoc); } diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp index 9e0fdfd909faa..5f4bb592b6c7b 100644 --- a/clang/lib/Sema/SemaTemplate.cpp +++ b/clang/lib/Sema/SemaTemplate.cpp @@ -2169,8 +2169,7 @@ DeclResult Sema::CheckClassTemplate( NamedDecl *Hidden = nullptr; bool HiddenDefVisible = false; if (SkipBody && - (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || - isFromSameSingleIncludeHeader(Def, NameLoc))) { + isRedefinitionAllowedFor(Def, NameLoc, &Hidden, HiddenDefVisible)) { SkipBody->ShouldSkip = true; SkipBody->Previous = Def; if (!HiddenDefVisible && Hidden) { @@ -9080,8 +9079,8 @@ DeclResult Sema::ActOnClassTemplateSpecialization( NamedDecl *Hidden = nullptr; bool HiddenDefVisible = false; if (Def && SkipBody && - (isRedefinitionAllowedFor(Def, &Hidden, HiddenDefVisible) || - isFromSameSingleIncludeHeader(Def, TemplateNameLoc))) { + isRedefinitionAllowedFor(Def, TemplateNameLoc, &Hidden, + HiddenDefVisible)) { SkipBody->ShouldSkip = true; SkipBody->Previous = Def; if (!HiddenDefVisible && Hidden) >From 527df23ee0974d93fdfcddb97c1ab8670cb8a32d Mon Sep 17 00:00:00 2001 From: Volodymyr Sapsai <[email protected]> Date: Mon, 31 Aug 2026 21:47:23 -0700 Subject: [PATCH 3/3] Add a test showing SourceLocation equality isn't sufficient for decl equality. We are still comparing decls properly despite the SourceLocation equality. --- .../test/Modules/reentered-header-duplicate.m | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/clang/test/Modules/reentered-header-duplicate.m b/clang/test/Modules/reentered-header-duplicate.m index a349517c4bb64..bb56eed8f2dd6 100644 --- a/clang/test/Modules/reentered-header-duplicate.m +++ b/clang/test/Modules/reentered-header-duplicate.m @@ -19,6 +19,9 @@ // RUN: %clang_cc1 -fsyntax-only -I %t/headers-unguarded -I %t/headers-unguarded/sub %t/unguarded.c -verify \ // RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache +// RUN: %clang_cc1 -fsyntax-only -I %t/headers-mismatched -I %t/headers-mismatched/sub %t/mismatched.c -verify \ +// RUN: -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache + //--- headers-c/top.h #ifndef TOP_H #define TOP_H @@ -178,3 +181,38 @@ - (void)testMethod:(float)b; #if __has_feature(modules) // [email protected]:* {{top_unguarded defined here}} #endif + + +//--- headers-mismatched/top.h +#ifndef TOP_H +#define TOP_H +#include <sub/sub.h> +#endif + +//--- headers-mismatched/sub/sub.h +#ifndef SUB_H +#define SUB_H + +#include <top.h> + +struct MismatchedStruct { + int x; +#ifdef EXTRA_FIELD + char z; +#endif +}; + +#endif + +//--- headers-mismatched/module.modulemap +module top_mismatched { + header "top.h" + export * +} + +//--- mismatched.c +#define EXTRA_FIELD 1 +#include <sub.h> +// [email protected]:* {{type 'struct MismatchedStruct' has incompatible definitions}} +// [email protected]:* {{field 'z' has type 'char' here}} +// [email protected]:* {{no corresponding field here}} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
