Skip to content

[clang][Interp] Bail out on missing ComparisonCategoryInfo #80131

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.

Sign up for GitHub

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
Feb 6, 2024
Merged

[clang][Interp] Bail out on missing ComparisonCategoryInfo #80131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3548cdb
[clang][Interp] Bail out on missing ComparisonCategoryInfo
tbaederr Jan 31, 2024
b2a76d1
Check for C++ instead
tbaederr Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/lib/AST/Interp/ByteCodeExprGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ bool ByteCodeExprGen<Emitter>::VisitBinaryOperator(const BinaryOperator *BO) {
// Special case for C++'s three-way/spaceship operator <=>, which
// returns a std::{strong,weak,partial}_ordering (which is a class, so doesn't
// have a PrimType).
if (!T) {
if (!T && Ctx.getLangOpts().CPlusPlus) {
if (DiscardResult)
return true;
const ComparisonCategoryInfo *CmpInfo =
Expand Down
1 change: 1 addition & 0 deletions clang/test/Sema/struct-cast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -fsyntax-only %s -verify
// RUN: %clang_cc1 -fsyntax-only %s -fexperimental-new-constant-interpreter -verify
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm really confused what this test has to do with the changes above. The changes relate to three-way comparison operators but this test is a C test file and has no comparison operators involved. Why would this test hit that assertion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AST we trip up on is

BinaryOperator 0x52100006ed08 'struct S' '='
|-DeclRefExpr 0x52100006eba0 'struct S' lvalue Var 0x52100006e998 'tmp' 'struct S'
`-CStyleCastExpr 0x52100006ecd8 'struct S' <NoOp>
  `-CallExpr 0x52100006ec88 'struct S'
    `-ImplicitCastExpr 0x52100006ec68 'const struct S (*)(void)' <FunctionToPointerDecay>
      `-DeclRefExpr 0x52100006ebc8 'const struct S (void)' Function 0x52100006e870 'foo' 'const struct S (void)'

IIUC this should be rejected since the assignment doesn't work, but we're asserting that we found CmpInfo. We could of course also add more assertions that we're even in C++ mode when looking for the spaceship operator stuff.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should this assignment not work? (The code looks valid to me). Oh, wait, you mean this assignment shouldn't be a constant expression? (If so, I agree.)

That said, I see now why we're hitting the assertion on an assignment operator in C; the call to lookupInfoForType() will return nullptr in that case and we need to handle that.

I don't think we want to look for the spaceship operator stuff outside of C++ mode, but that can be done in a follow-up as this is fixing a failing assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, what I mean is, the constant interpreter should return false for this assignment in C.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Excellent, thank you!

// expected-no-diagnostics

struct S {
Expand Down