This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by manmanren on Mar 29 2016, 11:54 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

manmanren updated this revision to Diff 51961.Mar 29 2016, 11:54 AM
manmanren retitled this revision from to Block: Fix a crash when we have type attributes or qualifiers with omitted return type..
manmanren updated this object.
manmanren added reviewers: rjmccall, Anastasia.
manmanren added a subscriber: cfe-commits.
rjmccall added inline comments.Mar 29 2016, 1:25 PM
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?

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 added inline comments.Mar 30 2016, 9:22 AM
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 added inline comments.Mar 31 2016, 9:45 AM
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 added a subscriber: rsmith.Mar 31 2016, 12:31 PM
rsmith added inline comments.
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?

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 updated this revision to Diff 52316.Mar 31 2016, 5:34 PM

Addressing review comments.

rsmith added inline comments.Mar 31 2016, 5:59 PM
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 marked 3 inline comments as done.Mar 31 2016, 9:30 PM
manmanren added inline comments.
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 added inline comments.Apr 1 2016, 9:29 AM
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 updated this revision to Diff 52398.Apr 1 2016, 11:09 AM

Addressing review comments.

rjmccall edited edge metadata.Apr 11 2016, 5:54 PM

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 accepted this revision.Apr 15 2016, 4:28 AM
Anastasia edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Apr 15 2016, 4:28 AM
This revision was automatically updated to reflect the committed changes.