[C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn

[MaskRay]

Summary

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]

[MaskRay]

use %itanium_abi_triple. remove -S

[rjmccall]

LGTM, but I have a suggested elaboration for the comment.

clang/lib/AST/ASTContext.cpp

10258

[MaskRay]

use @rjmccall's comment (thanks a lot!)

[aaron.ballman]

C2x clarified the behavior of *standard* attributes when determining a composite type, and this particular case really straddles that boundary. We support [[noreturn]] and _Noreturn as standard attributes in C as well as __attribute__((noreturn)) (as a nonstandard attribute with the same semantics as the standard attribute).

C2x 6.2.7p3 says, in part: "If one of the types has a standard attribute, the composite type also has that attribute." which is the current behavior before your patch. So this change introduces a regression for C2x for the standard attribute spelling but is fine for the GNU attribute spelling, but do we really want to have different answers depending on how the user spells the attribute? (FWIW, this change came in as part of the "unsequenced functions" attribute paper: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2956.htm).

[rjmccall]

Isn't the C feature not technically part of the type? I thought Clang was fairly unique in modeling noreturn the way we do.

[aaron.ballman]

In D140868#4024087, @rjmccall wrote:

Isn't the C feature not technically part of the type? I thought Clang was fairly unique in modeling noreturn the way we do.

That's true, I just verified I didn't change that as part of https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2764.pdf when doing [[noreturn]]. That makes this more reasonable than I was originally thinking, but it still makes me a bit uneasy. We form a composite type in two situations: needing a common type for ?: and when doing function redeclaration merging. It seems odd to me that we would treat those differently, but at the same time, the rationale for why ?: should be conservative makes a lot of sense to me.

What do GCC and ICC do in this case?

[ArsenArsen]

noreturn is a bit special among the attributes, perhaps it'd be worthwhile altering the standard to say that a composite is noreturn only of all of it's types are.

[ArsenArsen]

It'd appear that GCC merges them here:

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/c/c-typeck.cc;h=e06f052eb46a72d3d50835330c5af975e7c52084;hb=HEAD#l708

/* For function types do not merge const qualifiers, but drop them
   if used inconsistently.  The middle-end uses these to mark const
   and noreturn functions.  */
if (TREE_CODE (pointed_to_1) == FUNCTION_TYPE)
  target_quals = (quals1 & quals2);
else
  target_quals = (quals1 | quals2);

Volatile functions are considered noreturn, as a result, this makes target_quals only volatile if both types are.
This behavior seems better.
Note that the same applies for [[gnu::const]] by the same logic.

[MaskRay]

I am not a C language lawyer :) I wonder what else should be done to move this patch forward.
The https://github.com/llvm/llvm-project/issues/59792 has got some traction and has been added a candidate for the next 15.x patch release https://github.com/llvm/llvm-project/milestone/18

[thesamesam]

Confirmed this patch fixes the miscompile.

Right. While the spec side needs discussion (and almost certainly addressing!), I don't think anyone in the real world is expecting the Clang behaviour vs the GCC one. The Clang one as-is is the one that's less conservative, as aggressively imposing Noreturn can only lead to something breaking.

I don't have ICC available to test.

[rjmccall]

In D140868#4027480, @MaskRay wrote:

I am not a C language lawyer :) I wonder what else should be done to move this patch forward.
The https://github.com/llvm/llvm-project/issues/59792 has got some traction and has been added a candidate for the next 15.x patch release https://github.com/llvm/llvm-project/milestone/18

We should test that this doesn't affect functions declared using the standard C attributes. Otherwise, I think we can go forward with this change. Aaron, does that seem reasonable to you? For better or worse, the different spellings of noreturn do have different behavior already, since the standard spellings aren't supposed to be type-affecting. And I feel like the suggested rule here is clearly correct if we have the flexibility to set it on our own.

[MaskRay]

show that C23 [[noreturn]] codegen is unaffected.

The diff base improves attr-noreturn.c

[aaron.ballman]

In D140868#4027635, @rjmccall wrote:

In D140868#4027480, @MaskRay wrote:

I am not a C language lawyer :) I wonder what else should be done to move this patch forward.
The https://github.com/llvm/llvm-project/issues/59792 has got some traction and has been added a candidate for the next 15.x patch release https://github.com/llvm/llvm-project/milestone/18

