[clang] [clang][bytecode] Diagnose comparisons with literals (PR #106734)
Timm Baeder via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 30 17:54:33 PDT 2024
https://github.com/tbaederr updated https://github.com/llvm/llvm-project/pull/106734
>From 0786e92abfceb279ad79b3da3c33b3c645aa8754 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Timm=20B=C3=A4der?= <tbaeder at redhat.com>
Date: Fri, 30 Aug 2024 15:51:17 +0200
Subject: [PATCH] [clang][bytecode] Diagnose comparisons with literals
This requires adding a new opcode for PointerToBoolean casts,
since we otherwise emit too many diagnostics. But that fixes an older
problem when casting weak pointers to bool.
---
clang/lib/AST/ByteCode/Compiler.cpp | 11 +---
clang/lib/AST/ByteCode/Interp.h | 62 +++++++++++++------
clang/lib/AST/ByteCode/MemberPointer.h | 5 ++
clang/lib/AST/ByteCode/Opcodes.td | 5 ++
clang/lib/AST/ByteCode/Pointer.cpp | 11 ++++
clang/lib/AST/ByteCode/Pointer.h | 4 ++
clang/test/AST/ByteCode/builtin-functions.cpp | 6 ++
clang/test/AST/ByteCode/weak.cpp | 6 --
8 files changed, 76 insertions(+), 34 deletions(-)
diff --git a/clang/lib/AST/ByteCode/Compiler.cpp b/clang/lib/AST/ByteCode/Compiler.cpp
index 9bd77edb0a550f..dced9ea3493732 100644
--- a/clang/lib/AST/ByteCode/Compiler.cpp
+++ b/clang/lib/AST/ByteCode/Compiler.cpp
@@ -505,14 +505,9 @@ bool Compiler<Emitter>::VisitCastExpr(const CastExpr *CE) {
case CK_MemberPointerToBoolean: {
PrimType PtrT = classifyPrim(SubExpr->getType());
- // Just emit p != nullptr for this.
if (!this->visit(SubExpr))
return false;
-
- if (!this->emitNull(PtrT, nullptr, CE))
- return false;
-
- return this->emitNE(PtrT, CE);
+ return this->emitIsNonNull(PtrT, CE);
}
case CK_IntegralComplexToBoolean:
@@ -2323,8 +2318,8 @@ bool Compiler<Emitter>::VisitMaterializeTemporaryExpr(
// For everyhing else, use local variables.
if (SubExprT) {
- unsigned LocalIndex = allocateLocalPrimitive(
- SubExpr, *SubExprT, /*IsConst=*/true, /*IsExtended=*/true);
+ unsigned LocalIndex = allocateLocalPrimitive(E, *SubExprT, /*IsConst=*/true,
+ /*IsExtended=*/true);
if (!this->visit(SubExpr))
return false;
if (!this->emitSetLocal(*SubExprT, LocalIndex, E))
diff --git a/clang/lib/AST/ByteCode/Interp.h b/clang/lib/AST/ByteCode/Interp.h
index f8af51b71101d2..aa790a71a6b476 100644
--- a/clang/lib/AST/ByteCode/Interp.h
+++ b/clang/lib/AST/ByteCode/Interp.h
@@ -986,24 +986,7 @@ inline bool CmpHelperEQ<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
}
}
- if (!Pointer::hasSameBase(LHS, RHS)) {
- if (LHS.isOnePastEnd() && !RHS.isOnePastEnd() && !RHS.isZero() &&
- RHS.getOffset() == 0) {
- const SourceInfo &Loc = S.Current->getSource(OpPC);
- S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_past_end)
- << LHS.toDiagnosticString(S.getASTContext());
- return false;
- } else if (RHS.isOnePastEnd() && !LHS.isOnePastEnd() && !LHS.isZero() &&
- LHS.getOffset() == 0) {
- const SourceInfo &Loc = S.Current->getSource(OpPC);
- S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_past_end)
- << RHS.toDiagnosticString(S.getASTContext());
- return false;
- }
-
- S.Stk.push<BoolT>(BoolT::from(Fn(ComparisonCategoryResult::Unordered)));
- return true;
- } else {
+ if (Pointer::hasSameBase(LHS, RHS)) {
unsigned VL = LHS.getByteOffset();
unsigned VR = RHS.getByteOffset();
@@ -1019,6 +1002,35 @@ inline bool CmpHelperEQ<Pointer>(InterpState &S, CodePtr OpPC, CompareFn Fn) {
S.Stk.push<BoolT>(BoolT::from(Fn(Compare(VL, VR))));
return true;
}
+ // Otherwise we need to do a bunch of extra checks before returning Unordered.
+ if (LHS.isOnePastEnd() && !RHS.isOnePastEnd() && !RHS.isZero() &&
+ RHS.getOffset() == 0) {
+ const SourceInfo &Loc = S.Current->getSource(OpPC);
+ S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_past_end)
+ << LHS.toDiagnosticString(S.getASTContext());
+ return false;
+ } else if (RHS.isOnePastEnd() && !LHS.isOnePastEnd() && !LHS.isZero() &&
+ LHS.getOffset() == 0) {
+ const SourceInfo &Loc = S.Current->getSource(OpPC);
+ S.FFDiag(Loc, diag::note_constexpr_pointer_comparison_past_end)
+ << RHS.toDiagnosticString(S.getASTContext());
+ return false;
+ }
+
+ bool BothNonNull = !LHS.isZero() && !RHS.isZero();
+ // Reject comparisons to literals.
+ for (const auto &P : {LHS, RHS}) {
+ if (P.isZero())
+ continue;
+ if (BothNonNull && P.pointsToLiteral()) {
+ const SourceInfo &Loc = S.Current->getSource(OpPC);
+ S.FFDiag(Loc, diag::note_constexpr_literal_comparison);
+ return false;
+ }
+ }
+
+ S.Stk.push<BoolT>(BoolT::from(Fn(ComparisonCategoryResult::Unordered)));
+ return true;
}
template <>
@@ -1030,9 +1042,10 @@ inline bool CmpHelperEQ<MemberPointer>(InterpState &S, CodePtr OpPC,
// If either operand is a pointer to a weak function, the comparison is not
// constant.
for (const auto &MP : {LHS, RHS}) {
- if (const CXXMethodDecl *MD = MP.getMemberFunction(); MD && MD->isWeak()) {
+ if (MP.isWeak()) {
const SourceInfo &Loc = S.Current->getSource(OpPC);
- S.FFDiag(Loc, diag::note_constexpr_mem_pointer_weak_comparison) << MD;
+ S.FFDiag(Loc, diag::note_constexpr_mem_pointer_weak_comparison)
+ << MP.getMemberFunction();
return false;
}
}
@@ -2291,6 +2304,15 @@ inline bool Null(InterpState &S, CodePtr OpPC, const Descriptor *Desc) {
return true;
}
+template <PrimType Name, class T = typename PrimConv<Name>::T>
+inline bool IsNonNull(InterpState &S, CodePtr OpPC) {
+ const auto &P = S.Stk.pop<T>();
+ if (P.isWeak())
+ return false;
+ S.Stk.push<Boolean>(Boolean::from(!P.isZero()));
+ return true;
+}
+
//===----------------------------------------------------------------------===//
// This, ImplicitThis
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/AST/ByteCode/MemberPointer.h b/clang/lib/AST/ByteCode/MemberPointer.h
index 2b3be124db4267..de135a40a3c77b 100644
--- a/clang/lib/AST/ByteCode/MemberPointer.h
+++ b/clang/lib/AST/ByteCode/MemberPointer.h
@@ -84,6 +84,11 @@ class MemberPointer final {
bool isZero() const { return Base.isZero() && !Dcl; }
bool hasBase() const { return !Base.isZero(); }
+ bool isWeak() const {
+ if (const auto *MF = getMemberFunction())
+ return MF->isWeak();
+ return false;
+ }
void print(llvm::raw_ostream &OS) const {
OS << "MemberPtr(" << Base << " " << (const void *)Dcl << " + " << PtrOffset
diff --git a/clang/lib/AST/ByteCode/Opcodes.td b/clang/lib/AST/ByteCode/Opcodes.td
index 7374a441c8bb19..f286c71a129d1d 100644
--- a/clang/lib/AST/ByteCode/Opcodes.td
+++ b/clang/lib/AST/ByteCode/Opcodes.td
@@ -597,6 +597,11 @@ def Comp: Opcode {
let HasGroup = 1;
}
+def IsNonNull : Opcode {
+ let Types = [PtrTypeClass];
+ let HasGroup = 1;
+}
+
//===----------------------------------------------------------------------===//
// Cast, CastFP.
//===----------------------------------------------------------------------===//
diff --git a/clang/lib/AST/ByteCode/Pointer.cpp b/clang/lib/AST/ByteCode/Pointer.cpp
index 5b9e83764cfa50..9eaf0db45c7451 100644
--- a/clang/lib/AST/ByteCode/Pointer.cpp
+++ b/clang/lib/AST/ByteCode/Pointer.cpp
@@ -463,6 +463,17 @@ bool Pointer::hasSameArray(const Pointer &A, const Pointer &B) {
A.getFieldDesc()->IsArray;
}
+bool Pointer::pointsToLiteral() const {
+ if (isZero() || !isBlockPointer())
+ return false;
+
+ const Expr *E = block()->getDescriptor()->asExpr();
+ if (block()->isDynamic())
+ return false;
+
+ return E && !isa<MaterializeTemporaryExpr, StringLiteral>(E);
+}
+
std::optional<APValue> Pointer::toRValue(const Context &Ctx,
QualType ResultType) const {
const ASTContext &ASTCtx = Ctx.getASTContext();
diff --git a/clang/lib/AST/ByteCode/Pointer.h b/clang/lib/AST/ByteCode/Pointer.h
index ef90e6e0dd7bd1..d05d8e9bc1f388 100644
--- a/clang/lib/AST/ByteCode/Pointer.h
+++ b/clang/lib/AST/ByteCode/Pointer.h
@@ -692,6 +692,10 @@ class Pointer {
/// Checks if both given pointers point to the same block.
static bool pointToSameBlock(const Pointer &A, const Pointer &B);
+ /// Whether this points to a block that's been created for a "literal lvalue",
+ /// i.e. a non-MaterializeTemporaryExpr Expr.
+ bool pointsToLiteral() const;
+
/// Prints the pointer.
void print(llvm::raw_ostream &OS) const;
diff --git a/clang/test/AST/ByteCode/builtin-functions.cpp b/clang/test/AST/ByteCode/builtin-functions.cpp
index 1cff2228cd7a97..e215597579224c 100644
--- a/clang/test/AST/ByteCode/builtin-functions.cpp
+++ b/clang/test/AST/ByteCode/builtin-functions.cpp
@@ -951,3 +951,9 @@ namespace shufflevector {
}
#endif
+
+namespace FunctionStart {
+ void a(void) {}
+ static_assert(__builtin_function_start(a) == a, ""); // both-error {{not an integral constant expression}} \
+ // both-note {{comparison of addresses of literals has unspecified value}}
+}
diff --git a/clang/test/AST/ByteCode/weak.cpp b/clang/test/AST/ByteCode/weak.cpp
index d4aac3ff764dde..0322241beef83b 100644
--- a/clang/test/AST/ByteCode/weak.cpp
+++ b/clang/test/AST/ByteCode/weak.cpp
@@ -1,14 +1,8 @@
// RUN: %clang_cc1 -std=c++20 -fexperimental-new-constant-interpreter -verify=expected,both %s
// RUN: %clang_cc1 -std=c++20 -verify=ref,both %s
-
-
-
-/// FIXME: The new interpreter also emits the "address of weak declaration" note in the pointer-to-bool case.
-
[[gnu::weak]] extern int a;
int ha[(bool)&a]; // both-warning {{variable length arrays in C++ are a Clang extension}} \
- // expected-note {{comparison against address of weak declaration}} \
// both-error {{variable length array declaration not allowed at file scope}}
int ha2[&a == nullptr]; // both-warning {{variable length arrays in C++ are a Clang extension}} \
// both-note {{comparison against address of weak declaration '&a' can only be performed at runtime}} \
More information about the cfe-commits
mailing list