Block: Fix a crash when we have type attributes or qualifiers with omitted return type.

[manmanren]

Summary

A simple example will crash at IRGen:
void (^simpleBlock)() = ^ _Nonnull {

return;

};

The Return type for the block will be AttributedType of a DependentTy and it will not be resolved. We will get a crash at IRGen.
The fix is to warn when we have type attributes or qualifiers with omitted return type. The type attributes and qualifiers will be ignored.
This breaks test/SemaOpenCL/invalid-block.cl where it uses "^const(){}".
"int (^const bl3)() = ^const(){};" should emit error since we are converting between incompatible block pointer types, the block literal's return type should be void.

[rjmccall]

lib/Sema/SemaType.cpp

1569 ↗

(On Diff #51961)

It's not generally a good idea to set things as invalid if you're just emitting a warning. It might be different for parsed AttributeList objects, but... I'm not sure about that.

1609 ↗

(On Diff #51961)

Checking TypeQuals again here is redundant.

1611 ↗

(On Diff #51961)

You're missing at least TQ_restrict. But why make this an enumerated list at all?

[manmanren]

Cheers,
Manman

lib/Sema/SemaType.cpp

1569 ↗

(On Diff #51961)

Here we mark the AttributeList as invalid so when we call processTypeAttrs later, we will ignore these attributes, instead of creating AttributedType based on DependentTy for omitted block return type.

1609 ↗

(On Diff #51961)

You are right, I was following the code below this.

1611 ↗

(On Diff #51961)

I was trying to reuse diagnoseAndRemoveTypeQualifiers and didn't notice that it does not cover all the qualifiers.
I will rewrite this to not use diagnoseAndRemoveTypeQualifiers.

[Anastasia]

test/SemaOpenCL/invalid-block.cl

9 ↗

(On Diff #51961)

I think this test had a bug initially (multiple actually!). It was meant to initialize with int(^)() expression. Could you please add an int return so that this line has no error.

9 ↗

(On Diff #51961)

I am not getting why this error didn't appear before your change. How does your change trigger this error now, as you seem to be only changing the attributes and not the deduced block type...

14 ↗

(On Diff #51961)

The same here, line 7 and 16. Could we return int please.

32 ↗

(On Diff #51961)

could you remove second const please?

39 ↗

(On Diff #51961)

So the return type is deduced to be int for this block expression here?

[rjmccall]

lib/Sema/SemaType.cpp

1569 ↗

(On Diff #51961)

I'm just worried that there might be code which suppresses *error* diagnostics (or semantic analysis) when it sees an invalid attribute. Like I said, though, that might not be a problem for AttributeList.

test/SemaOpenCL/invalid-block.cl

9 ↗

(On Diff #51961)

Probably because of the unfortunate choice in block semantic analysis to use DependentTy as the marker for an unspecified block result type rather than some other placeholder type (like the undeduced-auto placeholder). This will cause the block to have a dependent type and basically disable a lot of downstream analysis if, as here, the placeholder is never actually replaced.

39 ↗

(On Diff #51961)

Yes. Block return types are deduced according to the types of the operands of the return statements within the block. The deduction rule is somewhat more permissive / complex than the lambda deduction rule.

[rsmith]

lib/Sema/SemaType.cpp

1569 ↗

(On Diff #51961)

Even if it didn't cause problems today, it's very likely to cause confusion and problems down the line. We should not have two different meanings for "invalid" for different kinds of AST nodes. Manman, can you remove the attribute(s) in question from the attribute list instead?

[manmanren]

Thanks for reviewing!

I will update the patch addressing the comments soon!

Manman

lib/Sema/SemaType.cpp

1569 ↗

(On Diff #51961)

John and Richard,

I got your point now. I didn't think about removing it from the attribute list before.

I looked at the interface for AttributeList, there is no interface for removing an item from the list, it has a setNext function.
But I found a helper function called spliceAttrOutOfList, I will try to use it.

[manmanren]

Addressing review comments.

[rsmith]

lib/Sema/SemaType.cpp

1253–1254 ↗

(On Diff #52316)

Instead of checking for qualifiers below, how about checking here (or, preferably, in isOmittedBlockReturnType) whether there were any present, and not treating the block as having an omitted return type if there actually is a return type present?

1561 ↗

(On Diff #52316)

Maybe write this as for (AttributeList *cur = attrs; cur; cur = cur->getNext()) and drop the cur = cur->getNext() assignments below.

1575–1583 ↗

(On Diff #52316)

You can express this more simply as:

if (prev)
  prev->setNext(cur->getNext());
else
  attrs = cur->getNext();
cur = cur->getNext();

[manmanren]

lib/Sema/SemaType.cpp

1253–1254 ↗

(On Diff #52316)

Makes sense to put related things together .

1561 ↗

(On Diff #52316)

Yes.

1575–1583 ↗

(On Diff #52316)

Yes.

test/SemaOpenCL/invalid-block.cl

8–9 ↗

(On Diff #52316)

Yes, John is right. The DependentTy is never replaced if wrapped with an Attributed type or a qualified type.

[Anastasia]

test/SemaOpenCL/invalid-block.cl

8–9 ↗

(On Diff #52316)

Ah, I see. And now the error occurs because you are invalidating/removing the attributes?

[manmanren]

Addressing review comments.

[manmanren]

Ping :]

[rjmccall]

Seems okay to me. Normally we wouldn't want to remove an attribute from the DeclSpec because it can apply to multiple declarators, but that's not possible with a block-literal declarator, so it's fine. Richard should sign off, though.

[Anastasia]

LGTM!

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_objc_type_arg_does_not_match_bound : Error<
   "type argument %0 does not satisfy the bound (%1) of type parameter %2">;
 
 def warn_objc_redundant_qualified_class_type : Warning<
   "parameterized class %0 already conforms to the protocols listed; did you "
   "forget a '*'?">, InGroup<ObjCProtocolQualifiers>;
 
+def warn_block_literal_attributes_on_omitted_return_type : Warning<
+  "attribute %0 ignored, because it cannot be applied to omitted return type">,
+  InGroup<IgnoredAttributes>;
+
+def warn_block_literal_qualifiers_on_omitted_return_type : Warning<
+  "'%0' qualifier on omitted return type %1 has no effect">,
+  InGroup<IgnoredQualifiers>;
+
 } // end of sema component.

cfe/trunk/lib/Sema/SemaType.cpp

static void diagnoseAndRemoveTypeQualifiers(Sema &S, const DeclSpec &DS,
                                             unsigned &TypeQuals,
                                             QualType TypeSoFar,
                                             unsigned RemoveTQs,
                                             unsigned DiagID) {
   // If this occurs outside a template instantiation, warn the user about
   // it; they probably didn't mean to specify a redundant qualifier.
   typedef std::pair<DeclSpec::TQ, SourceLocation> QualLoc;
   for (QualLoc Qual : {QualLoc(DeclSpec::TQ_const, DS.getConstSpecLoc()),
+                       QualLoc(DeclSpec::TQ_restrict, DS.getRestrictSpecLoc()),
                        QualLoc(DeclSpec::TQ_volatile, DS.getVolatileSpecLoc()),
                        QualLoc(DeclSpec::TQ_atomic, DS.getAtomicSpecLoc())}) {
     if (!(RemoveTQs & Qual.first))
       continue;
 
     if (S.ActiveTemplateInstantiations.empty()) {
       if (TypeQuals & Qual.first)
         S.Diag(Qual.second, DiagID)
           << DeclSpec::getSpecifierName(Qual.first) << TypeSoFar
           << FixItHint::CreateRemoval(Qual.second);
     }
 
     TypeQuals &= ~Qual.first;
   }
 }
 
+/// Return true if this is omitted block return type. Also check type
+/// attributes and type qualifiers when returning true.
+static bool checkOmittedBlockReturnType(Sema &S, Declarator &declarator,
+                                        QualType Result) {
+  if (!isOmittedBlockReturnType(declarator))
+    return false;
+
+  // Warn if we see type attributes for omitted return type on a block literal.
+  AttributeList *&attrs =
+      declarator.getMutableDeclSpec().getAttributes().getListRef();
+  AttributeList *prev = nullptr;
+  for (AttributeList *cur = attrs; cur; cur = cur->getNext()) {
+    AttributeList &attr = *cur;
+    // Skip attributes that were marked to be invalid or non-type
+    // attributes.
+    if (attr.isInvalid() || !attr.isTypeAttr()) {
+      prev = cur;
+      continue;
+    }
+    S.Diag(attr.getLoc(),
+           diag::warn_block_literal_attributes_on_omitted_return_type)
+        << attr.getName();
+    // Remove cur from the list.
+    if (prev) {
+      prev->setNext(cur->getNext());
+      prev = cur;
+    } else {
+      attrs = cur->getNext();
+    }
+  }
+
+  // Warn if we see type qualifiers for omitted return type on a block literal.
+  const DeclSpec &DS = declarator.getDeclSpec();
+  unsigned TypeQuals = DS.getTypeQualifiers();
+  diagnoseAndRemoveTypeQualifiers(S, DS, TypeQuals, Result, (unsigned)-1,
+      diag::warn_block_literal_qualifiers_on_omitted_return_type);
+  declarator.getMutableDeclSpec().ClearTypeQualifiers();
+
+  return true;
+}
+
 /// Apply Objective-C type arguments to the given type.
 static QualType applyObjCTypeArgs(Sema &S, SourceLocation loc, QualType type,
                                   ArrayRef<TypeSourceInfo *> typeArgs,
                                   SourceRange typeArgsRange,
                                   bool failOnError = false) {
   // We can only apply type arguments to an Objective-C class type.
   const auto *objcObjectType = type->getAs<ObjCObjectType>();
   if (!objcObjectType || !objcObjectType->getInterface()) {
case DeclSpec::TST_unspecified:
     // The declspec is always missing in a lambda expr context; it is either
     // specified with a trailing return type or inferred.
     if (S.getLangOpts().CPlusPlus14 &&
         declarator.getContext() == Declarator::LambdaExprContext) {
       // In C++1y, a lambda's implicit return type is 'auto'.
       Result = Context.getAutoDeductType();
       break;
     } else if (declarator.getContext() == Declarator::LambdaExprContext ||
-               isOmittedBlockReturnType(declarator)) {
+               checkOmittedBlockReturnType(S, declarator,
+                                           Context.DependentTy)) {
       Result = Context.DependentTy;
       break;
     }
 
     // Unspecified typespec defaults to int in C90.  However, the C90 grammar
     // [C90 6.5] only allows a decl-spec if there was *some* type-specifier,
     // type-qualifier, or storage-class-specifier.  If not, emit an extwarn.
     // Note that the one exception to this is function definitions, which are

cfe/trunk/test/SemaObjC/block-omitted-return-type.m

+// RUN: %clang_cc1 %s -fblocks -verify -fsyntax-only
+
+@interface NSObject
+@end
+
+@interface Test : NSObject
+- (void)test;
+@end
+
+@implementation Test
+- (void)test
+{
+  void (^simpleBlock)() = ^ _Nonnull { //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+    return;
+  };
+  void (^simpleBlock2)() = ^ _Nonnull void { //expected-error {{nullability specifier '_Nonnull' cannot be applied to non-pointer type 'void'}}
+    return;
+  };
+  void (^simpleBlock3)() = ^ _Nonnull (void) {  //expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}}
+    return;
+  };
+
+  void (^simpleBlock4)() = ^ const { //expected-warning {{'const' qualifier on omitted return type '<dependent type>' has no effect}}
+    return;
+  };
+  void (^simpleBlock5)() = ^ const void { //expected-error {{incompatible block pointer types initializing 'void (^)()' with an expression of type 'const void (^)(void)'}}
+    return;
+  };
+  void (^simpleBlock6)() = ^ const (void) { //expected-warning {{'const' qualifier on omitted return type '<dependent type>' has no effect}}
+    return;
+  };
+  void (^simpleBlock7)() = ^ _Nonnull __attribute__((align_value(128))) _Nullable const (void) { // expected-warning {{attribute '_Nullable' ignored, because it cannot be applied to omitted return type}} \
+    // expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}} \
+    // expected-warning {{'const' qualifier on omitted return type '<dependent type>' has no effect}} \
+    // expected-warning {{'align_value' attribute only applies to variables and typedefs}}
+    return;
+  };
+  void (^simpleBlock9)() = ^ __attribute__ ((align_value(128))) _Nonnull const (void) { // expected-warning {{attribute '_Nonnull' ignored, because it cannot be applied to omitted return type}} \
+    // expected-warning {{'const' qualifier on omitted return type '<dependent type>' has no effect}} \
+    // expected-warning {{'align_value' attribute only applies to variables and typedefs}}
+    return;
+  };
+}
+@end

cfe/trunk/test/SemaOpenCL/invalid-block.cl

 // RUN: %clang_cc1 -verify -fblocks -cl-std=CL2.0 %s
 
 // OpenCL v2.0 s6.12.5
 
 // All blocks declarations must be const qualified and initialized.
 void f1() {
-  int (^bl1)() = ^() {}; // expected-error{{invalid block variable declaration - must be const qualified}}
+  int (^bl1)() = ^() {return 1;}; // expected-error{{invalid block variable declaration - must be const qualified}}
   int (^const bl2)(); // expected-error{{invalid block variable declaration - must be initialized}}
-  int (^const bl3)() = ^const(){
-  };
+  int (^const bl3)() = ^(){return 1;};
 }
 
 // A block with extern storage class is not allowed.
-extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 void f2() {
-  extern int (^const bl)() = ^const(){}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
+  extern int (^const bl)() = ^(){return 1;}; // expected-error{{invalid block variable declaration - using 'extern' storage class is disallowed}}
 }
 
 // A block cannot be the return value of a function.
 typedef int (^const bl_t)(void);
 bl_t f3(bl_t bl); // expected-error{{declaring function return value of type 'bl_t' (aka 'int (^const)(void)') is not allowed}}
 
 struct bl_s {
   int (^const bl)(void); // expected-error {{the 'int (^const)(void)' type cannot be used to declare a structure or union field}}
 };
 
 void f4() {
   __block int a = 10; // expected-error {{the __block storage type is not permitted}}
 }
 
 // A block with variadic argument is not allowed.
-int (^const bl)(int, ...) = ^const int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
+int (^const bl)(int, ...) = ^int(int I, ...) { // expected-error {{invalid block prototype, variadic arguments are not allowed in OpenCL}}
   return 0;
 };
 
 // A block can't be used to declare an array
 typedef int (^const bl1_t)(int);
 void f5(int i) {
-  bl1_t bl1 = ^const(int i) {return 1;};
-  bl1_t bl2 = ^const(int i) {return 2;};
+  bl1_t bl1 = ^(int i) {return 1;};
+  bl1_t bl2 = ^(int i) {return 2;};
   bl1_t arr[] = {bl1, bl2}; // expected-error {{array of 'bl1_t' (aka 'int (^const)(int)') type is invalid in OpenCL}}
   int tmp = i ? bl1(i)      // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
               : bl2(i);     // expected-error {{block type cannot be used as expression in ternary expression in OpenCL}}
 }
 
 void f6(bl1_t * bl_ptr) {
-  bl1_t bl = ^const(int i) {return 1;};
+  bl1_t bl = ^(int i) {return 1;};
   bl1_t *p = &bl; // expected-error {{invalid argument type 'bl1_t' (aka 'int (^const)(int)') to unary expression}}
   bl = *bl_ptr;  // expected-error {{dereferencing pointer of type '__generic bl1_t *' (aka 'int (^const __generic *)(int)') is not allowed in OpenCL}}
 }