OpenCL: Improve vector printf warnings

The vector modifier is considered separate, so
don't treat it as a conversion specifier.

This is still not warning on some cases, like
using a type that isn't a valid vector element.

Fixes bug 39652

llvm-svn: 348084

clang/include/clang/AST/FormatString.h

@@ -166,8 +166,6 @@ public:
 
     ZArg, // MS extension
 
-    VArg, // OpenCL vectors
-
     // Objective-C specific specifiers.
     ObjCObjArg, // '@'
     ObjCBeg = ObjCObjArg,
@@ -305,6 +303,8 @@ public:
 
   QualType getRepresentativeType(ASTContext &C) const;
 
+  ArgType makeVectorType(ASTContext &C, unsigned NumElts) const;
+
   std::string getRepresentativeTypeName(ASTContext &C) const;
 };
 
@@ -324,6 +324,10 @@ public:
   : start(nullptr),length(0), hs(valid ? NotSpecified : Invalid), amt(0),
   UsesPositionalArg(0), UsesDotPrefix(0) {}
 
+  explicit OptionalAmount(unsigned Amount)
+    : start(nullptr), length(0), hs(Constant), amt(Amount),
+    UsesPositionalArg(false), UsesDotPrefix(false) {}
+
   bool isInvalid() const {
     return hs == Invalid;
   }
@@ -381,6 +385,8 @@ protected:
   LengthModifier LM;
   OptionalAmount FieldWidth;
   ConversionSpecifier CS;
+  OptionalAmount VectorNumElts;
+
   /// Positional arguments, an IEEE extension:
   ///  IEEE Std 1003.1, 2004 Edition
   ///  http://www.opengroup.org/onlinepubs/009695399/functions/printf.html
@@ -388,7 +394,8 @@ protected:
   unsigned argIndex;
 public:
   FormatSpecifier(bool isPrintf)
-    : CS(isPrintf), UsesPositionalArg(false), argIndex(0) {}
+    : CS(isPrintf), VectorNumElts(false),
+      UsesPositionalArg(false), argIndex(0) {}
 
   void setLengthModifier(LengthModifier lm) {
     LM = lm;
@@ -416,6 +423,14 @@ public:
     return FieldWidth;
   }
 
+  void setVectorNumElts(const OptionalAmount &Amt) {
+    VectorNumElts = Amt;
+  }
+
+  const OptionalAmount &getVectorNumElts() const {
+    return VectorNumElts;
+  }
+
   void setFieldWidth(const OptionalAmount &Amt) {
     FieldWidth = Amt;
   }
@@ -480,6 +495,9 @@ class PrintfSpecifier : public analyze_format_string::FormatSpecifier {
   OptionalFlag IsSensitive;          // '{sensitive}'
   OptionalAmount Precision;
   StringRef MaskType;
+
+  ArgType getScalarArgType(ASTContext &Ctx, bool IsObjCLiteral) const;
+
 public:
   PrintfSpecifier()
       : FormatSpecifier(/* isPrintf = */ true), HasThousandsGrouping("'"),

clang/lib/AST/FormatString.cpp

@@ -178,6 +178,36 @@ clang::analyze_format_string::ParseArgPosition(FormatStringHandler &H,
   return false;
 }
 
+bool
+clang::analyze_format_string::ParseVectorModifier(FormatStringHandler &H,
+                                                  FormatSpecifier &FS,
+                                                  const char *&I,
+                                                  const char *E,
+                                                  const LangOptions &LO) {
+  if (!LO.OpenCL)
+    return false;
+
+  const char *Start = I;
+  if (*I == 'v') {
+    ++I;
+
+    if (I == E) {
+      H.HandleIncompleteSpecifier(Start, E - Start);
+      return true;
+    }
+
+    OptionalAmount NumElts = ParseAmount(I, E);
+    if (NumElts.getHowSpecified() != OptionalAmount::Constant) {
+      H.HandleIncompleteSpecifier(Start, E - Start);
+      return true;
+    }
+
+    FS.setVectorNumElts(NumElts);
+  }
+
+  return false;
+}
+
 bool
 clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,
                                                   const char *&I,
@@ -457,6 +487,14 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
   llvm_unreachable("Invalid ArgType Kind!");
 }
 
