Author: Fangrui Song Date: 2023-01-05T14:36:36-08:00 New Revision: cf8fd210a35c8e93136cb8edc5c6a2e818dc1b1d
URL: https://github.com/llvm/llvm-project/commit/cf8fd210a35c8e93136cb8edc5c6a2e818dc1b1d DIFF: https://github.com/llvm/llvm-project/commit/cf8fd210a35c8e93136cb8edc5c6a2e818dc1b1d.diff LOG: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn In C mode, if e1 has __attribute__((noreturn)) but e2 doesn't, `(c ? e1 : e2)` is incorrectly noreturn and Clang codegen produces `unreachable` which may lead to miscompiles (see [1] `gawk/support/dfa.c`). This problem has been known since 8c6b56f39d967347f28dd9c93f1cffddf6d7e4cd (2010) or earlier. Fix this by making the result type noreturn only if both e1 and e2 are noreturn, matching GCC. `_Noreturn` and `[[noreturn]]` do not have the aforementioned problem. Fix https://github.com/llvm/llvm-project/issues/59792 [1] Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D140868 Added: Modified: clang/docs/ReleaseNotes.rst clang/include/clang/AST/ASTContext.h clang/lib/AST/ASTContext.cpp clang/lib/Sema/SemaExpr.cpp clang/test/CodeGen/attr-noreturn.c Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index ba464df6c0d3b..1e370acff3cc2 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -335,6 +335,9 @@ Bug Fixes incorrect line numbers in some cases when a header file used an end of line character sequence that diff ered from the primary source file. `Issue 59736 <https://github.com/llvm/llvm-project/issues/59736>`_ +- In C mode, when ``e1`` has ``__attribute__((noreturn))`` but ``e2`` doesn't, + ``(c ? e1 : e2)`` is no longer considered noreturn. + `Issue 59792 <https://github.com/llvm/llvm-project/issues/59792>`_ Improvements to Clang's diagnostics ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 326f115f1bda0..4c0f5075cf2b6 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -2865,10 +2865,12 @@ class ASTContext : public RefCountedBase<ASTContext> { bool canBindObjCObjectType(QualType To, QualType From); // Functions for calculating composite types - QualType mergeTypes(QualType, QualType, bool OfBlockPointer=false, - bool Unqualified = false, bool BlockReturnType = false); - QualType mergeFunctionTypes(QualType, QualType, bool OfBlockPointer=false, - bool Unqualified = false, bool AllowCXX = false); + QualType mergeTypes(QualType, QualType, bool OfBlockPointer = false, + bool Unqualified = false, bool BlockReturnType = false, + bool IsConditionalOperator = false); + QualType mergeFunctionTypes(QualType, QualType, bool OfBlockPointer = false, + bool Unqualified = false, bool AllowCXX = false, + bool IsConditionalOperator = false); QualType mergeFunctionParameterTypes(QualType, QualType, bool OfBlockPointer = false, bool Unqualified = false); diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index d90d59380534e..263123dbbbe0f 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -10191,7 +10191,8 @@ QualType ASTContext::mergeFunctionParameterTypes(QualType lhs, QualType rhs, QualType ASTContext::mergeFunctionTypes(QualType lhs, QualType rhs, bool OfBlockPointer, bool Unqualified, - bool AllowCXX) { + bool AllowCXX, + bool IsConditionalOperator) { const auto *lbase = lhs->castAs<FunctionType>(); const auto *rbase = rhs->castAs<FunctionType>(); const auto *lproto = dyn_cast<FunctionProtoType>(lbase); @@ -10254,9 +10255,27 @@ QualType ASTContext::mergeFunctionTypes(QualType lhs, QualType rhs, if (lbaseInfo.getNoCfCheck() != rbaseInfo.getNoCfCheck()) return {}; - // FIXME: some uses, e.g. conditional exprs, really want this to be 'both'. - bool NoReturn = lbaseInfo.getNoReturn() || rbaseInfo.getNoReturn(); - + // When merging declarations, it's common for supplemental information like + // attributes to only be present in one of the declarations, and we generally + // want type merging to preserve the union of information. So a merged + // function type should be noreturn if it was noreturn in *either* operand + // type. + // + // But for the conditional operator, this is backwards. The result of the + // operator could be either operand, and its type should conservatively + // reflect that. So a function type in a composite type is noreturn only + // if it's noreturn in *both* operand types. + // + // Arguably, noreturn is a kind of subtype, and the conditional operator + // ought to produce the most specific common supertype of its operand types. + // That would diff er from this rule in contravariant positions. However, + // neither C nor C++ generally uses this kind of subtype reasoning. Also, + // as a practical matter, it would only affect C code that does abstraction of + // higher-order functions (taking noreturn callbacks!), which is uncommon to + // say the least. So we use the simpler rule. + bool NoReturn = IsConditionalOperator + ? lbaseInfo.getNoReturn() && rbaseInfo.getNoReturn() + : lbaseInfo.getNoReturn() || rbaseInfo.getNoReturn(); if (lbaseInfo.getNoReturn() != NoReturn) allLTypes = false; if (rbaseInfo.getNoReturn() != NoReturn) @@ -10389,9 +10408,9 @@ static QualType mergeEnumWithInteger(ASTContext &Context, const EnumType *ET, return {}; } -QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, - bool OfBlockPointer, - bool Unqualified, bool BlockReturnType) { +QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, bool OfBlockPointer, + bool Unqualified, bool BlockReturnType, + bool IsConditionalOperator) { // For C++ we will not reach this code with reference types (see below), // for OpenMP variant call overloading we might. // @@ -10684,7 +10703,8 @@ QualType ASTContext::mergeTypes(QualType LHS, QualType RHS, ArrayType::ArraySizeModifier(), 0); } case Type::FunctionNoProto: - return mergeFunctionTypes(LHS, RHS, OfBlockPointer, Unqualified); + return mergeFunctionTypes(LHS, RHS, OfBlockPointer, Unqualified, + /*AllowCXX=*/false, IsConditionalOperator); case Type::Record: case Type::Enum: return {}; diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 94f52004cf6c2..b14bd97eccb8a 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -8262,7 +8262,9 @@ static QualType checkConditionalPointerCompatibility(Sema &S, ExprResult &LHS, lhptee = S.Context.getQualifiedType(lhptee.getUnqualifiedType(), lhQual); rhptee = S.Context.getQualifiedType(rhptee.getUnqualifiedType(), rhQual); - QualType CompositeTy = S.Context.mergeTypes(lhptee, rhptee); + QualType CompositeTy = S.Context.mergeTypes( + lhptee, rhptee, /*OfBlockPointer=*/false, /*Unqualified=*/false, + /*BlockReturnType=*/false, /*IsConditionalOperator=*/true); if (CompositeTy.isNull()) { // In this situation, we assume void* type. No especially good diff --git a/clang/test/CodeGen/attr-noreturn.c b/clang/test/CodeGen/attr-noreturn.c index d6f21c61ba436..93816b7570e87 100644 --- a/clang/test/CodeGen/attr-noreturn.c +++ b/clang/test/CodeGen/attr-noreturn.c @@ -13,6 +13,7 @@ void __attribute__((noreturn)) f(void) { // CHECK-LABEL: @test_conditional_gnu( // CHECK: %cond = select i1 %tobool, ptr @t1, ptr @t2 // CHECK: call void %cond( +// CHECK: call void %cond2( // CHECK-NEXT: unreachable // CHECK-CXX-LABEL: @_Z20test_conditional_gnui( _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
