[clang] cf8fd21 - [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn
Fangrui Song via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 5 14:36:42 PST 2023
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(
More information about the cfe-commits
mailing list