fix: constexpr bit_cast with empty base classes by sethp · Pull Request #82383 · llvm/llvm-project (original) (raw)
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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(0x12);
e.g. https://godbolt.org/z/srrbTMPq4
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
Clang issues not falling into any other category
Language frontend issues, e.g. anything involving "Sema"
labels
@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(b); static_assert(t4 == tuple4{1, 2, 3, 4}); static_assert(round_trip(b));
- constexpr auto b2 = bit_cast(t4);
- static_assert(t4 == b2); }
void test_partially_initialized() {
This will need a release note.
I'm curious why the removed code was there in the first place. Any idea @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
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.
@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.
There's still a merge conflict now. Do you not have the necessary permissions to merge it yourself?
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.
Labels
Language frontend issues, e.g. anything involving "Sema"
Clang issues not falling into any other category