OpenCL: Improve vector printf warnings

[arsenm]

Summary

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.

[Anastasia]

test/SemaOpenCL/format-strings-fixit.cl

14

Does this not work yet?

test/SemaOpenCL/printf-format-strings.cl

77

So there is no way to print vector of float? What will happen on architectures that don't support doubles?

I guess it's the same for printf in general with the float type?

[arsenm]

test/SemaOpenCL/format-strings-fixit.cl

14

It does. This was a leftover reference I was using I can remove

test/SemaOpenCL/printf-format-strings.cl

77

There is, it's converted to a vector of doubles. This case warns because the element count mismatches

[arsenm]

test/SemaOpenCL/printf-format-strings.cl

77

The conversion is to float if doubles aren't supported

[arsenm]

test/SemaOpenCL/printf-format-strings.cl

77

The warning message is wrong though without doubles, and still says double.

[Anastasia]

test/SemaOpenCL/printf-format-strings.cl

77

Can we add this explicitly to the test just for a record? I guess the warning message can't be changed to print the type correctly?

Also would it make sense to test other builtin types: double, char...?

[arsenm]

test/SemaOpenCL/printf-format-strings.cl

77

This should be easy to fix, but the way to test for doubles is enabled is making this unnecessarily difficult. It requires threading OpenCLOptions through all of the relevant functions to check for cl_khr_fp64. Why is this separate from LangOptions?

[arsenm]

Add tests without fp64

[Anastasia]

LGTM! Thanks!

test/SemaOpenCL/printf-format-strings.cl

77

I see. I think the problem is that it is set conditionally based on the targets in earlier CL versions. So it had to be a separated into target specific option. :(

[arsenm]

test/SemaOpenCL/printf-format-strings.cl

77

Would it be reasonable to add a new HasNoDouble field or something in LangOptions?

[arsenm]

r348084

include/clang/AST/FormatString.h

enum Kind {
     // to be copied by os_log. The precision indicates the number of bytes to
     // copy.
     PArg,
 
     // ** Printf-specific **
 
     ZArg, // MS extension
 
-    VArg, // OpenCL vectors
-
     // Objective-C specific specifiers.
     ObjCObjArg, // '@'
     ObjCBeg = ObjCObjArg,
     ObjCEnd = ObjCObjArg,
 
     // FreeBSD kernel specific specifiers.
     FreeBSDbArg,
     FreeBSDDArg,
static ArgType makePtrdiffT(const ArgType &A) {
     Res.TK = TypeKind::PtrdiffT;
     return Res;
   }
 
   MatchKind matchesType(ASTContext &C, QualType argTy) const;
 
   QualType getRepresentativeType(ASTContext &C) const;
 
+  ArgType makeVectorType(ASTContext &C, unsigned NumElts) const;
+
   std::string getRepresentativeTypeName(ASTContext &C) const;
 };
 
 class OptionalAmount {
 public:
   enum HowSpecified { NotSpecified, Constant, Arg, Invalid };
 
   OptionalAmount(HowSpecified howSpecified,
                  unsigned amount,
                  const char *amountStart,
                  unsigned amountLength,
                  bool usesPositionalArg)
   : start(amountStart), length(amountLength), hs(howSpecified), amt(amount),
   UsesPositionalArg(usesPositionalArg), UsesDotPrefix(0) {}
 
   OptionalAmount(bool valid = true)
   : 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;
   }
 
   HowSpecified getHowSpecified() const { return hs; }
   void setHowSpecified(HowSpecified h) { hs = h; }
 
   bool hasDataArgument() const { return hs == Arg; }

 };
 
 
 class FormatSpecifier {
 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
   bool UsesPositionalArg;
   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;
   }
 
   void setUsesPositionalArg() { UsesPositionalArg = true; }
 
   void setArgIndex(unsigned i) {
public:
   const LengthModifier &getLengthModifier() const {
     return LM;
   }
 
   const OptionalAmount &getFieldWidth() const {
     return FieldWidth;
   }
 
+  void setVectorNumElts(const OptionalAmount &Amt) {
+    VectorNumElts = Amt;
+  }
+
+  const OptionalAmount &getVectorNumElts() const {
+    return VectorNumElts;
+  }
+
   void setFieldWidth(const OptionalAmount &Amt) {
     FieldWidth = Amt;
   }
 
   bool usesPositionalArg() const { return UsesPositionalArg; }
 
   bool hasValidLengthModifier(const TargetInfo &Target) const;
 
class PrintfSpecifier : public analyze_format_string::FormatSpecifier {
   OptionalFlag HasAlternativeForm; // '#'
   OptionalFlag HasLeadingZeroes; // '0'
   OptionalFlag HasObjCTechnicalTerm; // '[tt]'
   OptionalFlag IsPrivate;            // '{private}'
   OptionalFlag IsPublic;             // '{public}'
   OptionalFlag IsSensitive;          // '{sensitive}'
   OptionalAmount Precision;
   StringRef MaskType;
+
+  ArgType getScalarArgType(ASTContext &Ctx, bool IsObjCLiteral) const;
+
 public:
   PrintfSpecifier()
       : FormatSpecifier(/* isPrintf = */ true), HasThousandsGrouping("'"),
         IsLeftJustified("-"), HasPlusPrefix("+"), HasSpacePrefix(" "),
         HasAlternativeForm("#"), HasLeadingZeroes("0"),
         HasObjCTechnicalTerm("tt"), IsPrivate("private"), IsPublic("public"),
         IsSensitive("sensitive") {}
 

lib/AST/FormatString.cpp

if (Amt.getHowSpecified() == OptionalAmount::Constant && *(I++) == '$') {
     Beg = I;
     return false;
   }
 
   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,
                                                   const char *E,
                                                   const LangOptions &LO,
                                                   bool IsScanf) {
   LengthModifier::Kind lmKind = LengthModifier::None;
   const char *lmPosition = I;
   switch (*I) {
case ObjCPointerTy: {
       }
       return NoMatch;
     }
   }
 
   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) {
     case InvalidTy:
       llvm_unreachable("No representative type for Invalid ArgType");
     case UnknownTy:
       llvm_unreachable("No representative type for Unknown ArgType");
     case AnyCharTy:
const char *ConversionSpecifier::toString() const {
   case FreeBSDrArg: return "r";
   case FreeBSDyArg: return "y";
 
   // GlibC specific specifiers.
   case PrintErrno: return "m";
 
   // MS specific specifiers.
   case ZArg: return "Z";
-
- // OpenCL specific specifiers.
-  case VArg: return "v";
   }
   return nullptr;
 }
 
 Optional<ConversionSpecifier>
 ConversionSpecifier::getStandardSpecifier() const {
   ConversionSpecifier::Kind NewKind;
 
switch (CS.getKind()) {
     case ConversionSpecifier::ObjCObjArg:
     case ConversionSpecifier::ScanListArg:
     case ConversionSpecifier::PercentArg:
     case ConversionSpecifier::PArg:
       return true;
     case ConversionSpecifier::CArg:
     case ConversionSpecifier::SArg:
       return LangOpt.ObjC;
-    case ConversionSpecifier::VArg:
-      return LangOpt.OpenCL;
     case ConversionSpecifier::InvalidSpecifier:
     case ConversionSpecifier::FreeBSDbArg:
     case ConversionSpecifier::FreeBSDDArg:
     case ConversionSpecifier::FreeBSDrArg:
     case ConversionSpecifier::FreeBSDyArg:
     case ConversionSpecifier::PrintErrno:
     case ConversionSpecifier::DArg:
     case ConversionSpecifier::OArg:

lib/AST/FormatStringParsing.h

bool ParseFieldWidth(FormatStringHandler &H,
                      FormatSpecifier &CS,
                      const char *Start, const char *&Beg, const char *E,
                      unsigned *argIndex);
 
 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,
                          const LangOptions &LO, bool IsScanf = false);
 
 /// Returns true if the invalid specifier in \p SpecifierBegin is a UTF-8
 /// string; check that it won't go further than \p FmtStrEnd and write
 /// up the total size in \p Len.

lib/AST/PrintfFormatString.cpp

if (*I == '.') {
     if (I == E) {
       // No more characters left?
       if (Warn)
         H.HandleIncompleteSpecifier(Start, E - Start);
       return true;
     }
   }
 
+  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?
     if (Warn)
       H.HandleIncompleteSpecifier(Start, E - Start);
     return true;
   }
 
