From 0ff50d49d1f2611a1adbeaa502d9bf7b1237d31b Mon Sep 17 00:00:00 2001 From: Matt Arsenault Date: Sat, 1 Dec 2018 22:16:27 +0000 Subject: [PATCH] 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 | 24 +++++++- clang/lib/AST/FormatString.cpp | 43 ++++++++++++-- clang/lib/AST/FormatStringParsing.h | 4 ++ clang/lib/AST/PrintfFormatString.cpp | 49 +++++++++++----- clang/test/SemaOpenCL/format-strings-fixit.cl | 24 ++++++++ clang/test/SemaOpenCL/printf-format-strings.cl | 80 ++++++++++++++++++++++---- 6 files changed, 192 insertions(+), 32 deletions(-) create mode 100644 clang/test/SemaOpenCL/format-strings-fixit.cl diff --git a/clang/include/clang/AST/FormatString.h b/clang/include/clang/AST/FormatString.h index 4170d718..4a89c79 100644 --- a/clang/include/clang/AST/FormatString.h +++ b/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("'"), diff --git a/clang/lib/AST/FormatString.cpp b/clang/lib/AST/FormatString.cpp index 565bd03..04bd48f 100644 --- a/clang/lib/AST/FormatString.cpp +++ b/clang/lib/AST/FormatString.cpp @@ -179,6 +179,36 @@ clang::analyze_format_string::ParseArgPosition(FormatStringHandler &H, } 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, @@ -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: diff --git a/clang/lib/AST/FormatStringParsing.h b/clang/lib/AST/FormatStringParsing.h index 91fab15..9da829a 100644 --- a/clang/lib/AST/FormatStringParsing.h +++ b/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, diff --git a/clang/lib/AST/PrintfFormatString.cpp b/clang/lib/AST/PrintfFormatString.cpp index 877f890..e0a0c5b 100644 --- a/clang/lib/AST/PrintfFormatString.cpp +++ b/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, - bool IsObjCLiteral) const { - const PrintfConversionSpecifier &CS = getConversionSpecifier(); - - if (!CS.consumesDataArgument()) - return ArgType::Invalid(); - +ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx, + bool IsObjCLiteral) const { 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()) QT = ETy->getDecl()->getIntegerType(); - // We can only work with builtin types. const BuiltinType *BT = QT->getAs(); + if (!BT) { + const VectorType *VT = QT->getAs(); + if (VT) { + QT = VT->getElementType(); + BT = QT->getAs(); + 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: diff --git a/clang/test/SemaOpenCL/format-strings-fixit.cl b/clang/test/SemaOpenCL/format-strings-fixit.cl new file mode 100644 index 0000000..b9f949f --- /dev/null +++ b/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); +} diff --git a/clang/test/SemaOpenCL/printf-format-strings.cl b/clang/test/SemaOpenCL/printf-format-strings.cl index d5748e1..079a834 100644 --- a/clang/test/SemaOpenCL/printf-format-strings.cl +++ b/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_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); } -- 2.7.4