[Modules] Detect ODR mismatches for enums in non-C++ like in C++. #90298
Conversation
There is no reason for C and Objective-C to differ from C++ in this matter. rdar://85531830
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules Author: Volodymyr Sapsai (vsapsai) ChangesThere is no reason for C and Objective-C to differ from C++ in this matter. rdar://85531830 Full diff: https://github.com/llvm/llvm-project/pull/90298.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 744f11de88c2f8..6afbbfe20f26b7 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -805,9 +805,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
// If this is a definition subject to the ODR, and we already have a
// definition, merge this one into it.
- if (ED->isCompleteDefinition() &&
- Reader.getContext().getLangOpts().Modules &&
- Reader.getContext().getLangOpts().CPlusPlus) {
+ if (ED->isCompleteDefinition() && Reader.getContext().getLangOpts().Modules) {
EnumDecl *&OldDef = Reader.EnumDefinitions[ED->getCanonicalDecl()];
if (!OldDef) {
// This is the first time we've seen an imported definition. Look for a
@@ -3304,8 +3302,7 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
return RD->getDefinition();
if (auto *ED = dyn_cast<EnumDecl>(DC))
- return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
- : nullptr;
+ return ED->getDefinition();
if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
return OID->getDefinition();
diff --git a/clang/test/Modules/odr_hash-enum.c b/clang/test/Modules/odr_hash-enum.c
new file mode 100644
index 00000000000000..f8ede923fe2caa
--- /dev/null
+++ b/clang/test/Modules/odr_hash-enum.c
@@ -0,0 +1,75 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s >> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.modulemap
+// RUN: echo " header \"first.h\"" >> %t/Inputs/module.modulemap
+// RUN: echo "}" >> %t/Inputs/module.modulemap
+// RUN: echo "module SecondModule {" >> %t/Inputs/module.modulemap
+// RUN: echo " header \"second.h\"" >> %t/Inputs/module.modulemap
+// RUN: echo "}" >> %t/Inputs/module.modulemap
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+enum DifferentEnumConstants { kDifferentEnumConstantsValueFirst };
+#elif defined(SECOND)
+enum DifferentEnumConstants { kDifferentEnumConstantsValueSecond };
+#else
+enum DifferentEnumConstants differentEnumConstants;
+// [email protected]:* {{'kDifferentEnumConstantsValueSecond' from module 'SecondModule' is not present in definition of 'enum DifferentEnumConstants' in module 'FirstModule'}}
+// [email protected]:* {{definition has no member 'kDifferentEnumConstantsValueSecond'}}
+#endif
+
+#if defined(FIRST)
+enum DifferentEnumValues { kDifferentEnumValue = 0 };
+#elif defined(SECOND)
+enum DifferentEnumValues { kDifferentEnumValue = 1 };
+#else
+enum DifferentEnumValues differentEnumValue;
+// [email protected]:* {{'DifferentEnumValues' has different definitions in different modules; definition in module 'FirstModule' first difference is 1st element 'kDifferentEnumValue' has an initializer}}
+// [email protected]:* {{but in 'SecondModule' found 1st element 'kDifferentEnumValue' has different initializer}}
+#endif
+
+#if defined(FIRST)
+enum {
+ kAnonymousEnumValueFirst = 1,
+};
+#elif defined(SECOND)
+enum {
+ kAnonymousEnumValueSecond = 2,
+};
+#else
+// Anonymous enums don't have to match, no errors expected.
+int anonymousEnumValue = kAnonymousEnumValueFirst + kAnonymousEnumValueSecond;
+#endif
+
+// Keep macros contained to one file.
+#ifdef FIRST
+#undef FIRST
+#endif
+
+#ifdef SECOND
+#undef SECOND
+#endif
|
|
@llvm/pr-subscribers-clang Author: Volodymyr Sapsai (vsapsai) ChangesThere is no reason for C and Objective-C to differ from C++ in this matter. rdar://85531830 Full diff: https://github.com/llvm/llvm-project/pull/90298.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 744f11de88c2f8..6afbbfe20f26b7 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -805,9 +805,7 @@ void ASTDeclReader::VisitEnumDecl(EnumDecl *ED) {
// If this is a definition subject to the ODR, and we already have a
// definition, merge this one into it.
- if (ED->isCompleteDefinition() &&
- Reader.getContext().getLangOpts().Modules &&
- Reader.getContext().getLangOpts().CPlusPlus) {
+ if (ED->isCompleteDefinition() && Reader.getContext().getLangOpts().Modules) {
EnumDecl *&OldDef = Reader.EnumDefinitions[ED->getCanonicalDecl()];
if (!OldDef) {
// This is the first time we've seen an imported definition. Look for a
@@ -3304,8 +3302,7 @@ DeclContext *ASTDeclReader::getPrimaryContextForMerging(ASTReader &Reader,
return RD->getDefinition();
if (auto *ED = dyn_cast<EnumDecl>(DC))
- return ED->getASTContext().getLangOpts().CPlusPlus? ED->getDefinition()
- : nullptr;
+ return ED->getDefinition();
if (auto *OID = dyn_cast<ObjCInterfaceDecl>(DC))
return OID->getDefinition();
diff --git a/clang/test/Modules/odr_hash-enum.c b/clang/test/Modules/odr_hash-enum.c
new file mode 100644
index 00000000000000..f8ede923fe2caa
--- /dev/null
+++ b/clang/test/Modules/odr_hash-enum.c
@@ -0,0 +1,75 @@
+// Clear and create directories
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: mkdir %t/cache
+// RUN: mkdir %t/Inputs
+
+// Build first header file
+// RUN: echo "#define FIRST" >> %t/Inputs/first.h
+// RUN: cat %s >> %t/Inputs/first.h
+
+// Build second header file
+// RUN: echo "#define SECOND" >> %t/Inputs/second.h
+// RUN: cat %s >> %t/Inputs/second.h
+
+// Test that each header can compile
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/first.h
+// RUN: %clang_cc1 -fsyntax-only -x c %t/Inputs/second.h
+
+// Build module map file
+// RUN: echo "module FirstModule {" >> %t/Inputs/module.modulemap
+// RUN: echo " header \"first.h\"" >> %t/Inputs/module.modulemap
+// RUN: echo "}" >> %t/Inputs/module.modulemap
+// RUN: echo "module SecondModule {" >> %t/Inputs/module.modulemap
+// RUN: echo " header \"second.h\"" >> %t/Inputs/module.modulemap
+// RUN: echo "}" >> %t/Inputs/module.modulemap
+
+// Run test
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache -x c -I%t/Inputs -verify %s
+
+#if !defined(FIRST) && !defined(SECOND)
+#include "first.h"
+#include "second.h"
+#endif
+
+#if defined(FIRST)
+enum DifferentEnumConstants { kDifferentEnumConstantsValueFirst };
+#elif defined(SECOND)
+enum DifferentEnumConstants { kDifferentEnumConstantsValueSecond };
+#else
+enum DifferentEnumConstants differentEnumConstants;
+// [email protected]:* {{'kDifferentEnumConstantsValueSecond' from module 'SecondModule' is not present in definition of 'enum DifferentEnumConstants' in module 'FirstModule'}}
+// [email protected]:* {{definition has no member 'kDifferentEnumConstantsValueSecond'}}
+#endif
+
+#if defined(FIRST)
+enum DifferentEnumValues { kDifferentEnumValue = 0 };
+#elif defined(SECOND)
+enum DifferentEnumValues { kDifferentEnumValue = 1 };
+#else
+enum DifferentEnumValues differentEnumValue;
+// [email protected]:* {{'DifferentEnumValues' has different definitions in different modules; definition in module 'FirstModule' first difference is 1st element 'kDifferentEnumValue' has an initializer}}
+// [email protected]:* {{but in 'SecondModule' found 1st element 'kDifferentEnumValue' has different initializer}}
+#endif
+
+#if defined(FIRST)
+enum {
+ kAnonymousEnumValueFirst = 1,
+};
+#elif defined(SECOND)
+enum {
+ kAnonymousEnumValueSecond = 2,
+};
+#else
+// Anonymous enums don't have to match, no errors expected.
+int anonymousEnumValue = kAnonymousEnumValueFirst + kAnonymousEnumValueSecond;
+#endif
+
+// Keep macros contained to one file.
+#ifdef FIRST
+#undef FIRST
+#endif
+
+#ifdef SECOND
+#undef SECOND
+#endif
|
|
C doesn't have an odr, does it? |
For non-C++ "ODR" has a meaning more like "ODR-inspired checks". But there is no language rule that would require enforcement and there is no impact on linkage (at least during deserialization). |
Not sure I'm following the response here - but I guess what I'm trying to say, with more words, is that my understanding was that C doesn't have an ODR, and you can have different definitions of a type, with the same name, in C and that's OK. And it sounds like this change makes that not OK in some way? (either by diagnosing/erroring on such divergent types, or assuming they won't diverge and carrying on silently) |
In different translation units you can still use different types with the same name. And you can use enum constant with the same name but different values. It is still OK from the compiler's perspective.
The change is for Clang modules only. In the non-modular world including the same file twice (no header guards, no pragma once) would result in a type redefinition. But with modules it is more likely to find the same type definition in multiple modules. To make the compiler more practical we don't reject such situations. Though we detect when the types aren't identical and don't try to use them interchangeably. The change extends the existing behavior for structs/unions to enums. If you have use cases when different enums should be used interchangeably, I can see how to accommodate such use cases. |
|
@dwblaikie if you have no further comments, I'll merge this approved change some time soon (in a day or two). |
OK, still a bit confused though - "like in C++", I assume in C++ we reject mismatched types coming from different modules. But it sounds like in C you're suggesting that we (moreso with this patch) detect mismatches and silently allow different type definitions to work? Which would presumably be the right thing for C. So, I guess, maybe my concern is only with a confusing title/description... |
|
In C++ we reject mismatched types coming from different Clang modules. Haven't checked the behavior for C++20 modules as I'm not changing it. In C modules aren't a part of any standard as far as I know. But for Clang modules we reject most of the mismatched types coming from different modules. The current change extends the set of "most of the mismatched types" to include enums. |
There is no reason for C and Objective-C to differ from C++ in this matter.
rdar://85531830