Author: Daedie-git Date: 2026-09-08T09:59:22+08:00 New Revision: 2b03c66b5f0fdd12f616d845b61417442322f03c
URL: https://github.com/llvm/llvm-project/commit/2b03c66b5f0fdd12f616d845b61417442322f03c DIFF: https://github.com/llvm/llvm-project/commit/2b03c66b5f0fdd12f616d845b61417442322f03c.diff LOG: [Clang][Modules] Fix ODR handling of types found through using-declarations (#221839) Fixes #78850. Clang can diagnose an ODR violation for identical definitions when a type is found directly in one module's global module fragment and through a using-declaration in another. The lookup routes produce different AST type wrappers even though they refer to the same declaration. Normalize `UsingType` to the target declaration's type before hashing it. Keep the keyword and qualifier from the use site, rather than from the using-declaration. This also lets the hash distinguish differently qualified uses without canonicalizing away meaningful spelling differences. The normalization exposes an existing assertion in the enum ODR diagnostic path: `AddEnumDecl` hashes canonical underlying integer types, but the diagnostic emitter compares hashes of their spelled types. Compare the actual underlying integer types instead, while retaining the original spelling in the diagnostic. The extended `pr76638.cppm` regression also reproduces the assertion without the normalization change, when both underlying types are found through same-named using-declarations. Use `EnumDecl::getIntegerType()` for this comparison so ignored cv qualifiers in the enum-base do not incorrectly take precedence over an actual difference in enumerator initializers. A regression checks that diagnostic as well. Regression coverage includes both lookup directions, typedefs and aliases, record and enum types, chained using-declarations, templates, and negative tests for qualification, elaborated keywords, and cv qualification. Remove the now-fixed #78850 example from the modules documentation. ## Validation Built from main at `44a4dbf32a6b54de32e4d064756d7a6ba5b9e808` plus this patch, in Release mode with assertions enabled on Windows x86-64 (X86 backend, Clang 23.1.0 bootstrap, VS 2022 SDK). | Suite | Passed | Unsupported | Expected Failures | | :------------ | -----: | ----------: | ----------------: | | Clang Modules | 914 | 34 | 0 | | Clang PCH | 289 | 6 | 2 | | **Total** | 1,203 | 40 | 2 | Unexpected failures: **0**. - Removing both implementation changes and rebuilding on the same main base makes both affected regression tests fail: the new lookup test reports false ODR errors, and the extended enum test asserts while diagnosing an ODR violation. Restoring the changes and rebuilding passes both complete suites. - The cv-qualifier regression also fails with the earlier comparison of spelled underlying types, then passes with `getIntegerType()`. - Changed implementation lines pass clang-format checks; `git diff --check` passes. The original release/23.x patch was also tested: 1,180 passed, 39 unsupported, 2 expected failures, and no unexpected failures across Modules and PCH. Its unmodified implementation reproduced the false-positive diagnostics, missed qualification diagnostic, and enum assertion. Other platforms and configurations have not been tested locally. This is a targeted ODR fix, not a general resolution of Clang's remaining C++ module limitations. The fix is also relevant to release/23.x; a backport can follow upstream review and merge. Assisted-by: OpenAI Codex Astra Co-authored-by: Bjorn Schobben <[email protected]> Added: clang/test/Modules/odr-hash-using-declarations.cppm Modified: clang/docs/ReleaseNotes.md clang/docs/StandardCPlusPlusModules.md clang/lib/AST/ODRDiagsEmitter.cpp clang/lib/AST/ODRHash.cpp clang/test/Modules/pr76638.cppm Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.md b/clang/docs/ReleaseNotes.md index d57876b429de3..38d5ecc09c52d 100644 --- a/clang/docs/ReleaseNotes.md +++ b/clang/docs/ReleaseNotes.md @@ -541,6 +541,13 @@ features cannot lower the translation-unit ABI level; #### Bug Fixes to C++ Support +- Fixed false-positive module ODR diagnostics when a type is found through a + using-declaration in one definition and directly in another. ODR hashing also + now distinguishes diff erently qualified uses of types found through + using-declarations. (#GH78850) +- Fixed an assertion when diagnosing module ODR violations for enum underlying + types found through using-declarations with the same name but diff erent types. + - Fixed a false type mismatch when a typedef naming an anonymous enumeration was used through a C++20 named module and its defining header was subsequently included. (#GH213299) diff --git a/clang/docs/StandardCPlusPlusModules.md b/clang/docs/StandardCPlusPlusModules.md index 4905f5ad2d49e..634cf2e100a9b 100644 --- a/clang/docs/StandardCPlusPlusModules.md +++ b/clang/docs/StandardCPlusPlusModules.md @@ -2084,39 +2084,7 @@ However, the behavior is inconsistent with other compilers. This is tracked by ODR violations are a common issue when using modules. Clang sometimes produces false-positive diagnostics or fails to produce true-positive diagnostics of the -One Definition Rule. One often-reported example is: - -```c++ -// part.cc -module; -typedef long T; -namespace ns { -inline void fun() { - (void)(T)0; -} -} -export module repro:part; - -// repro.cc -module; -typedef long T; -namespace ns { - using ::T; -} -namespace ns { -inline void fun() { - (void)(T)0; -} -} -export module repro; -export import :part; -``` - -Currently the compiler incorrectly diagnoses the inconsistent definition of -`fun()` in two module units. Because both definitions of `fun()` have the -same spelling and `T` refers to the same type entity, there is no ODR -violation. This is tracked by -[#78850](https://github.com/llvm/llvm-project/issues/78850). +One Definition Rule. #### Using TU-local entity in other units diff --git a/clang/lib/AST/ODRDiagsEmitter.cpp b/clang/lib/AST/ODRDiagsEmitter.cpp index 74f3881ed3c96..b78efa9b02c0e 100644 --- a/clang/lib/AST/ODRDiagsEmitter.cpp +++ b/clang/lib/AST/ODRDiagsEmitter.cpp @@ -1853,8 +1853,9 @@ bool ODRDiagsEmitter::diagnoseMismatch(const EnumDecl *FirstEnum, } if (!FirstUnderlyingType.isNull() && !SecondUnderlyingType.isNull()) { - if (computeODRHash(FirstUnderlyingType) != - computeODRHash(SecondUnderlyingType)) { + // Match AddEnumDecl, which hashes the canonical underlying type. + if (!Context.hasSameType(FirstEnum->getIntegerType(), + SecondEnum->getIntegerType())) { DiagError(FirstEnum, DifferentSpecifiedTypes) << FirstUnderlyingType; DiagNote(SecondEnum, DifferentSpecifiedTypes) << SecondUnderlyingType; return true; diff --git a/clang/lib/AST/ODRHash.cpp b/clang/lib/AST/ODRHash.cpp index 9151fa2dae5f6..85f61089fb860 100644 --- a/clang/lib/AST/ODRHash.cpp +++ b/clang/lib/AST/ODRHash.cpp @@ -956,6 +956,15 @@ class ODRTypeVisitor : public TypeVisitor<ODRTypeVisitor> { } void Visit(const Type *T) { + if (const auto *UsingT = dyn_cast<UsingType>(T)) { + // A using-declaration changes lookup, not the referenced entity. Preserve + // the keyword and qualifier at the use, not at the using-declaration. + const auto *Target = cast<TypeDecl>(UsingT->getDecl()->getTargetDecl()); + T = Target->getASTContext() + .getTypeDeclType(UsingT->getKeyword(), UsingT->getQualifier(), + Target) + .getTypePtr(); + } if (handleTypedef(T)) return; ID.AddInteger(T->getTypeClass()); diff --git a/clang/test/Modules/odr-hash-using-declarations.cppm b/clang/test/Modules/odr-hash-using-declarations.cppm new file mode 100644 index 0000000000000..46986a68e9438 --- /dev/null +++ b/clang/test/Modules/odr-hash-using-declarations.cppm @@ -0,0 +1,128 @@ +// RUN: split-file %s %t +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -emit-module-interface %t/part.cppm -o %t/part.pcm +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -fmodule-file=repro:part=%t/part.pcm -emit-module-interface %t/module.cppm -o %t/module.pcm -verify +// RUN: %clang_cc1 -std=c++20 -DREVERSE -fno-skip-odr-check-in-gmf -emit-module-interface %t/part.cppm -o %t/part.pcm +// RUN: %clang_cc1 -std=c++20 -DREVERSE -fno-skip-odr-check-in-gmf -fmodule-file=repro:part=%t/part.pcm -emit-module-interface %t/module.cppm -o %t/module.pcm -verify +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -emit-module-interface %t/first.cppm -o %t/first.pcm +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -emit-module-interface %t/second.cppm -o %t/second.pcm +// RUN: not %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -fprebuilt-module-path=%t -fsyntax-only %t/bad.cpp 2>&1 | FileCheck %s --check-prefix=BAD +// BAD-DAG: error: ' diff erent_spelling::qualification' has diff erent definitions +// BAD-DAG: error: ' diff erent_spelling::keyword' has diff erent definitions +// BAD-DAG: error: ' diff erent_spelling::cv' has diff erent definitions + +//--- declarations.h +typedef long T; +using Qualified = const volatile long; +struct Record { int value; }; +enum class Kind { value }; +typedef struct CRecord { int value; } CRecord; + +namespace chain { +using ::T; +} + +namespace ns { +#ifdef INDIRECT +using chain::T; +using ::Qualified; +using ::Record; +using ::Kind; +using ::CRecord; +#endif + +inline void fun() { (void)(T)0; } +inline Qualified *pointer(Qualified *p) { return p; } +inline Record make() { return Record{0}; } +inline unsigned sizes() { + return sizeof(T) + sizeof(Record) + sizeof(Kind) + sizeof(CRecord); +} +struct Fields { + T value; + Qualified *pointer; + Record record; + Kind kind; +}; +template <class U> inline void templ(U) { + const T value = 0; + (void)value; +} +} + +//--- part.cppm +module; +#ifdef REVERSE +#define INDIRECT +#endif +#include "declarations.h" +export module repro:part; +export void use() { + ns::Fields fields{}; + ns::fun(); + ns::pointer(nullptr); + ns::make(); + ns::sizes(); + ns::templ(0); +} + +//--- module.cppm +// expected-no-diagnostics +module; +#ifndef REVERSE +#define INDIRECT +#endif +#include "declarations.h" +export module repro; +export import :part; + +//--- bad.h +namespace types { +using Scalar = int; +struct Record {}; +} +namespace diff erent_spelling { +using types::Scalar; +using types::Record; +inline int qualification() { +#ifdef FIRST + return sizeof(Scalar); +#else + return sizeof( diff erent_spelling::Scalar); +#endif +} +inline int keyword() { +#ifdef FIRST + return sizeof(Record); +#else + return sizeof(struct Record); +#endif +} +inline int cv() { +#ifdef FIRST + return sizeof(Scalar); +#else + return sizeof(const Scalar); +#endif +} +} + +//--- first.cppm +module; +#define FIRST +#include "bad.h" +export module first; +export using diff erent_spelling::qualification; +export using diff erent_spelling::keyword; +export using diff erent_spelling::cv; + +//--- second.cppm +module; +#include "bad.h" +export module second; +export using diff erent_spelling::qualification; +export using diff erent_spelling::keyword; +export using diff erent_spelling::cv; + +//--- bad.cpp +import first; +import second; +int use() { return qualification() + keyword() + cv(); } diff --git a/clang/test/Modules/pr76638.cppm b/clang/test/Modules/pr76638.cppm index e4820ba3d79d9..9a6d584b88546 100644 --- a/clang/test/Modules/pr76638.cppm +++ b/clang/test/Modules/pr76638.cppm @@ -10,12 +10,22 @@ // RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \ // RUN: -fsyntax-only -verify +// Check the underlying types even when both are found through using-declarations. +// RUN: %clang_cc1 -std=c++20 -DBOTH_USE_USING %t/mod3.cppm -emit-module-interface -o %t/mod3.pcm +// RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm -fsyntax-only -verify + // Testing the behavior of `-fskip-odr-check-in-gmf` // RUN: %clang_cc1 -std=c++20 %t/mod3.cppm -fskip-odr-check-in-gmf \ // RUN: -emit-module-interface -o %t/mod3.pcm // RUN: %clang_cc1 -std=c++20 %t/mod4.cppm -fmodule-file=mod3=%t/mod3.pcm \ // RUN: -fskip-odr-check-in-gmf -DSKIP_ODR_CHECK_IN_GMF -fsyntax-only -verify +// Ignored cv qualifiers must not hide a diff erence in enumerator initializers. +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf -Wno-underlying-cv-qualifier-ignored \ +// RUN: %t/cv1.cppm -emit-module-interface -o %t/cv1.pcm +// RUN: %clang_cc1 -std=c++20 -fno-skip-odr-check-in-gmf \ +// RUN: %t/cv2.cppm -fmodule-file=cv1=%t/cv1.pcm -fsyntax-only -verify + //--- size_t.h extern "C" { @@ -58,6 +68,9 @@ extern "C" { //--- mod3.cppm module; #include "size_t.h" +#ifdef BOTH_USE_USING +#include "csize_t" +#endif #include "align.h" export module mod3; export using std::align_val_t; @@ -77,3 +90,24 @@ export using std::align_val_t; // [email protected]:* {{'std::align_val_t' has diff erent definitions in diff erent modules; defined here first diff erence is enum with specified type 'size_t' (aka 'int')}} // [email protected]:* {{but in 'mod3.<global>' found enum with specified type 'size_t' (aka 'unsigned int')}} #endif + +//--- cv1.cppm +module; +namespace ignored_cv { +using Int = int; +enum class E : const Int { value = 1 }; +} +export module cv1; +export using ignored_cv::E; + +//--- cv2.cppm +module; +namespace ignored_cv { +using Int = int; +enum class E : Int { value = 2 }; +} +export module cv2; +import cv1; +export using ignored_cv::E; +// [email protected]:* {{'ignored_cv::E' has diff erent definitions in diff erent modules; defined here first diff erence is 1st element 'value' has an initializer}} +// [email protected]:* {{but in 'cv1.<global>' found 1st element 'value' has diff erent initializer}} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
