Skip to content

[C++20][Modules] Fix false compilation error with constexpr #143168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

dmpolukhin
Copy link
Contributor

Use canonical field decl when evaluating constexpr to avoid resetting computed union value due to using different instances of the merged field decl. Pointer to the field comparison Value->isUnion() && Value->getUnionField() != FD few lines below requires stable ID for the field and canonical decl seems to be good option.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules labels Jun 6, 2025
@llvmbot
Copy link
Member

llvmbot commented Jun 6, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-modules

Author: Dmitry Polukhin (dmpolukhin)

Changes

Use canonical field decl when evaluating constexpr to avoid resetting computed union value due to using different instances of the merged field decl. Pointer to the field comparison Value->isUnion() && Value->getUnionField() != FD few lines below requires stable ID for the field and canonical decl seems to be good option.


Full diff: https://github.com/llvm/llvm-project/pull/143168.diff

2 Files Affected:

  • (modified) clang/lib/AST/ExprConstant.cpp (+1-1)
  • (added) clang/test/Modules/constexpr-initialization-failure.cpp (+44)
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index d1cc722fb7945..8f7016d8256fb 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -6781,7 +6781,7 @@ static bool HandleConstructorCall(const Expr *E, const LValue &This,
       // and make sure we've initialized every step along it.
       auto IndirectFieldChain = IFD->chain();
       for (auto *C : IndirectFieldChain) {
-        FD = cast<FieldDecl>(C);
+        FD = cast<FieldDecl>(C)->getCanonicalDecl();
         CXXRecordDecl *CD = cast<CXXRecordDecl>(FD->getParent());
         // Switch the union field if it differs. This happens if we had
         // preceding zero-initialization, and we're now initializing a union
diff --git a/clang/test/Modules/constexpr-initialization-failure.cpp b/clang/test/Modules/constexpr-initialization-failure.cpp
new file mode 100644
index 0000000000000..8ff20f2fc8ac6
--- /dev/null
+++ b/clang/test/Modules/constexpr-initialization-failure.cpp
@@ -0,0 +1,44 @@
+// RUN: rm -fR %t
+// RUN: split-file %s %t
+// RUN: cd %t
+// RUN: %clang_cc1 -verify -w -std=c++20 -fmodule-name=h1.h -emit-header-unit -xc++-user-header h1.h -o h1.pcm
+// RUN: %clang_cc1 -verify -w -std=c++20 -fmodule-map-file=module.modulemap -fmodule-file=h1.h=h1.pcm main.cpp -o main.o
+
+//--- module.modulemap
+module "h1.h" {
+  header "h1.h"
+  export *
+}
+
+//--- h0.h
+// expected-no-diagnostics
+#pragma once
+
+template <typename T> struct A {
+  union {
+    struct {
+      T x, y, z;
+    };
+  };
+  constexpr A(T, T, T) : x(), y(), z() {}
+};
+typedef A<float> packed_vec3;
+
+//--- h1.h
+// expected-no-diagnostics
+#pragma once
+
+#include "h0.h"
+
+constexpr packed_vec3 kMessThingsUp = packed_vec3(5.0f, 5.0f, 5.0f);
+
+//--- main.cpp
+// expected-no-diagnostics
+#include "h0.h"
+
+static_assert(sizeof(packed_vec3) == sizeof(float) * 3);
+static_assert(alignof(packed_vec3) == sizeof(float));
+
+import "h1.h";
+
+constexpr packed_vec3 kDefaultHalfExtents = packed_vec3(5.0f, 5.0f, 5.0f);

@cor3ntin
Copy link
Contributor

cor3ntin commented Jun 7, 2025

This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

@@ -6781,7 +6781,7 @@ static bool HandleConstructorCall(const Expr *E, const LValue &This,
// and make sure we've initialized every step along it.
auto IndirectFieldChain = IFD->chain();
for (auto *C : IndirectFieldChain) {
FD = cast<FieldDecl>(C);
FD = cast<FieldDecl>(C)->getCanonicalDecl();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly tired to see a lot of random "getCanonicalDecl()" in the code... but it is not your fault. Let it go.

Copy link
Contributor

@mizvekov mizvekov Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this change, this should just do !declaresSameEntity(Value->getUnionField(), FD) instead of Value->getUnionField() != FD a few lines below (outside of GitHub context).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this change, this should just do !declaresSameEntity(Value->getUnionField(), FD) instead of Value->getUnionField() != FD a few lines below (outside of GitHub context).

Done, it is basically the same getCanonicalDecl but better for explaining the intent.

Use cannonical field decl when evaluating constexpr to avoid resetting
computed union value due to using different instances of merged decls.
@dmpolukhin dmpolukhin force-pushed the constexpr-initialization-failure branch from 010a693 to 15aaef0 Compare June 9, 2025 09:55
@dmpolukhin dmpolukhin merged commit 9797b5f into llvm:main Jun 11, 2025
8 checks passed
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
)

Use declaresSameEntity when evaluating constexpr to avoid resetting
computed union value due to using different instances of the merged
field decl.
akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
)

Use declaresSameEntity when evaluating constexpr to avoid resetting
computed union value due to using different instances of the merged
field decl.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants