fix: constexpr bit_cast with empty base classes

[sethp]

Prior to this commit, clang would fail to produce a constant value for b in:

struct base {
};

struct s : base {
    int z;
};

constexpr auto b = std::bit_cast<s>(0x12); 

e.g. https://godbolt.org/z/srrbTMPq4

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (sethp)

Changes

Prior to this commit, clang would fail to produce a constant value for b in:

struct base {
};

struct s : base {
    int z;
};

constexpr auto b = std::bit_cast<s>(0x12); 

e.g. https://godbolt.org/z/srrbTMPq4

---

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

2 Files Affected:

• (modified) clang/lib/AST/ExprConstant.cpp (-3)
• (modified) clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp (+5-1)

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index fcf8f6591a7923..e1863cbf6c317c 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -7312,9 +7312,6 @@ class BufferToAPValueConverter {
       for (size_t I = 0, E = CXXRD->getNumBases(); I != E; ++I) {
         const CXXBaseSpecifier &BS = CXXRD->bases_begin()[I];
         CXXRecordDecl *BaseDecl = BS.getType()->getAsCXXRecordDecl();
-        if (BaseDecl->isEmpty() ||
-            Info.Ctx.getASTRecordLayout(BaseDecl).getNonVirtualSize().isZero())
-          continue;
 
         std::optional<APValue> SubObj = visitType(
             BS.getType(), Layout.getBaseClassOffset(BaseDecl) + Offset);
diff --git a/clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp b/clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
index c5b8032f40b131..7520b43a194aba 100644
--- a/clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
+++ b/clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp
@@ -90,7 +90,8 @@ void test_record() {
   struct tuple4 {
     unsigned x, y, z, doublez;
 
-    constexpr bool operator==(tuple4 const &other) const {
+    bool operator==(tuple4 const &other) const = default;
+    constexpr bool operator==(bases const &other) const {
       return x == other.x && y == other.y &&
              z == other.z && doublez == other.doublez;
     }
@@ -99,6 +100,9 @@ void test_record() {
   constexpr tuple4 t4 = bit_cast<tuple4>(b);
   static_assert(t4 == tuple4{1, 2, 3, 4});
   static_assert(round_trip<tuple4>(b));
+
+  constexpr auto b2 = bit_cast<bases>(t4);
+  static_assert(t4 == b2);
 }
 
 void test_partially_initialized() {

[sethp]

Just to cross-link, I originally asked about this on discord: https://discord.com/channels/636084430946959380/636732781086638081/1209534556126973993

(cc @cor3ntin )

[cor3ntin]

This will need a release note.
I'm curious why the removed code was there in the first place. Any idea @tbaederr ?

[tbaederr]

This will need a release note. I'm curious why the removed code was there in the first place. Any idea @tbaederr ?

No idea, it's been there since the original commit introducing __builtin_bit_cast. Maybe just an optimization, or maybe something else in that loop used to assert that the class isn't empty.

Aside from the release not, this LGTM.

[cor3ntin]

LGTM, thanks

[sethp]

I'm curious why the removed code was there in the first place.

I had the same thought; there was nothing that was no test trying to produce an empty struct from a bit-cast before this change, which made me think it was (overly) defensive coding that didn't get fully exercised.

In a broader sense, I think that's maybe where there's interesting leverage: the constant evaluators in clang (& gcc too) have some, uh, emergent behaviors. It's hard to say whether they're on purpose or not: the testing strategy seems to be largely demonstrative (i.e. "there exists a situation that ought to work out like so") and based on hand-crafted examples.

Property-based testing (AKA "fuzz" testing) might offer a win here, if the properties were reasonably coherent to write. It'd be (relatively) easy to write a thing that verified something like "all sub-expressions of a constant-producing expression are themselves constant-producing expressions", which would've caught this bug. What I don't know is whether that property makes sense in the context of C++, though.

[sethp]

@cor3ntin @tbaederr is there anything else you need from me here? I would expect github's "squash and merge" button to work fine for this change, but I could rebase if you'd prefer.

[tbaederr]

There's still a merge conflict now. Do you not have the necessary permissions to merge it yourself?

[sethp]

Ah, got it, thank you: I am once again wishing for a .gitattributes "keep both" merge strategy more usable than union for the release notes.

Do you not have the necessary permissions to merge it yourself?

No, I don't have write permissions on the LLVM repo: I think I will need one of y'all's help hitting the button here. I'll look into getting permissions, too, but I presume there's some steps involved.