assert when vector variable is used as the size of array

Clang Frontend
1

Hi all,

I accidentally stumbled upon the fact that situation when a vector variable is used as the size of array is not handled. Compiling the code

void f()
{
   int __attribute__ ((vector_size (16))) a;
   int b[a];
}

ends with

Assertion failed: castIsValid(getOpcode(), S, Ty) && "Illegal BitCast", file Instructions.cpp, line 2654

While investigating the problem I found out that methods Type::isIntegerType(), Type::isSignedIntegerType() and Type::isUnsignedIntegerType() (.\lib\AST\Type.cpp ) return 'true' for vector types with corresponding element types. It looks strange as vector types are not treated as integer types.
I made two patches: the first one - vector_size_of_array.patch - removes code for vectors from the above methods and adds additional methods instead - Type::hasIntegerRepresentation(), Type::hasSignedIntRepresentation() and Type::hasUnsignedIntRepresentation() - by analogy with Type::isFloatingType() and Type::hasFloatingRepresentation(). Calls to old methods were replaced with calls to new ones where it was needed (at least no regression bugs). The patch solves the subject. It also make Clang sliiiightly faster..
The second patch - vector_size_of_array_SIMPLE.patch - only solves the subject.
Please review and comment.

vector_size_of_array.patch (6.97 KB)

vector_size_of_array_SIMPLE.patch (639 Bytes)

2

Your patch looks great. I had meant to introduce hasIntegerRepresentation et al when I added hasFloatingRepresentation, but ran out of time. Only a few comments on the patch:

  1) I'd rather spell out "Integer" in the names of these functions, e.g., Type::hasSignedIntegerRepresentation(). Sure, it's long, but it's also more consistent.
  2) I was surprised at how few places in Sema and CodeGen had to change from is(Signed|Unsigned|)IntegerType to has(Signed|Unsigned|)Representation; did you audit all of the callers of the "is(Signed|Unsigned|)IntegerType" functions?
  3) Could you a test case that verifies that we reject the code above?

- Doug

3

I replaced functions until test results became the same as before changes.. Now I have looked through all the occurrences, hope that fixed all of them. Attached is the updated patch and the test.

vector_size_of_array.patch (14.4 KB)

vector_as_size_of_array.c (226 Bytes)

4

Wonderful! Here's some feedback on the patch, which I've committed here:

  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20100719/032490.html

With a few changes/additions from my own pass through the is(Signed|Unsigned|)IntegerType calls. It's quite amusing how many latent bugs this fixes... I added a few more in a new test case (test/Sema/vector-ops.c), which also incorporates your test case.

Index: lib/Sema/SemaExpr.cpp
@@ -5781,8 +5783,10 @@
   // Diagnose cases where the user write a logical and/or but probably meant a
   // bitwise one. We do this when the LHS is a non-bool integer and the RHS
   // is a constant.
- if (lex->getType()->isIntegerType() && !lex->getType()->isBooleanType() &&
- rex->getType()->isIntegerType() && rex->isEvaluatable(Context) &&
+ if (lex->getType()->hasIntegerRepresentation() &&
+ !lex->getType()->isBooleanType() &&
+ rex->getType()->hasIntegerRepresentation() &&
+ rex->isEvaluatable(Context) &&
       // Don't warn if the RHS is a (constant folded) boolean expression like
       // "sizeof(int) == 4".
       !rex->isKnownToHaveBooleanValue() &&

I don't think we want to trigger this warning for vector &/&& or |/||.

Index: lib/AST/ASTContext.cpp