+ArgType ArgType::makeVectorType(ASTContext &C, unsigned NumElts) const {
+  if (K != SpecificTy) // Won't be a valid vector element type.
+    return ArgType::Invalid();
+
+  QualType Vec = C.getExtVectorType(T, NumElts);
+  return ArgType(Vec, Name);
+}
+
 QualType ArgType::getRepresentativeType(ASTContext &C) const {
   QualType Res;
   switch (K) {
@@ -618,9 +656,6 @@ const char *ConversionSpecifier::toString() const {
 
   // MS specific specifiers.
   case ZArg: return "Z";
-
- // OpenCL specific specifiers.
-  case VArg: return "v";
   }
   return nullptr;
 }
@@ -878,8 +913,6 @@ bool FormatSpecifier::hasStandardConversionSpecifier(
     case ConversionSpecifier::CArg:
     case ConversionSpecifier::SArg:
       return LangOpt.ObjC;
-    case ConversionSpecifier::VArg:
-      return LangOpt.OpenCL;
     case ConversionSpecifier::InvalidSpecifier:
     case ConversionSpecifier::FreeBSDbArg:
     case ConversionSpecifier::FreeBSDDArg:

clang/lib/AST/FormatStringParsing.h

@@ -41,6 +41,10 @@ bool ParseArgPosition(FormatStringHandler &H,
                       FormatSpecifier &CS, const char *Start,
                       const char *&Beg, const char *E);
 
+bool ParseVectorModifier(FormatStringHandler &H,
+                         FormatSpecifier &FS, const char *&Beg, const char *E,
+                         const LangOptions &LO);
+
 /// Returns true if a LengthModifier was parsed and installed in the
 /// FormatSpecifier& argument, and false otherwise.
 bool ParseLengthModifier(FormatSpecifier &FS, const char *&Beg, const char *E,

clang/lib/AST/PrintfFormatString.cpp

@@ -247,6 +247,9 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
     }
   }
 
+  if (ParseVectorModifier(H, FS, I, E, LO))
+    return true;
+
   // Look for the length modifier.
   if (ParseLengthModifier(FS, I, E, LO) && I == E) {
     // No more characters left?
@@ -363,11 +366,6 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
       if (Target.getTriple().isOSMSVCRT())
         k = ConversionSpecifier::ZArg;
       break;
-    // OpenCL specific.
-    case 'v':
-      if (LO.OpenCL)
-        k = ConversionSpecifier::VArg;
-      break;
   }
 
   // Check to see if we used the Objective-C modifier flags with
@@ -466,13 +464,8 @@ bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I,
 // Methods on PrintfSpecifier.
 //===----------------------------------------------------------------------===//
 
-ArgType PrintfSpecifier::getArgType(ASTContext &Ctx,
+ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
-                                    bool IsObjCLiteral) const {
+                                          bool IsObjCLiteral) const {
-  const PrintfConversionSpecifier &CS = getConversionSpecifier();
-
-  if (!CS.consumesDataArgument())
-    return ArgType::Invalid();
-
   if (CS.getKind() == ConversionSpecifier::cArg)
     switch (LM.getKind()) {
       case LengthModifier::None:
@@ -632,6 +625,21 @@ ArgType PrintfSpecifier::getArgType(ASTContext &Ctx,
   return ArgType();
 }
 
+
+ArgType PrintfSpecifier::getArgType(ASTContext &Ctx,
+                                    bool IsObjCLiteral) const {
+  const PrintfConversionSpecifier &CS = getConversionSpecifier();
+
+  if (!CS.consumesDataArgument())
+    return ArgType::Invalid();
+
+  ArgType ScalarTy = getScalarArgType(Ctx, IsObjCLiteral);
+  if (!ScalarTy.isValid() || VectorNumElts.isInvalid())
+    return ScalarTy;
+
+  return ScalarTy.makeVectorType(Ctx, VectorNumElts.getConstantAmount());
+}
+
 bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
                               ASTContext &Ctx, bool IsObjCLiteral) {
   // %n is different from other conversion specifiers; don't try to fix it.
@@ -681,8 +689,17 @@ bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
   if (const EnumType *ETy = QT->getAs<EnumType>())
     QT = ETy->getDecl()->getIntegerType();
 
-  // We can only work with builtin types.
   const BuiltinType *BT = QT->getAs<BuiltinType>();
+  if (!BT) {
+    const VectorType *VT = QT->getAs<VectorType>();
+    if (VT) {
+      QT = VT->getElementType();
+      BT = QT->getAs<BuiltinType>();
+      VectorNumElts = OptionalAmount(VT->getNumElements());
+    }
+  }
+
+  // We can only work with builtin types.
   if (!BT)
     return false;
 
@@ -854,6 +871,11 @@ void PrintfSpecifier::toString(raw_ostream &os) const {
   FieldWidth.toString(os);
   // Precision
   Precision.toString(os);
+
+  // Vector modifier
+  if (!VectorNumElts.isInvalid())
+    os << 'v' << VectorNumElts.getConstantAmount();
+
   // Length modifier
   os << LM.toString();
   // Conversion specifier
@@ -1032,7 +1054,6 @@ bool PrintfSpecifier::hasValidPrecision() const {
   case ConversionSpecifier::FreeBSDrArg:
   case ConversionSpecifier::FreeBSDyArg:
   case ConversionSpecifier::PArg:
-  case ConversionSpecifier::VArg:
     return true;
 
   default:

clang/test/SemaOpenCL/format-strings-fixit.cl

@@ -0,0 +1,24 @@
+// RUN: cp %s %t
+// RUN: %clang_cc1 -cl-std=CL1.2 -pedantic -Wall -fixit %t
+// RUN: %clang_cc1 -cl-std=CL1.2 -fsyntax-only -pedantic -Wall -Werror %t
+// RUN: %clang_cc1 -cl-std=CL1.2 -E -o - %t | FileCheck %s
+
+typedef __attribute__((ext_vector_type(4))) int int4;
+typedef __attribute__((ext_vector_type(8))) int int8;
+
+int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
+
+
+void vector_fixits() {
+  printf("%v4f", (int4) 123);
+  // CHECK: printf("%v4d", (int4) 123);
+
+  printf("%v8d", (int4) 123);
+  // CHECK: printf("%v4d", (int4) 123);
+
+  printf("%v4d", (int8) 123);
+  // CHECK: printf("%v8d", (int8) 123);
+
+  printf("%v4f", (int8) 123);
+  // CHECK: printf("%v8d", (int8) 123);
+}

clang/test/SemaOpenCL/printf-format-strings.cl

@@ -1,34 +1,94 @@
-// RUN: %clang_cc1 -cl-std=CL1.2 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=+cl_khr_fp64 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -cl-std=CL1.2 -cl-ext=-cl_khr_fp64 -fsyntax-only -verify %s
 
 typedef __attribute__((ext_vector_type(2))) float float2;
 typedef __attribute__((ext_vector_type(4))) float float4;
+
+typedef __attribute__((ext_vector_type(2))) int int2;
 typedef __attribute__((ext_vector_type(4))) int int4;
+typedef __attribute__((ext_vector_type(16))) int int16;
 
 int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
 
 kernel void format_v4f32(float4 arg)
 {
-    printf("%v4f\n", arg); // expected-no-diagnostics
+#ifdef cl_khr_fp64
+    printf("%v4f\n", arg);
+
+    // Precision modifier
+    printf("%.2v4f\n", arg);
+#else
+    // FIXME: These should not warn, and the type should be expected to be float.
+    printf("%v4f\n", arg);  // expected-warning {{double __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
+
+    // Precision modifier
+    printf("%.2v4f\n", arg); // expected-warning {{double __attribute__((ext_vector_type(4)))' but the argument has type 'float4' (vector of 4 'float' values)}}
+#endif
+}
+
+kernel void format_only_v(int arg)
+{
+    printf("%v", arg); // expected-warning {{incomplete format specifier}}
+}
+
+kernel void format_missing_num(int arg)
+{
+    printf("%v4", arg); // expected-warning {{incomplete format specifier}}
+}
+
+kernel void format_not_num(int arg)
+{
+    printf("%vNd", arg); // expected-warning {{incomplete format specifier}}
+    printf("%v*d", arg); // expected-warning {{incomplete format specifier}}
+}
+
+kernel void format_v16i32(int16 arg)
+{
+    printf("%v16d\n", arg);
+}
+
+kernel void format_v4i32_scalar(int arg)
+{
+   printf("%v4d\n", arg); // expected-warning  {{format specifies type 'int __attribute__((ext_vector_type(4)))' but the argument has type 'int'}}
+}
+
+kernel void format_v4i32_wrong_num_elts_2_to_4(int2 arg)
+{
+    printf("%v4d\n", arg); // expected-warning {{format specifies type 'int __attribute__((ext_vector_type(4)))' but the argument has type 'int2' (vector of 2 'int' values)}}
+}
+
+kernel void format_missing_num_elts_format(int4 arg)
+{
+    printf("%vd\n", arg); // expected-warning {{incomplete format specifier}}
+}
+
+kernel void format_v4f32_scalar(float arg)
+{
+    printf("%v4f\n", arg); // expected-warning {{format specifies type 'double __attribute__((ext_vector_type(4)))' but the argument has type 'float'}}
 }
 
 kernel void format_v4f32_wrong_num_elts(float2 arg)
 {
-    printf("%v4f\n", arg); // expected-no-diagnostics
+    printf("%v4f\n", arg); // expected-warning {{format specifies type 'double __attribute__((ext_vector_type(4)))' but the argument has type 'float2' (vector of 2 'float' values)}}
 }
 
-kernel void vector_precision_modifier_v4f32(float4 arg)
-{
-    printf("%.2v4f\n", arg); // expected-no-diagnostics
-}
-
-// FIXME: This should warn
 kernel void format_missing_num_elts(float4 arg)
 {
-    printf("%vf\n", arg); // expected-no-diagnostics
+    printf("%vf\n", arg); // expected-warning {{incomplete format specifier}}
+}
+
+kernel void vector_precision_modifier_v4i32_to_v4f32(int4 arg)
+{
+    printf("%.2v4f\n", arg); // expected-warning {{format specifies type 'double __attribute__((ext_vector_type(4)))' but the argument has type 'int4' (vector of 4 'int' values)}}
+}
+
+kernel void invalid_Y(int4 arg)
+{
+    printf("%v4Y\n", arg); // expected-warning {{invalid conversion specifier 'Y'}}
 }
 
 // FIXME: This should warn
-kernel void vector_precision_modifier_v4i32(int4 arg)
+kernel void crash_on_s(int4 arg)
 {
-    printf("%.2v4f\n", arg); // expected-no-diagnostics
+    printf("%v4s\n", arg);
 }