MaskRay created this revision.
MaskRay added reviewers: aaron.ballman, mizvekov, rjmccall.
Herald added a subscriber: StephenFan.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
In C mode, if e1 is noreturn but e2 isn'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.
Fix https://github.com/llvm/llvm-project/issues/59792 [1]
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D140868
Files:
clang/include/clang/AST/ASTContext.h
clang/lib/AST/ASTContext.cpp
clang/lib/Sema/SemaExpr.cpp
clang/test/CodeGen/attr-noreturn.c
Index: clang/test/CodeGen/attr-noreturn.c
===================================================================
--- clang/test/CodeGen/attr-noreturn.c
+++ clang/test/CodeGen/attr-noreturn.c
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64 -x c++ -S -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CXX
typedef void (*fptrs_t[4])(void);
fptrs_t p __attribute__((noreturn));
@@ -8,3 +9,24 @@
}
// CHECK: call void
// CHECK-NEXT: unreachable
+
+// CHECK-LABEL: @test_conditional(
+// CHECK: %cond = select i1 %tobool, ptr @t1, ptr @t2
+// CHECK: call void %cond(
+// CHECK: call void %cond2(
+// CHECK-NEXT: unreachable
+
+// CHECK-CXX-LABEL: @_Z16test_conditionali(
+// CHECK-CXX: %cond{{.*}} = phi ptr [ @_Z2t1i, %{{.*}} ], [ @_Z2t2i, %{{.*}} ]
+// CHECK-CXX: call void %cond{{.*}}(
+// CHECK-CXX: %cond{{.*}} = phi ptr [ @_Z2t1i, %{{.*}} ], [ @_Z2t1i, %{{.*}} ]
+// CHECK-CXX: call void %cond{{.*}}(
+// CHECK-CXX-NEXT: unreachable
+void t1(int) __attribute__((noreturn));
+void t2(int);
+__attribute__((noreturn)) void test_conditional(int a) {
+ // The conditional operator isn't noreturn because t2 isn't.
+ (a ? t1 : t2)(a);
+ // The conditional operator is noreturn.
+ (a ? t1 : t1)(a);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===================================================================
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -8262,7 +8262,9 @@
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
Index: clang/lib/AST/ASTContext.cpp
===================================================================
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -10191,7 +10191,8 @@
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,11 @@
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();
-
+ // For ConditionalOperator, the result type is noreturn if both operands are
+ // noreturn.
+ bool NoReturn = IsConditionalOperator
+ ? lbaseInfo.getNoReturn() && rbaseInfo.getNoReturn()
+ : lbaseInfo.getNoReturn() || rbaseInfo.getNoReturn();
if (lbaseInfo.getNoReturn() != NoReturn)
allLTypes = false;
if (rbaseInfo.getNoReturn() != NoReturn)
@@ -10389,9 +10392,9 @@
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 +10687,8 @@
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 {};
Index: clang/include/clang/AST/ASTContext.h
===================================================================
--- clang/include/clang/AST/ASTContext.h
+++ clang/include/clang/AST/ASTContext.h
@@ -2865,10 +2865,12 @@
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);
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