We should test that this doesn't affect functions declared using the standard C attributes. Otherwise, I think we can go forward with this change. Aaron, does that seem reasonable to you? For better or worse, the different spellings of noreturn do have different behavior already, since the standard spellings aren't supposed to be type-affecting. And I feel like the suggested rule here is clearly correct if we have the flexibility to set it on our own.

Yeah, I think this is the right approach. I was worried about the inconsistency with function declaration merging, but [[noreturn]] is required to be on the first declaration (6.7.12.6p3) and so there's no merging issue there. _Noreturn does not have the same requirement but neither _Noreturn nor [[noreturn]] are part of the function type anyway. So yeah, this seems like the right way to go in this case, thank you!

Please add a release note for the changes.

[MaskRay]

add release note (this makes it slightly difficult to backport)

clang/include/clang/AST/ASTContext.h

bool canAssignObjCInterfacesInBlockPointer(
                                           const ObjCObjectPointerType *RHSOPT,
                                           bool BlockReturnType);
   bool areComparableObjCPointerTypes(QualType LHS, QualType RHS);
   QualType areCommonBaseCompatible(const ObjCObjectPointerType *LHSOPT,
                                    const ObjCObjectPointerType *RHSOPT);
   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);
   QualType mergeTransparentUnionType(QualType, QualType,
                                      bool OfBlockPointer=false,
                                      bool Unqualified = false);
 
   QualType mergeObjCGCQualifiers(QualType, QualType);

clang/lib/AST/ASTContext.cpp

QualType ASTContext::mergeFunctionParameterTypes(QualType lhs, QualType rhs,
   if (!rmerge.isNull())
     return rmerge;
 
   return mergeTypes(lhs, rhs, OfBlockPointer, Unqualified);
 }
 
 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);
   const auto *rproto = dyn_cast<FunctionProtoType>(rbase);
   bool allLTypes = true;
   bool allRTypes = true;
 
   // Check return type
QualType ASTContext::mergeFunctionTypes(QualType lhs, QualType rhs,
 
   if (lbaseInfo.getProducesResult() != rbaseInfo.getProducesResult())
     return {};
   if (lbaseInfo.getNoCallerSavedRegs() != rbaseInfo.getNoCallerSavedRegs())
     return {};
   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)
     allRTypes = false;
 
   FunctionType::ExtInfo einfo = lbaseInfo.withNoReturn(NoReturn);
 
   if (lproto && rproto) { // two C99 style function prototypes
static QualType mergeEnumWithInteger(ASTContext &Context, const EnumType *ET,
   // integral type of the same size.
   if (isBlockReturnType && other->isIntegerType() &&
       Context.getTypeSize(underlyingType) == Context.getTypeSize(other))
     return other;
 
   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.
   //
   // C++ [expr]: If an expression initially has the type "reference to T", the
   // type is adjusted to "T" prior to any further analysis, the expression
   // designates the object or function denoted by the reference, and the
   // expression is an lvalue unless the reference is an rvalue reference and
   // the expression is a function call (possibly inside parentheses).
if (RVAT) {
       return RHS;
     }
     if (getCanonicalType(LHSElem) == getCanonicalType(ResultType)) return LHS;
     if (getCanonicalType(RHSElem) == getCanonicalType(ResultType)) return RHS;
     return getIncompleteArrayType(ResultType,
                                   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 {};
   case Type::Builtin:
     // Only exactly equal builtin types are compatible, which is tested above.
     return {};
   case Type::Complex:
     // Distinct complex types are incompatible.

clang/lib/Sema/SemaExpr.cpp

static QualType checkConditionalPointerCompatibility(Sema &S, ExprResult &LHS,
   RHSCastKind =
       RAddrSpace == ResultAddrSpace ? CK_BitCast : CK_AddressSpaceConversion;
   lhQual.removeAddressSpace();
   rhQual.removeAddressSpace();
 
   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
     // reason, but this is what gcc does, and we do have to pick
     // to get a consistent AST.
     QualType incompatTy;
     incompatTy = S.Context.getPointerType(
         S.Context.getAddrSpaceQualType(S.Context.VoidTy, ResultAddrSpace));

clang/test/CodeGen/attr-noreturn.c

-// RUN: %clang_cc1 -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -x c++ -emit-llvm %s -o - | FileCheck %s --check-prefix=CHECK-CXX
 
 typedef void (*fptrs_t[4])(void);
 fptrs_t p __attribute__((noreturn));
 
 void __attribute__((noreturn)) f(void) {
   p[0]();
 }
 // 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);
+}