case 'U':
       if (Target.getTriple().isOSDarwin())
         k = ConversionSpecifier::UArg;
       break;
     // MS specific.
     case 'Z':
       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
   // a conversion specifier other than '@'.
   if (k != ConversionSpecifier::ObjCObjArg &&
       k != ConversionSpecifier::InvalidSpecifier &&
       ObjCModifierFlagsStart) {
     H.HandleObjCFlagsWithNonObjCConversion(ObjCModifierFlagsStart,
bool clang::analyze_format_string::ParseFormatStringHasSArg(const char *I,
   }
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 // Methods on PrintfSpecifier.
 //===----------------------------------------------------------------------===//
 
-ArgType PrintfSpecifier::getArgType(ASTContext &Ctx,
+ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
                                           bool IsObjCLiteral) const {
-  const PrintfConversionSpecifier &CS = getConversionSpecifier();
-
-  if (!CS.consumesDataArgument())
-    return ArgType::Invalid();
-
   if (CS.getKind() == ConversionSpecifier::cArg)
     switch (LM.getKind()) {
       case LengthModifier::None:
         return Ctx.IntTy;
       case LengthModifier::AsLong:
       case LengthModifier::AsWide:
         return ArgType(ArgType::WIntTy, "wint_t");
       case LengthModifier::AsShort:
switch (CS.getKind()) {
     default:
       break;
   }
 
   // FIXME: Handle other cases.
   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.
   if (CS.getKind() == ConversionSpecifier::nArg)
     return false;
 
   // Handle Objective-C objects first. Note that while the '%@' specifier will
   // not warn for structure pointer or void pointer arguments (because that's
if (QT->isPointerType() && (QT->getPointeeType()->isAnyCharacterType())) {
 
     return true;
   }
 
   // If it's an enum, get its underlying type.
   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;
 
   // Set length modifier
   switch (BT->getKind()) {
   case BuiltinType::Bool:
   case BuiltinType::WChar_U:
   case BuiltinType::WChar_S:
void PrintfSpecifier::toString(raw_ostream &os) const {
   if (HasSpacePrefix)     os << " ";
   if (HasAlternativeForm) os << "#";
   if (HasLeadingZeroes)   os << "0";
 
   // Minimum field width
   FieldWidth.toString(os);
   // Precision
   Precision.toString(os);
+
+  // Vector modifier
+  if (!VectorNumElts.isInvalid())
+    os << 'v' << VectorNumElts.getConstantAmount();
+
   // Length modifier
   os << LM.toString();
   // Conversion specifier
   os << CS.toString();
 }
 
 bool PrintfSpecifier::hasValidPlusPrefix() const {
   if (!HasPlusPrefix)
bool PrintfSpecifier::hasValidPrecision() const {
   case ConversionSpecifier::fArg:
   case ConversionSpecifier::FArg:
   case ConversionSpecifier::gArg:
   case ConversionSpecifier::GArg:
   case ConversionSpecifier::sArg:
   case ConversionSpecifier::FreeBSDrArg:
   case ConversionSpecifier::FreeBSDyArg:
   case ConversionSpecifier::PArg:
-  case ConversionSpecifier::VArg:
     return true;
 
   default:
     return false;
   }
 }
 bool PrintfSpecifier::hasValidFieldWidth() const {
   if (FieldWidth.getHowSpecified() == OptionalAmount::NotSpecified)

test/SemaOpenCL/format-strings-fixit.cl

+// 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);
+}

test/SemaOpenCL/printf-format-strings.cl

-// 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_v4f32_wrong_num_elts(float2 arg)
+kernel void format_only_v(int arg)
 {
-    printf("%v4f\n", arg); // expected-no-diagnostics
+    printf("%v", arg); // expected-warning {{incomplete format specifier}}
 }
 
-kernel void vector_precision_modifier_v4f32(float4 arg)
+kernel void format_missing_num(int arg)
 {
-    printf("%.2v4f\n", arg); // expected-no-diagnostics
+  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-warning {{format specifies type 'double __attribute__((ext_vector_type(4)))' but the argument has type 'float2' (vector of 2 'float' values)}}
 }
 
-// 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);
 }