Skip to content

Commit 58fc808

Browse files
arsenmarsenm
committed
OpenCL: Use length modifier for warning on vector printf arguments
Re-enable format string warnings on printf. The warnings are still incomplete. Apparently it is undefined to use a vector specifier without a length modifier, which is not currently warned on. Additionally, type warnings appear to not be working with the hh modifier, and aren't warning on all of the special restrictions from c99 printf. llvm-svn: 352540
1 parent 297afb1 commit 58fc808

File tree

10 files changed

+245
-33
lines changed

10 files changed

+245
-33
lines changed

‎clang/include/clang/AST/FormatString.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ class LengthModifier {
6767
None,
6868
AsChar, // 'hh'
6969
AsShort, // 'h'
70+
AsShortLong, // 'hl' (OpenCL float/int vector element)
7071
AsLong, // 'l'
7172
AsLongLong, // 'll'
7273
AsQuad, // 'q' (BSD, deprecated, for 64-bit integer types)
@@ -436,7 +437,8 @@ class FormatSpecifier {
436437

437438
bool usesPositionalArg() const { return UsesPositionalArg; }
438439

439-
bool hasValidLengthModifier(const TargetInfo &Target) const;
440+
bool hasValidLengthModifier(const TargetInfo &Target,
441+
const LangOptions &LO) const;
440442

441443
bool hasStandardLengthModifier() const;
442444

‎clang/lib/AST/FormatString.cpp

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,9 @@ clang::analyze_format_string::ParseLengthModifier(FormatSpecifier &FS,
223223
if (I != E && *I == 'h') {
224224
++I;
225225
lmKind = LengthModifier::AsChar;
226+
} else if (I != E && *I == 'l' && LO.OpenCL) {
227+
++I;
228+
lmKind = LengthModifier::AsShortLong;
226229
} else {
227230
lmKind = LengthModifier::AsShort;
228231
}
@@ -487,7 +490,8 @@ ArgType::matchesType(ASTContext &C, QualType argTy) const {
487490
}
488491

489492
ArgType ArgType::makeVectorType(ASTContext &C, unsigned NumElts) const {
490-
if (K != SpecificTy) // Won't be a valid vector element type.
493+
// Check for valid vector element types.
494+
if (T.isNull())
491495
return ArgType::Invalid();
492496

493497
QualType Vec = C.getExtVectorType(T, NumElts);
@@ -572,6 +576,8 @@ analyze_format_string::LengthModifier::toString() const {
572576
return "hh";
573577
case AsShort:
574578
return "h";
579+
case AsShortLong:
580+
return "hl";
575581
case AsLong: // or AsWideChar
576582
return "l";
577583
case AsLongLong:
@@ -707,13 +713,18 @@ void OptionalAmount::toString(raw_ostream &os) const {
707713
}
708714
}
709715

710-
bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target) const {
716+
bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target,
717+
const LangOptions &LO) const {
711718
switch (LM.getKind()) {
712719
case LengthModifier::None:
713720
return true;
714721

715722
// Handle most integer flags
716723
case LengthModifier::AsShort:
724+
// Length modifier only applies to FP vectors.
725+
if (LO.OpenCL && CS.isDoubleArg())
726+
return !VectorNumElts.isInvalid();
727+
717728
if (Target.getTriple().isOSMSVCRT()) {
718729
switch (CS.getKind()) {
719730
case ConversionSpecifier::cArg:
@@ -752,8 +763,18 @@ bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target) const {
752763
return false;
753764
}
754765

766+
case LengthModifier::AsShortLong:
767+
return LO.OpenCL && !VectorNumElts.isInvalid();
768+
755769
// Handle 'l' flag
756770
case LengthModifier::AsLong: // or AsWideChar
771+
if (CS.isDoubleArg()) {
772+
// Invalid for OpenCL FP scalars.
773+
if (LO.OpenCL && VectorNumElts.isInvalid())
774+
return false;
775+
return true;
776+
}
777+
757778
switch (CS.getKind()) {
758779
case ConversionSpecifier::dArg:
759780
case ConversionSpecifier::DArg:
@@ -764,14 +785,6 @@ bool FormatSpecifier::hasValidLengthModifier(const TargetInfo &Target) const {
764785
case ConversionSpecifier::UArg:
765786
case ConversionSpecifier::xArg:
766787
case ConversionSpecifier::XArg:
767-
case ConversionSpecifier::aArg:
768-
case ConversionSpecifier::AArg:
769-
case ConversionSpecifier::fArg:
770-
case ConversionSpecifier::FArg:
771-
case ConversionSpecifier::eArg:
772-
case ConversionSpecifier::EArg:
773-
case ConversionSpecifier::gArg:
774-
case ConversionSpecifier::GArg:
775788
case ConversionSpecifier::nArg:
776789
case ConversionSpecifier::cArg:
777790
case ConversionSpecifier::sArg:
@@ -878,6 +891,7 @@ bool FormatSpecifier::hasStandardLengthModifier() const {
878891
case LengthModifier::AsInt3264:
879892
case LengthModifier::AsInt64:
880893
case LengthModifier::AsWide:
894+
case LengthModifier::AsShortLong: // ???
881895
return false;
882896
}
883897
llvm_unreachable("Invalid LengthModifier Kind!");

‎clang/lib/AST/PrintfFormatString.cpp

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,11 @@ static PrintfSpecifierResult ParsePrintfSpecifier(FormatStringHandler &H,
315315
case 'f': k = ConversionSpecifier::fArg; break;
316316
case 'g': k = ConversionSpecifier::gArg; break;
317317
case 'i': k = ConversionSpecifier::iArg; break;
318-
case 'n': k = ConversionSpecifier::nArg; break;
318+
case 'n':
319+
// Not handled, but reserved in OpenCL.
320+
if (!LO.OpenCL)
321+
k = ConversionSpecifier::nArg;
322+
break;
319323
case 'o': k = ConversionSpecifier::oArg; break;
320324
case 'p': k = ConversionSpecifier::pArg; break;
321325
case 's': k = ConversionSpecifier::sArg; break;
@@ -486,10 +490,12 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
486490
// GNU extension.
487491
return Ctx.LongLongTy;
488492
case LengthModifier::None:
493+
case LengthModifier::AsShortLong:
489494
return Ctx.IntTy;
490495
case LengthModifier::AsInt32:
491496
return ArgType(Ctx.IntTy, "__int32");
492-
case LengthModifier::AsChar: return ArgType::AnyCharTy;
497+
case LengthModifier::AsChar:
498+
return ArgType::AnyCharTy;
493499
case LengthModifier::AsShort: return Ctx.ShortTy;
494500
case LengthModifier::AsLong: return Ctx.LongTy;
495501
case LengthModifier::AsLongLong:
@@ -520,6 +526,7 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
520526
// GNU extension.
521527
return Ctx.UnsignedLongLongTy;
522528
case LengthModifier::None:
529+
case LengthModifier::AsShortLong:
523530
return Ctx.UnsignedIntTy;
524531
case LengthModifier::AsInt32:
525532
return ArgType(Ctx.UnsignedIntTy, "unsigned __int32");
@@ -549,6 +556,18 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
549556
}
550557

551558
if (CS.isDoubleArg()) {
559+
if (!VectorNumElts.isInvalid()) {
560+
switch (LM.getKind()) {
561+
case LengthModifier::AsShort:
562+
return Ctx.HalfTy;
563+
case LengthModifier::AsShortLong:
564+
return Ctx.FloatTy;
565+
case LengthModifier::AsLong:
566+
default:
567+
return Ctx.DoubleTy;
568+
}
569+
}
570+
552571
if (LM.getKind() == LengthModifier::AsLongDouble)
553572
return Ctx.LongDoubleTy;
554573
return Ctx.DoubleTy;
@@ -582,6 +601,8 @@ ArgType PrintfSpecifier::getScalarArgType(ASTContext &Ctx,
582601
case LengthModifier::AsInt64:
583602
case LengthModifier::AsWide:
584603
return ArgType::Invalid();
604+
case LengthModifier::AsShortLong:
605+
llvm_unreachable("only used for OpenCL which doesn not handle nArg");
585606
}
586607
}
587608

@@ -760,10 +781,13 @@ bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
760781
case BuiltinType::UInt:
761782
case BuiltinType::Int:
762783
case BuiltinType::Float:
784+
LM.setKind(VectorNumElts.isInvalid() ?
785+
LengthModifier::None : LengthModifier::AsShortLong);
786+
break;
763787
case BuiltinType::Double:
764-
LM.setKind(LengthModifier::None);
788+
LM.setKind(VectorNumElts.isInvalid() ?
789+
LengthModifier::None : LengthModifier::AsLong);
765790
break;
766-
767791
case BuiltinType::Char_U:
768792
case BuiltinType::UChar:
769793
case BuiltinType::Char_S:
@@ -796,7 +820,7 @@ bool PrintfSpecifier::fixType(QualType QT, const LangOptions &LangOpt,
796820
namedTypeToLengthModifier(QT, LM);
797821

798822
// If fixing the length modifier was enough, we might be done.
799-
if (hasValidLengthModifier(Ctx.getTargetInfo())) {
823+
if (hasValidLengthModifier(Ctx.getTargetInfo(), LangOpt)) {
800824
// If we're going to offer a fix anyway, make sure the sign matches.
801825
switch (CS.getKind()) {
802826
case ConversionSpecifier::uArg:

‎clang/lib/AST/ScanfFormatString.cpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,9 +261,10 @@ ArgType ScanfSpecifier::getArgType(ASTContext &Ctx) const {
261261
case LengthModifier::AsInt32:
262262
case LengthModifier::AsInt3264:
263263
case LengthModifier::AsWide:
264+
case LengthModifier::AsShortLong:
264265
return ArgType::Invalid();
265266
}
266-
llvm_unreachable("Unsupported LenghtModifier Type");
267+
llvm_unreachable("Unsupported LengthModifier Type");
267268

268269
// Unsigned int.
269270
case ConversionSpecifier::oArg:
@@ -301,9 +302,10 @@ ArgType ScanfSpecifier::getArgType(ASTContext &Ctx) const {
301302
case LengthModifier::AsInt32:
302303
case LengthModifier::AsInt3264:
303304
case LengthModifier::AsWide:
305+
case LengthModifier::AsShortLong:
304306
return ArgType::Invalid();
305307
}
306-
llvm_unreachable("Unsupported LenghtModifier Type");
308+
llvm_unreachable("Unsupported LengthModifier Type");
307309

308310
// Float.
309311
case ConversionSpecifier::aArg:
@@ -396,6 +398,7 @@ ArgType ScanfSpecifier::getArgType(ASTContext &Ctx) const {
396398
case LengthModifier::AsInt32:
397399
case LengthModifier::AsInt3264:
398400
case LengthModifier::AsWide:
401+
case LengthModifier::AsShortLong:
399402
return ArgType::Invalid();
400403
}
401404

@@ -501,7 +504,7 @@ bool ScanfSpecifier::fixType(QualType QT, QualType RawQT,
501504
namedTypeToLengthModifier(PT, LM);
502505

503506
// If fixing the length modifier was enough, we are done.
504-
if (hasValidLengthModifier(Ctx.getTargetInfo())) {
507+
if (hasValidLengthModifier(Ctx.getTargetInfo(), LangOpt)) {
505508
const analyze_scanf::ArgType &AT = getArgType(Ctx);
506509
if (AT.isValid() && AT.matchesType(Ctx, QT))
507510
return true;

‎clang/lib/Headers/opencl-c.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14469,7 +14469,7 @@ half16 __ovld __cnfn shuffle2(half16 x, half16 y, ushort16 mask);
1446914469
#if __OPENCL_C_VERSION__ >= CL_VERSION_1_2
1447014470
// OpenCL v1.2 s6.12.13, v2.0 s6.13.13 - printf
1447114471

14472-
int printf(__constant const char* st, ...);
14472+
int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
1447314473
#endif
1447414474

1447514475
// OpenCL v1.1 s6.11.3, v1.2 s6.12.14, v2.0 s6.13.14 - Image Read and Write Functions

‎clang/lib/Sema/SemaChecking.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7650,7 +7650,8 @@ CheckPrintfHandler::HandlePrintfSpecifier(const analyze_printf::PrintfSpecifier
76507650
startSpecifier, specifierLen);
76517651

76527652
// Check the length modifier is valid with the given conversion specifier.
7653-
if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo()))
7653+
if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo(),
7654+
S.getLangOpts()))
76547655
HandleInvalidLengthModifier(FS, CS, startSpecifier, specifierLen,
76557656
diag::warn_format_nonsensical_length);
76567657
else if (!FS.hasStandardLengthModifier())
@@ -8154,7 +8155,8 @@ bool CheckScanfHandler::HandleScanfSpecifier(
81548155
}
81558156

81568157
// Check the length modifier is valid with the given conversion specifier.
8157-
if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo()))
8158+
if (!FS.hasValidLengthModifier(S.getASTContext().getTargetInfo(),
8159+
S.getLangOpts()))
81588160
HandleInvalidLengthModifier(FS, CS, startSpecifier, specifierLen,
81598161
diag::warn_format_nonsensical_length);
81608162
else if (!FS.hasStandardLengthModifier())

‎clang/test/Sema/format-strings.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,8 @@ void test_opencl_vector_format(int x) {
617617
printf("%v4d", x); // expected-warning{{invalid conversion specifier 'v'}}
618618
printf("%vd", x); // expected-warning{{invalid conversion specifier 'v'}}
619619
printf("%0vd", x); // expected-warning{{invalid conversion specifier 'v'}}
620+
printf("%hlf", x); // expected-warning{{invalid conversion specifier 'l'}}
621+
printf("%hld", x); // expected-warning{{invalid conversion specifier 'l'}}
620622
}
621623

622624
// Test that we correctly merge the format in both orders.

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

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,22 +3,72 @@
33
// RUN: %clang_cc1 -cl-std=CL1.2 -fsyntax-only -pedantic -Wall -Werror %t
44
// RUN: %clang_cc1 -cl-std=CL1.2 -E -o - %t | FileCheck %s
55

6+
typedef __attribute__((ext_vector_type(4))) char char4;
7+
typedef __attribute__((ext_vector_type(4))) short short4;
68
typedef __attribute__((ext_vector_type(4))) int int4;
9+
typedef __attribute__((ext_vector_type(4))) unsigned int uint4;
710
typedef __attribute__((ext_vector_type(8))) int int8;
11+
typedef __attribute__((ext_vector_type(4))) long long4;
12+
typedef __attribute__((ext_vector_type(4))) float float4;
13+
typedef __attribute__((ext_vector_type(4))) double double4;
814

915
int printf(__constant const char* st, ...) __attribute__((format(printf, 1, 2)));
1016

1117

1218
void vector_fixits() {
1319
printf("%v4f", (int4) 123);
14-
// CHECK: printf("%v4d", (int4) 123);
20+
// CHECK: printf("%v4hld", (int4) 123);
1521

1622
printf("%v8d", (int4) 123);
17-
// CHECK: printf("%v4d", (int4) 123);
23+
// CHECK: printf("%v4hld", (int4) 123);
1824

1925
printf("%v4d", (int8) 123);
20-
// CHECK: printf("%v8d", (int8) 123);
26+
// CHECK: printf("%v8hld", (int8) 123);
2127

2228
printf("%v4f", (int8) 123);
23-
// CHECK: printf("%v8d", (int8) 123);
29+
// CHECK: printf("%v8hld", (int8) 123);
30+
31+
printf("%v4ld", (int8) 123);
32+
// CHECK: printf("%v8hld", (int8) 123);
33+
34+
printf("%v4hlf", (int4) 123);
35+
// CHECK: printf("%v4hld", (int4) 123);
36+
37+
printf("%v8hld", (int4) 123);
38+
// CHECK: printf("%v4hld", (int4) 123);
39+
40+
printf("%v4hld", (int8) 123);
41+
// CHECK: printf("%v8hld", (int8) 123);
42+
43+
printf("%v4hlf", (int8) 123);
44+
// CHECK: printf("%v8hld", (int8) 123);
45+
46+
printf("%v4hd", (int4) 123);
47+
// CHECK: printf("%v4hld", (int4) 123);
48+
49+
printf("%v4hld", (short4) 123);
50+
// CHECK: printf("%v4hd", (short4) 123);
51+
52+
printf("%v4ld", (short4) 123);
53+
// CHECK: printf("%v4hd", (short4) 123);
54+
55+
printf("%v4hld", (long4) 123);
56+
// CHECK: printf("%v4ld", (long4) 123);
57+
58+
printf("%v8f", (float4) 2.0f);
59+
// CHECK: printf("%v4hlf", (float4) 2.0f);
60+
61+
printf("%v4f", (float4) 2.0f);
62+
// CHECK: printf("%v4hlf", (float4) 2.0f);
63+
64+
printf("%v4lf", (double4) 2.0);
65+
// CHECK: printf("%v4lf", (double4) 2.0);
66+
67+
/// FIXME: This should be fixed
68+
printf("%v4hhd", (int4) 123);
69+
// CHECK: printf("%v4hhd", (int4) 123);
70+
71+
/// FIXME: This should be fixed
72+
printf("%v4hhd", (int8) 123);
73+
// CHECK: printf("%v4hhd", (int8) 123);
2474
}

‎clang/test/SemaOpenCL/printf-format-string-warnings.cl

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=CL2.0 -finclude-default-header
22

3-
// FIXME: Make sure warnings are produced based on printf format strings.
4-
5-
// expected-no-diagnostics
3+
// Make sure warnings are produced based on printf format strings.
64

75
kernel void format_string_warnings(__constant char* arg) {
86

9-
printf("%d", arg);
7+
printf("%d", arg); // expected-warning {{format specifies type 'int' but the argument has type '__constant char *'}}
108

11-
printf("not enough arguments %d %d", 4);
9+
printf("not enough arguments %d %d", 4); // expected-warning {{more '%' conversions than data arguments}}
1210

13-
printf("too many arguments", 4);
11+
printf("too many arguments", 4); // expected-warning {{data argument not used by format string}}
1412
}

0 commit comments

Comments
 (0)