[cfe-dev] assert when vector variable is used as the size of array

[Douglas Gregor]

On Jul 22, 2010, at 12:30 AM, Anton Yartsev wrote:

> On 22.07.2010 0:30, Douglas Gregor wrote:
>> On Jul 20, 2010, at 10:39 PM, Anton Yartsev wrote:
>> 
>>   
>>> 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..
>>>     
>> 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
>> 
>>   
> 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.


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
===================================================================
--- lib/AST/ASTContext.cpp	(revision 109088)
+++ lib/AST/ASTContext.cpp	(working copy)
@@ -1570,10 +1570,7 @@
   // If the element type isn't canonical, this won't be a canonical type either,
   // so fill in the canonical type field.
   QualType Canonical;
-  if (!vecType.isCanonical() || (AltiVecSpec == VectorType::AltiVec)) {
-    // pass VectorType::NotAltiVec for AltiVecSpec to make AltiVec canonical
-    // vector type (except 'vector bool ...' and 'vector Pixel') the same as
-    // the equivalent GCC vector types
+  if (!vecType.isCanonical()) {
     Canonical = getVectorType(getCanonicalType(vecType), NumElts,
       VectorType::NotAltiVec);
 
@@ -4200,6 +4197,28 @@

          LHS->getNumElements() == RHS->getNumElements();
 }
 
+bool ASTContext::areCompatibleVectorTypes(QualType FirstVec,
+                                          QualType SecondVec) {
+
+  assert(FirstVec->isVectorType() && SecondVec->isVectorType());
+
+  if (hasSameUnqualifiedType(FirstVec, SecondVec))
+    return true;
+
+  // AltiVec vectors types are identical to equivalent GCC vector types
+  const VectorType *First = FirstVec->getAs<VectorType>();
+  const VectorType *Second = SecondVec->getAs<VectorType>();
+  if ((((First->getAltiVecSpecific() == VectorType::AltiVec) &&
+        (Second->getAltiVecSpecific() == VectorType::NotAltiVec)) ||
+       ((First->getAltiVecSpecific() == VectorType::NotAltiVec) &&
+        (Second->getAltiVecSpecific() == VectorType::AltiVec))) &&
+      (First->getElementType() == Second->getElementType()) &&
+      (First->getNumElements() == Second->getNumElements()))
+    return true;
+
+  return false;
+}
+

I've not committed this yet; I'll review your vector bool/vector pixel patch separately.

I also added forwarding functions for the new has(Signed|Unsigned|)IntegerRepresentation functions into CanonicalType.h.

This is great work, thank you!

	- Doug