https://github.com/MitalAshok created https://github.com/llvm/llvm-project/pull/95718
Used to implement CWG2191 where `typeid` for a polymorphic glvalue only becomes potentially-throwing if the `typeid` operand was already potentially throwing or a `nullptr` check was inserted: https://cplusplus.github.io/CWG/issues/2191.html Also change `Expr::hasSideEffects` for `CXXTypeidExpr` to check the operand for side-effects instead of always reporting that there are side-effects Remove `IsDeref` parameter of `CGCXXABI::shouldTypeidBeNullChecked` because it should never return `true` if `!IsDeref` (we shouldn't add a null check that wasn't there in the first place) >From fc28e8f9b987ca35db457afaf19fa8c2af2f9574 Mon Sep 17 00:00:00 2001 From: Mital Ashok <mi...@mitalashok.co.uk> Date: Sun, 16 Jun 2024 20:27:15 +0100 Subject: [PATCH] [Clang] Introduce CXXTypeidExpr::hasNullCheck --- clang/docs/ReleaseNotes.rst | 3 ++ clang/include/clang/AST/ExprCXX.h | 4 ++ clang/lib/AST/Expr.cpp | 16 +++++-- clang/lib/AST/ExprCXX.cpp | 49 ++++++++++++++++++++++ clang/lib/CodeGen/CGCXXABI.h | 3 +- clang/lib/CodeGen/CGExprCXX.cpp | 53 ++++-------------------- clang/lib/CodeGen/ItaniumCXXABI.cpp | 7 ++-- clang/lib/CodeGen/MicrosoftCXXABI.cpp | 8 ++-- clang/lib/Sema/SemaExceptionSpec.cpp | 20 +++------ clang/test/CXX/drs/cwg21xx.cpp | 13 ++++++ clang/test/SemaCXX/warn-unused-value.cpp | 8 ++++ 11 files changed, 110 insertions(+), 74 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 69aea6c21ad39..6c92177d71298 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -268,6 +268,9 @@ Resolutions to C++ Defect Reports - Clang now requires a template argument list after a template keyword. (`CWG96: Syntactic disambiguation using the template keyword <https://cplusplus.github.io/CWG/issues/96.html>`_). +- Clang no longer always reports ``!noexcept(typeid(expr))`` when the ``typeid`` cannot throw a ``std::bad_typeid``. + (`CWG2191: Incorrect result for noexcept(typeid(v)) <https://cplusplus.github.io/CWG/issues/2191.html>`_). + C Language Changes ------------------ diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h index d2e8d93656359..c2feac525c1ea 100644 --- a/clang/include/clang/AST/ExprCXX.h +++ b/clang/include/clang/AST/ExprCXX.h @@ -919,6 +919,10 @@ class CXXTypeidExpr : public Expr { reinterpret_cast<Stmt **>(&const_cast<CXXTypeidExpr *>(this)->Operand); return const_child_range(begin, begin + 1); } + + /// Whether this is of a form like "typeid(*ptr)" that can throw a + /// std::bad_typeid if a pointer is a null pointer ([expr.typeid]p2) + bool hasNullCheck() const; }; /// A member reference to an MSPropertyDecl. diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index 7e555689b64c4..37ba5b69f446d 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -3769,10 +3769,18 @@ bool Expr::HasSideEffects(const ASTContext &Ctx, break; } - case CXXTypeidExprClass: - // typeid might throw if its subexpression is potentially-evaluated, so has - // side-effects in that case whether or not its subexpression does. - return cast<CXXTypeidExpr>(this)->isPotentiallyEvaluated(); + case CXXTypeidExprClass: { + const auto *TE = cast<CXXTypeidExpr>(this); + if (!TE->isPotentiallyEvaluated()) + return false; + + // If this type id expression can throw because of a null pointer, that is a + // side-effect independent of if the operand has a side-effect + if (IncludePossibleEffects && TE->hasNullCheck()) + return true; + + break; + } case CXXConstructExprClass: case CXXTemporaryObjectExprClass: { diff --git a/clang/lib/AST/ExprCXX.cpp b/clang/lib/AST/ExprCXX.cpp index 2abc0acbfde3b..7ecdb908e7d9f 100644 --- a/clang/lib/AST/ExprCXX.cpp +++ b/clang/lib/AST/ExprCXX.cpp @@ -166,6 +166,55 @@ QualType CXXTypeidExpr::getTypeOperand(ASTContext &Context) const { Operand.get<TypeSourceInfo *>()->getType().getNonReferenceType(), Quals); } +namespace { +static bool isGLValueFromPointerDeref(const Expr *E) { + E = E->IgnoreParens(); + + if (const auto *CE = dyn_cast<CastExpr>(E)) { + if (!CE->getSubExpr()->isGLValue()) + return false; + return isGLValueFromPointerDeref(CE->getSubExpr()); + } + + if (const auto *OVE = dyn_cast<OpaqueValueExpr>(E)) + return isGLValueFromPointerDeref(OVE->getSourceExpr()); + + if (const auto *BO = dyn_cast<BinaryOperator>(E)) + if (BO->getOpcode() == BO_Comma) + return isGLValueFromPointerDeref(BO->getRHS()); + + if (const auto *ACO = dyn_cast<AbstractConditionalOperator>(E)) + return isGLValueFromPointerDeref(ACO->getTrueExpr()) || + isGLValueFromPointerDeref(ACO->getFalseExpr()); + + // C++11 [expr.sub]p1: + // The expression E1[E2] is identical (by definition) to *((E1)+(E2)) + if (isa<ArraySubscriptExpr>(E)) + return true; + + if (const auto *UO = dyn_cast<UnaryOperator>(E)) + if (UO->getOpcode() == UO_Deref) + return true; + + return false; +} +} // namespace + +bool CXXTypeidExpr::hasNullCheck() const { + if (!isPotentiallyEvaluated()) + return false; + + // C++ [expr.typeid]p2: + // If the glvalue expression is obtained by applying the unary * operator to + // a pointer and the pointer is a null pointer value, the typeid expression + // throws the std::bad_typeid exception. + // + // However, this paragraph's intent is not clear. We choose a very generous + // interpretation which implores us to consider comma operators, conditional + // operators, parentheses and other such constructs. + return isGLValueFromPointerDeref(getExprOperand()); +} + QualType CXXUuidofExpr::getTypeOperand(ASTContext &Context) const { assert(isTypeOperand() && "Cannot call getTypeOperand for __uuidof(expr)"); Qualifiers Quals; diff --git a/clang/lib/CodeGen/CGCXXABI.h b/clang/lib/CodeGen/CGCXXABI.h index c7eccbd0095a9..104a20db8efaf 100644 --- a/clang/lib/CodeGen/CGCXXABI.h +++ b/clang/lib/CodeGen/CGCXXABI.h @@ -274,8 +274,7 @@ class CGCXXABI { getAddrOfCXXCatchHandlerType(QualType Ty, QualType CatchHandlerType) = 0; virtual CatchTypeInfo getCatchAllTypeInfo(); - virtual bool shouldTypeidBeNullChecked(bool IsDeref, - QualType SrcRecordTy) = 0; + virtual bool shouldTypeidBeNullChecked(QualType SrcRecordTy) = 0; virtual void EmitBadTypeidCall(CodeGenFunction &CGF) = 0; virtual llvm::Value *EmitTypeid(CodeGenFunction &CGF, QualType SrcRecordTy, Address ThisPtr, diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp index 3c4f59fc765fe..a2bdd7d0aa7b6 100644 --- a/clang/lib/CodeGen/CGExprCXX.cpp +++ b/clang/lib/CodeGen/CGExprCXX.cpp @@ -2143,40 +2143,9 @@ void CodeGenFunction::EmitCXXDeleteExpr(const CXXDeleteExpr *E) { } } -static bool isGLValueFromPointerDeref(const Expr *E) { - E = E->IgnoreParens(); - - if (const auto *CE = dyn_cast<CastExpr>(E)) { - if (!CE->getSubExpr()->isGLValue()) - return false; - return isGLValueFromPointerDeref(CE->getSubExpr()); - } - - if (const auto *OVE = dyn_cast<OpaqueValueExpr>(E)) - return isGLValueFromPointerDeref(OVE->getSourceExpr()); - - if (const auto *BO = dyn_cast<BinaryOperator>(E)) - if (BO->getOpcode() == BO_Comma) - return isGLValueFromPointerDeref(BO->getRHS()); - - if (const auto *ACO = dyn_cast<AbstractConditionalOperator>(E)) - return isGLValueFromPointerDeref(ACO->getTrueExpr()) || - isGLValueFromPointerDeref(ACO->getFalseExpr()); - - // C++11 [expr.sub]p1: - // The expression E1[E2] is identical (by definition) to *((E1)+(E2)) - if (isa<ArraySubscriptExpr>(E)) - return true; - - if (const auto *UO = dyn_cast<UnaryOperator>(E)) - if (UO->getOpcode() == UO_Deref) - return true; - - return false; -} - static llvm::Value *EmitTypeidFromVTable(CodeGenFunction &CGF, const Expr *E, - llvm::Type *StdTypeInfoPtrTy) { + llvm::Type *StdTypeInfoPtrTy, + bool HasNullCheck) { // Get the vtable pointer. Address ThisPtr = CGF.EmitLValue(E).getAddress(); @@ -2189,16 +2158,11 @@ static llvm::Value *EmitTypeidFromVTable(CodeGenFunction &CGF, const Expr *E, CGF.EmitTypeCheck(CodeGenFunction::TCK_DynamicOperation, E->getExprLoc(), ThisPtr, SrcRecordTy); - // C++ [expr.typeid]p2: - // If the glvalue expression is obtained by applying the unary * operator to - // a pointer and the pointer is a null pointer value, the typeid expression - // throws the std::bad_typeid exception. - // - // However, this paragraph's intent is not clear. We choose a very generous - // interpretation which implores us to consider comma operators, conditional - // operators, parentheses and other such constructs. - if (CGF.CGM.getCXXABI().shouldTypeidBeNullChecked( - isGLValueFromPointerDeref(E), SrcRecordTy)) { + // Whether we need an explicit null pointer check + // For example, with the Windows ABI, if this is a call to __RTtypeid, + // __RTtypeid(nullptr) is allowed will throw the std::bad_typeid + if (HasNullCheck && + CGF.CGM.getCXXABI().shouldTypeidBeNullChecked(SrcRecordTy)) { llvm::BasicBlock *BadTypeidBlock = CGF.createBasicBlock("typeid.bad_typeid"); llvm::BasicBlock *EndBlock = CGF.createBasicBlock("typeid.end"); @@ -2244,7 +2208,8 @@ llvm::Value *CodeGenFunction::EmitCXXTypeidExpr(const CXXTypeidExpr *E) { // type) to which the glvalue refers. // If the operand is already most derived object, no need to look up vtable. if (E->isPotentiallyEvaluated() && !E->isMostDerived(getContext())) - return EmitTypeidFromVTable(*this, E->getExprOperand(), PtrTy); + return EmitTypeidFromVTable(*this, E->getExprOperand(), PtrTy, + E->hasNullCheck()); QualType OperandTy = E->getExprOperand()->getType(); return MaybeASCast(CGM.GetAddrOfRTTIDescriptor(OperandTy)); diff --git a/clang/lib/CodeGen/ItaniumCXXABI.cpp b/clang/lib/CodeGen/ItaniumCXXABI.cpp index 8427286dee887..b6c76e1e8ea75 100644 --- a/clang/lib/CodeGen/ItaniumCXXABI.cpp +++ b/clang/lib/CodeGen/ItaniumCXXABI.cpp @@ -178,7 +178,7 @@ class ItaniumCXXABI : public CodeGen::CGCXXABI { return CatchTypeInfo{getAddrOfRTTIDescriptor(Ty), 0}; } - bool shouldTypeidBeNullChecked(bool IsDeref, QualType SrcRecordTy) override; + bool shouldTypeidBeNullChecked(QualType SrcRecordTy) override; void EmitBadTypeidCall(CodeGenFunction &CGF) override; llvm::Value *EmitTypeid(CodeGenFunction &CGF, QualType SrcRecordTy, Address ThisPtr, @@ -1419,9 +1419,8 @@ static llvm::FunctionCallee getBadTypeidFn(CodeGenFunction &CGF) { return CGF.CGM.CreateRuntimeFunction(FTy, "__cxa_bad_typeid"); } -bool ItaniumCXXABI::shouldTypeidBeNullChecked(bool IsDeref, - QualType SrcRecordTy) { - return IsDeref; +bool ItaniumCXXABI::shouldTypeidBeNullChecked(QualType SrcRecordTy) { + return true; } void ItaniumCXXABI::EmitBadTypeidCall(CodeGenFunction &CGF) { diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp index e4f798f6a97d9..9ab634fa6ce2e 100644 --- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp +++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp @@ -144,7 +144,7 @@ class MicrosoftCXXABI : public CGCXXABI { return CatchTypeInfo{nullptr, 0x40}; } - bool shouldTypeidBeNullChecked(bool IsDeref, QualType SrcRecordTy) override; + bool shouldTypeidBeNullChecked(QualType SrcRecordTy) override; void EmitBadTypeidCall(CodeGenFunction &CGF) override; llvm::Value *EmitTypeid(CodeGenFunction &CGF, QualType SrcRecordTy, Address ThisPtr, @@ -977,11 +977,9 @@ MicrosoftCXXABI::performBaseAdjustment(CodeGenFunction &CGF, Address Value, PolymorphicBase); } -bool MicrosoftCXXABI::shouldTypeidBeNullChecked(bool IsDeref, - QualType SrcRecordTy) { +bool MicrosoftCXXABI::shouldTypeidBeNullChecked(QualType SrcRecordTy) { const CXXRecordDecl *SrcDecl = SrcRecordTy->getAsCXXRecordDecl(); - return IsDeref && - !getContext().getASTRecordLayout(SrcDecl).hasExtendableVFPtr(); + return !getContext().getASTRecordLayout(SrcDecl).hasExtendableVFPtr(); } static llvm::CallBase *emitRTtypeidCall(CodeGenFunction &CGF, diff --git a/clang/lib/Sema/SemaExceptionSpec.cpp b/clang/lib/Sema/SemaExceptionSpec.cpp index 17acfca6b0112..67e0c7c63909e 100644 --- a/clang/lib/Sema/SemaExceptionSpec.cpp +++ b/clang/lib/Sema/SemaExceptionSpec.cpp @@ -1114,21 +1114,10 @@ static CanThrowResult canTypeidThrow(Sema &S, const CXXTypeidExpr *DC) { if (DC->isTypeOperand()) return CT_Cannot; - Expr *Op = DC->getExprOperand(); - if (Op->isTypeDependent()) + if (DC->isValueDependent()) return CT_Dependent; - const RecordType *RT = Op->getType()->getAs<RecordType>(); - if (!RT) - return CT_Cannot; - - if (!cast<CXXRecordDecl>(RT->getDecl())->isPolymorphic()) - return CT_Cannot; - - if (Op->Classify(S.Context).isPRValue()) - return CT_Cannot; - - return CT_Can; + return DC->hasNullCheck() ? CT_Can : CT_Cannot; } CanThrowResult Sema::canThrow(const Stmt *S) { @@ -1157,8 +1146,9 @@ CanThrowResult Sema::canThrow(const Stmt *S) { } case Expr::CXXTypeidExprClass: - // - a potentially evaluated typeid expression applied to a glvalue - // expression whose type is a polymorphic class type + // - a potentially evaluated typeid expression applied to a (possibly + // parenthesized) built-in unary * operator applied to a pointer to a + // polymorphic class type return canTypeidThrow(*this, cast<CXXTypeidExpr>(S)); // - a potentially evaluated call to a function, member function, function diff --git a/clang/test/CXX/drs/cwg21xx.cpp b/clang/test/CXX/drs/cwg21xx.cpp index 082deb42e4fa0..d7bc52dd9d446 100644 --- a/clang/test/CXX/drs/cwg21xx.cpp +++ b/clang/test/CXX/drs/cwg21xx.cpp @@ -11,6 +11,10 @@ // cxx98-error@-1 {{variadic macros are a C99 feature}} #endif +namespace std { +struct type_info; +} + namespace cwg2100 { // cwg2100: 12 template<const int *P, bool = true> struct X {}; template<typename T> struct A { @@ -231,6 +235,15 @@ static_assert(!__is_trivially_assignable(NonConstCopy &&, NonConstCopy &&), ""); #endif } // namespace cwg2171 +namespace cwg2191 { // cwg2191: 19 +#if __cplusplus >= 201103L +struct B { virtual void f() { } }; +struct D : B { } d; +static_assert(noexcept(typeid(d)), ""); +static_assert(!noexcept(typeid(*static_cast<D*>(nullptr))), ""); +#endif +} // namespace cwg2191 + namespace cwg2180 { // cwg2180: yes class A { A &operator=(const A &); // #cwg2180-A-copy diff --git a/clang/test/SemaCXX/warn-unused-value.cpp b/clang/test/SemaCXX/warn-unused-value.cpp index d964684069155..a2bf5148a4e9b 100644 --- a/clang/test/SemaCXX/warn-unused-value.cpp +++ b/clang/test/SemaCXX/warn-unused-value.cpp @@ -102,6 +102,14 @@ void f() { Bad b; (void)typeid(b.f()); // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}} + extern Bad * pb; + // This typeid can throw but that is not a side-effect that we care about + // warning for since this is idiomatic code + (void)typeid(*pb); + (void)sizeof(typeid(*pb)); + (void)typeid(*++pb); // expected-warning {{expression with side effects will be evaluated despite being used as an operand to 'typeid'}} + (void)sizeof(typeid(*++pb)); // expected-warning {{expression with side effects has no effect in an unevaluated context}} + // A dereference of a volatile pointer is a side effecting operation, however // since it is idiomatic code, and the alternatives induce higher maintenance // costs, it is allowed. _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
