[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit as r349497, thank you! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added a comment. Thanks, that would be great! Hopefully it will work this time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM then, thank you for the fix! Would you like me to commit this one for you? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan updated this revision to Diff 178605.
ebevhan added a comment.
Use `auto`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings-bitfield-promotion.c
test/Sema/format-strings-bitfield-promotion.cxx
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur, unlike C.
+// expected-no-diagnostics
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a);
+ printf("%lu", bf.b);
+ printf("%ld", bf.c);
+ printf("%lu", bf.d);
+}
Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7709,6 +7709,30 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's an integer promotion if the destination type is the promoted
+ // source type.
+ if (ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To)
+return true;
+ // Look through vector types, since we do default argument promotion for
+ // those in OpenCL.
+ if (const auto *VecTy = From->getAs())
+From = VecTy->getElementType();
+ if (const auto *VecTy = To->getAs())
+To = VecTy->getElementType();
+ // It's a floating promotion if the source type is a lower rank.
+ return ICE->getCastKind() == CK_FloatingCast &&
+ S.Context.getFloatingTypeOrder(From, To) < 0;
+}
+
bool
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier ,
const char *StartSpecifier,
@@ -7736,11 +7760,11 @@
// Look through argument promotions for our error message's reported type.
// This includes the integral and floating promotions, but excludes array
- // and function pointer decay; seeing that an argument intended to be a
- // string has type 'char [6]' is probably more confusing than 'char *'.
+ // and function pointer decay (seeing that an argument intended to be a
+ // string has type 'char [6]' is probably more confusing than 'char *') and
+ // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
E = ICE->getSubExpr();
ExprTy = E->getType();
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur, unlike C.
+// expected-no-diagnostics
+
+int printf(const char *restrict, ...);
+
+struct
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7725-7726 +return true; + // Look through vector types, since we do default argument promotion for + // those in OpenCL. + if (const ExtVectorType *VecTy = From->getAs()) aaron.ballman wrote: > Looking at `Sema::DefaultArgumentPromotion()`, it seems there is some special > logic there for _Float16 vs half/fp16. Do we need to deal with that here as > well? I think it should be fine. If the special handling of those don't take, there won't be a default promotion for them, so this routine won't care about it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7725-7726 +return true; + // Look through vector types, since we do default argument promotion for + // those in OpenCL. + if (const ExtVectorType *VecTy = From->getAs()) Looking at `Sema::DefaultArgumentPromotion()`, it seems there is some special logic there for _Float16 vs half/fp16. Do we need to deal with that here as well? Comment at: lib/Sema/SemaChecking.cpp:7727 + // those in OpenCL. + if (const ExtVectorType *VecTy = From->getAs()) +From = VecTy->getElementType(); Should use `const auto *` here and below. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan updated this revision to Diff 177831.
ebevhan added a comment.
Fix the build failures (caused by default argument promotion of float vectors).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings-bitfield-promotion.c
test/Sema/format-strings-bitfield-promotion.cxx
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur, unlike C.
+// expected-no-diagnostics
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a);
+ printf("%lu", bf.b);
+ printf("%ld", bf.c);
+ printf("%lu", bf.d);
+}
Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7709,6 +7709,30 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's an integer promotion if the destination type is the promoted
+ // source type.
+ if (ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To)
+return true;
+ // Look through vector types, since we do default argument promotion for
+ // those in OpenCL.
+ if (const ExtVectorType *VecTy = From->getAs())
+From = VecTy->getElementType();
+ if (const ExtVectorType *VecTy = To->getAs())
+To = VecTy->getElementType();
+ // It's a floating promotion if the source type is a lower rank.
+ return ICE->getCastKind() == CK_FloatingCast &&
+ S.Context.getFloatingTypeOrder(From, To) < 0;
+}
+
bool
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier ,
const char *StartSpecifier,
@@ -7736,11 +7760,11 @@
// Look through argument promotions for our error message's reported type.
// This includes the integral and floating promotions, but excludes array
- // and function pointer decay; seeing that an argument intended to be a
- // string has type 'char [6]' is probably more confusing than 'char *'.
+ // and function pointer decay (seeing that an argument intended to be a
+ // string has type 'char [6]' is probably more confusing than 'char *') and
+ // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
E = ICE->getSubExpr();
ExprTy = E->getType();
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur,
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added a comment. Hmm, sorry about that. I'll have a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman reopened this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
I had to revert the commit as the changes caused some test failures.
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40784/steps/test/logs/stdio
FAIL: Clang :: CodeGenOpenCL/printf.cl (8013 of 45545)
TEST 'Clang :: CodeGenOpenCL/printf.cl' FAILED
Script:
--
: 'RUN: at line 1';
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang
-cc1 -internal-isystem
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/lib/clang/8.0.0/include
-nostdsysteminc -cl-std=CL1.2 -cl-ext=-+cl_khr_fp64 -triple
spir-unknown-unknown -disable-llvm-passes -emit-llvm -o -
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl
|
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/FileCheck
-check-prefixes=FP64,ALL
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl
: 'RUN: at line 2';
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang
-cc1 -internal-isystem
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/lib/clang/8.0.0/include
-nostdsysteminc -cl-std=CL1.2 -cl-ext=-cl_khr_fp64 -triple
spir-unknown-unknown -disable-llvm-passes -emit-llvm -o -
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl
|
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/FileCheck
-check-prefixes=NOFP64,ALL
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl
--
Exit Code: 2
Command Output (stderr):
--
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl:20:18:
warning: format specifies type 'double __attribute__((ext_vector_type(2)))'
but the argument has type 'float2' (vector of 2 'float' values)
printf("%v2f", arg);
^~~
%v2f
clang:
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/lib/AST/ASTContext.cpp:5554:
FloatingRank getFloatingRank(clang::QualType): Assertion
`T->getAs() && "getFloatingRank(): not a floating type"' failed.
Stack dump:
0.Program arguments:
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang
-cc1 -internal-isystem
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/lib/clang/8.0.0/include
-nostdsysteminc -cl-std=CL1.2 -cl-ext=-cl_khr_fp64 -triple
spir-unknown-unknown -disable-llvm-passes -emit-llvm -o -
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl
1.
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl:30:21:
current parser token ')'
2.
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl:29:42:
parsing function body 'test_printf_half2'
3.
/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.src/tools/clang/test/CodeGenOpenCL/printf.cl:29:42:
in compound statement ('{}')
#0 0x015d9934 PrintStackTraceSignalHandler(void*)
(/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang+0x15d9934)
#1 0x015d769e llvm::sys::RunSignalHandlers()
(/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang+0x15d769e)
#2 0x015d9af8 SignalHandler(int)
(/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang+0x15d9af8)
#3 0x7f95cde44890 __restore_rt
(/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
#4 0x7f95ccb0ae97 gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x3ee97)
#5 0x7f95ccb0c801 abort (/lib/x86_64-linux-gnu/libc.so.6+0x40801)
#6 0x7f95ccafc39a (/lib/x86_64-linux-gnu/libc.so.6+0x3039a)
#7 0x7f95ccafc412 (/lib/x86_64-linux-gnu/libc.so.6+0x30412)
#8 0x032f872c getFloatingRank(clang::QualType)
(/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang+0x32f872c)
#9 0x032f877e
clang::ASTContext::getFloatingTypeOrder(clang::QualType, clang::QualType) const
(/home/buildslave/ps4-buildslave4/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/llvm.obj/bin/clang+0x32f877e)
#10 0x02bf9316 (anonymous
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r348889, thank you for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added a comment. Thanks! I don't have commit access, so I would appreciate it if you could commit the change. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
ebevhan wrote:
> aaron.ballman wrote:
> > ebevhan wrote:
> > > ebevhan wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > Running your test through GCC looks like the behavior matches here
> > > > > > for C; can you also add a C++ test that demonstrates the behavior
> > > > > > does not change?
> > > > > >
> > > > > > https://godbolt.org/z/zRYDMG
> > > > > Strangely, the above godbolt link dropped the output windows, here's
> > > > > a different link that shows the behavioral differences between C and
> > > > > C++ mode in GCC: https://godbolt.org/z/R3zRHe
> > > > Hmm, I'll have a look at this.
> > > That gcc godbolt is a bit odd. The type of the bitfield expression in the
> > > C++ example is `long` and not `int`, but in Clang, it's clearly being
> > > converted. If I change the example a bit, we get this warning:
> > > ```
> > > :11:12: warning: format '%d' expects argument of type 'int', but
> > > argument 2 has type 'long int' [-Wformat=]
> > >11 | printf("%d", bf.a); // expected-warning {{format specifies type
> > > 'long' but the argument has type 'int'}}
> > > | ~^
> > > || |
> > > |intlong int
> > > ```
> > > But in Clang, we get a cast to `int`:
> > > ```
> > > | `-ImplicitCastExpr 0xd190748 'int'
> > > | `-ImplicitCastExpr 0xd190730 'long'
> > >
> > > | `-MemberExpr 0xd190618 'long' lvalue bitfield
> > > .a 0xd18f790
> > > | `-DeclRefExpr 0xd1905f8 'struct
> > > bitfields':'bitfields' lvalue Var 0xd18fa18 'bf' 'struct
> > > bitfields':'bitfields'
> > > ```
> > >
> > > So gcc and Clang are doing things differently here.
> > >
> > > The code in `isPromotableBitField` says:
> > > ```
> > > // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
> > > //We perform that promotion here to match GCC and C++.
> > > ```
> > > but clearly gcc isn't doing this in the C++ case. The comments also
> > > mention some things about gcc bugs that Clang does not follow, but that's
> > > in reference to a C DR.
> > C++ disallows the rank conversion from int to long as well. [conv.prom]p1
> > does not apply because `long int` has a higher rank than `int`, but
> > [conv.prom]p5 allows the promotion if the range of values is identical
> > between the two types.
> >
> > C makes this UB in several ways -- you can't have a bit-field whose type is
> > something other than int, unsigned int, or _Bool (6.7.2.1p5) or promoting
> > from types other than those (6.3.1.1p2), but otherwise matches the C++
> > behavior in terms of promotion (including the rank conversion).
> >
> > You may have to dig further into what Clang is doing, but I would guess
> > that the diagnostics should be triggered in both C and C++ similarly.
> >
> > Ultimately, I'd like to see tests for cases where `sizeof(int) ==
> > sizeof(long)`, `sizeof(int) != sizeof(long)`, and variants for C and C++ of
> > each.
> I'm not sure the warning should trigger in C++; the behavior is correct
> there. The expression in those cases should be of type `long`, not `int`. The
> bitfield promotions in C++ say that values _can_ be promoted if the value
> fits in `int`, but the rules in C say that the value _is_ promoted.
>
> The strange promotion behavior only occurs in C because of the issue with
> bitfields larger than int. It's not really permitted according to the
> standard, but it's supported anyway to match C++. Though, it ends up not
> matching C++ due to these promotion differences.
>
> I'll add tests for the different int and long sizes, though the only case
> where it would make a difference would be if int was larger than 32 bits,
> which it isn't on any target.
> The bitfield promotions in C++ say that values _can_ be promoted if the value
> fits in int, but the rules in C say that the value _is_ promoted.
Ahhh, that explains the differences (my eyes glossed over that in the standards
text), thank you!
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan updated this revision to Diff 176571.
ebevhan added a comment.
Added testing for C++ and different sizes of `int` and `long`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings-bitfield-promotion.c
test/Sema/format-strings-bitfield-promotion.cxx
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur, unlike C.
+// expected-no-diagnostics
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a);
+ printf("%lu", bf.b);
+ printf("%ld", bf.c);
+ printf("%lu", bf.d);
+}
Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's a floating promotion if the source type is a lower rank.
+ if (ICE->getCastKind() == CK_FloatingCast &&
+ S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+ // It's an integer promotion if the destination type is the promoted
+ // source type.
+ return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;
+}
+
bool
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier ,
const char *StartSpecifier,
@@ -7724,11 +7742,11 @@
// Look through argument promotions for our error message's reported type.
// This includes the integral and floating promotions, but excludes array
- // and function pointer decay; seeing that an argument intended to be a
- // string has type 'char [6]' is probably more confusing than 'char *'.
+ // and function pointer decay (seeing that an argument intended to be a
+ // string has type 'char [6]' is probably more confusing than 'char *') and
+ // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
E = ICE->getSubExpr();
ExprTy = E->getType();
Index: test/Sema/format-strings-bitfield-promotion.cxx
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.cxx
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple x86_64-unknown-windows-msvc -fsyntax-only -verify %s
+
+// In C++, the bitfield promotion from long to int does not occur, unlike C.
+// expected-no-diagnostics
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added inline comments.
Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
aaron.ballman wrote:
> ebevhan wrote:
> > ebevhan wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > Running your test through GCC looks like the behavior matches here
> > > > > for C; can you also add a C++ test that demonstrates the behavior
> > > > > does not change?
> > > > >
> > > > > https://godbolt.org/z/zRYDMG
> > > > Strangely, the above godbolt link dropped the output windows, here's a
> > > > different link that shows the behavioral differences between C and C++
> > > > mode in GCC: https://godbolt.org/z/R3zRHe
> > > Hmm, I'll have a look at this.
> > That gcc godbolt is a bit odd. The type of the bitfield expression in the
> > C++ example is `long` and not `int`, but in Clang, it's clearly being
> > converted. If I change the example a bit, we get this warning:
> > ```
> > :11:12: warning: format '%d' expects argument of type 'int', but
> > argument 2 has type 'long int' [-Wformat=]
> >11 | printf("%d", bf.a); // expected-warning {{format specifies type
> > 'long' but the argument has type 'int'}}
> > | ~^
> > || |
> > |intlong int
> > ```
> > But in Clang, we get a cast to `int`:
> > ```
> > | `-ImplicitCastExpr 0xd190748 'int'
> > | `-ImplicitCastExpr 0xd190730 'long'
> >
> > | `-MemberExpr 0xd190618 'long' lvalue bitfield .a
> > 0xd18f790
> > | `-DeclRefExpr 0xd1905f8 'struct bitfields':'bitfields'
> > lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
> > ```
> >
> > So gcc and Clang are doing things differently here.
> >
> > The code in `isPromotableBitField` says:
> > ```
> > // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
> > //We perform that promotion here to match GCC and C++.
> > ```
> > but clearly gcc isn't doing this in the C++ case. The comments also mention
> > some things about gcc bugs that Clang does not follow, but that's in
> > reference to a C DR.
> C++ disallows the rank conversion from int to long as well. [conv.prom]p1
> does not apply because `long int` has a higher rank than `int`, but
> [conv.prom]p5 allows the promotion if the range of values is identical
> between the two types.
>
> C makes this UB in several ways -- you can't have a bit-field whose type is
> something other than int, unsigned int, or _Bool (6.7.2.1p5) or promoting
> from types other than those (6.3.1.1p2), but otherwise matches the C++
> behavior in terms of promotion (including the rank conversion).
>
> You may have to dig further into what Clang is doing, but I would guess that
> the diagnostics should be triggered in both C and C++ similarly.
>
> Ultimately, I'd like to see tests for cases where `sizeof(int) ==
> sizeof(long)`, `sizeof(int) != sizeof(long)`, and variants for C and C++ of
> each.
I'm not sure the warning should trigger in C++; the behavior is correct there.
The expression in those cases should be of type `long`, not `int`. The bitfield
promotions in C++ say that values _can_ be promoted if the value fits in `int`,
but the rules in C say that the value _is_ promoted.
The strange promotion behavior only occurs in C because of the issue with
bitfields larger than int. It's not really permitted according to the standard,
but it's supported anyway to match C++. Though, it ends up not matching C++ due
to these promotion differences.
I'll add tests for the different int and long sizes, though the only case where
it would make a difference would be if int was larger than 32 bits, which it
isn't on any target.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman added inline comments.
Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
ebevhan wrote:
> ebevhan wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > Running your test through GCC looks like the behavior matches here for
> > > > C; can you also add a C++ test that demonstrates the behavior does not
> > > > change?
> > > >
> > > > https://godbolt.org/z/zRYDMG
> > > Strangely, the above godbolt link dropped the output windows, here's a
> > > different link that shows the behavioral differences between C and C++
> > > mode in GCC: https://godbolt.org/z/R3zRHe
> > Hmm, I'll have a look at this.
> That gcc godbolt is a bit odd. The type of the bitfield expression in the C++
> example is `long` and not `int`, but in Clang, it's clearly being converted.
> If I change the example a bit, we get this warning:
> ```
> :11:12: warning: format '%d' expects argument of type 'int', but
> argument 2 has type 'long int' [-Wformat=]
>11 | printf("%d", bf.a); // expected-warning {{format specifies type
> 'long' but the argument has type 'int'}}
> | ~^
> || |
> |intlong int
> ```
> But in Clang, we get a cast to `int`:
> ```
> | `-ImplicitCastExpr 0xd190748 'int'
> | `-ImplicitCastExpr 0xd190730 'long'
> | `-MemberExpr 0xd190618 'long' lvalue bitfield .a
> 0xd18f790
> | `-DeclRefExpr 0xd1905f8 'struct bitfields':'bitfields'
> lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
> ```
>
> So gcc and Clang are doing things differently here.
>
> The code in `isPromotableBitField` says:
> ```
> // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
> //We perform that promotion here to match GCC and C++.
> ```
> but clearly gcc isn't doing this in the C++ case. The comments also mention
> some things about gcc bugs that Clang does not follow, but that's in
> reference to a C DR.
C++ disallows the rank conversion from int to long as well. [conv.prom]p1 does
not apply because `long int` has a higher rank than `int`, but [conv.prom]p5
allows the promotion if the range of values is identical between the two types.
C makes this UB in several ways -- you can't have a bit-field whose type is
something other than int, unsigned int, or _Bool (6.7.2.1p5) or promoting from
types other than those (6.3.1.1p2), but otherwise matches the C++ behavior in
terms of promotion (including the rank conversion).
You may have to dig further into what Clang is doing, but I would guess that
the diagnostics should be triggered in both C and C++ similarly.
Ultimately, I'd like to see tests for cases where `sizeof(int) ==
sizeof(long)`, `sizeof(int) != sizeof(long)`, and variants for C and C++ of
each.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan marked an inline comment as done.
ebevhan added inline comments.
Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
ebevhan wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Running your test through GCC looks like the behavior matches here for C;
> > > can you also add a C++ test that demonstrates the behavior does not
> > > change?
> > >
> > > https://godbolt.org/z/zRYDMG
> > Strangely, the above godbolt link dropped the output windows, here's a
> > different link that shows the behavioral differences between C and C++ mode
> > in GCC: https://godbolt.org/z/R3zRHe
> Hmm, I'll have a look at this.
That gcc godbolt is a bit odd. The type of the bitfield expression in the C++
example is `long` and not `int`, but in Clang, it's clearly being converted. If
I change the example a bit, we get this warning:
```
:11:12: warning: format '%d' expects argument of type 'int', but
argument 2 has type 'long int' [-Wformat=]
11 | printf("%d", bf.a); // expected-warning {{format specifies type
'long' but the argument has type 'int'}}
| ~^
|| |
|intlong int
```
But in Clang, we get a cast to `int`:
```
| `-ImplicitCastExpr 0xd190748 'int'
| `-ImplicitCastExpr 0xd190730 'long'
| `-MemberExpr 0xd190618 'long' lvalue bitfield .a
0xd18f790
| `-DeclRefExpr 0xd1905f8 'struct bitfields':'bitfields'
lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
```
So gcc and Clang are doing things differently here.
The code in `isPromotableBitField` says:
```
// FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
//We perform that promotion here to match GCC and C++.
```
but clearly gcc isn't doing this in the C++ case. The comments also mention
some things about gcc bugs that Clang does not follow, but that's in reference
to a C DR.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan marked 2 inline comments as done. ebevhan added inline comments. Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + aaron.ballman wrote: > aaron.ballman wrote: > > Running your test through GCC looks like the behavior matches here for C; > > can you also add a C++ test that demonstrates the behavior does not change? > > > > https://godbolt.org/z/zRYDMG > Strangely, the above godbolt link dropped the output windows, here's a > different link that shows the behavioral differences between C and C++ mode > in GCC: https://godbolt.org/z/R3zRHe Hmm, I'll have a look at this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7711-7715 + // It's an integer promotion if the destination type is the promoted + // source type. + return ICE->getCastKind() == CK_IntegralCast && + From->isPromotableIntegerType() && + S.Context.getPromotedIntegerType(From) == To; ebevhan wrote: > aaron.ballman wrote: > > This check is not checking against the promoted type of the bit-field. See > > `Sema::UsualArithmeticConversions()` for an example of what I'm talking > > about. Is that expected? > I'm not entirely sure what you mean. Are you referring to the type produced > by `isPromotableBitField`? The source type of that promotion is what we don't > want to see in these implicit casts. > > We don't want to look through promotions here if we promoted from a type > which was the result of a bitfield promotion, and that bitfield promotion was > from a higher ranked type to a lower ranked type. so, if we have a bitfield > of type `short`, then promoting that to `int` is fine, and we will give a > warning for the `short`. But if the type of the bitfield is `long`, it could > be promoted to `int`. However, the format specifier warning will look through > these promotions and think that we passed an expression of `long` to the > function even though it was `int`. Ahhh, okay, I see what's going on then. This makes more sense to me now, thank you for the explanation. Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + aaron.ballman wrote: > Running your test through GCC looks like the behavior matches here for C; can > you also add a C++ test that demonstrates the behavior does not change? > > https://godbolt.org/z/zRYDMG Strangely, the above godbolt link dropped the output windows, here's a different link that shows the behavioral differences between C and C++ mode in GCC: https://godbolt.org/z/R3zRHe CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan marked an inline comment as done. ebevhan added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7711-7715 + // It's an integer promotion if the destination type is the promoted + // source type. + return ICE->getCastKind() == CK_IntegralCast && + From->isPromotableIntegerType() && + S.Context.getPromotedIntegerType(From) == To; aaron.ballman wrote: > This check is not checking against the promoted type of the bit-field. See > `Sema::UsualArithmeticConversions()` for an example of what I'm talking > about. Is that expected? I'm not entirely sure what you mean. Are you referring to the type produced by `isPromotableBitField`? The source type of that promotion is what we don't want to see in these implicit casts. We don't want to look through promotions here if we promoted from a type which was the result of a bitfield promotion, and that bitfield promotion was from a higher ranked type to a lower ranked type. so, if we have a bitfield of type `short`, then promoting that to `int` is fine, and we will give a warning for the `short`. But if the type of the bitfield is `long`, it could be promoted to `int`. However, the format specifier warning will look through these promotions and think that we passed an expression of `long` to the function even though it was `int`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7711-7715 + // It's an integer promotion if the destination type is the promoted + // source type. + return ICE->getCastKind() == CK_IntegralCast && + From->isPromotableIntegerType() && + S.Context.getPromotedIntegerType(From) == To; This check is not checking against the promoted type of the bit-field. See `Sema::UsualArithmeticConversions()` for an example of what I'm talking about. Is that expected? Comment at: test/Sema/format-strings-bitfield-promotion.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s + Running your test through GCC looks like the behavior matches here for C; can you also add a C++ test that demonstrates the behavior does not change? https://godbolt.org/z/zRYDMG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan updated this revision to Diff 176130.
ebevhan added a comment.
Rebased and addressed review comments.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51211/new/
https://reviews.llvm.org/D51211
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings-bitfield-promotion.c
Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's a floating promotion if the source type is a lower rank.
+ if (ICE->getCastKind() == CK_FloatingCast &&
+ S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+ // It's an integer promotion if the destination type is the promoted
+ // source type.
+ return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;
+}
+
bool
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier ,
const char *StartSpecifier,
@@ -7724,11 +7742,11 @@
// Look through argument promotions for our error message's reported type.
// This includes the integral and floating promotions, but excludes array
- // and function pointer decay; seeing that an argument intended to be a
- // string has type 'char [6]' is probably more confusing than 'char *'.
+ // and function pointer decay (seeing that an argument intended to be a
+ // string has type 'char [6]' is probably more confusing than 'char *') and
+ // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
E = ICE->getSubExpr();
ExprTy = E->getType();
Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's a floating promotion if the source type is a lower rank.
+ if (ICE->getCastKind() == CK_FloatingCast &&
+
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:7598-7602 + if (ICE->getCastKind() == CK_IntegralCast && + From->isPromotableIntegerType() && + S.Context.getPromotedIntegerType(From) == To) +return true; + return false; Can be simplified to: `return ICE->getCastKind() == CK_IntegralCast && From->isPromotableIntegerType() && S.Context.getPromotedIntegerType(From) == To;` Comment at: test/Sema/format-strings.c:699-700 + unsigned long b : 2; + long c : 32; // assumes that int is 32 bits + unsigned long d : 32; // assumes that int is 32 bits +} bf; The test currently does not have a target triple, so this is likely to fail on some bots. Rather than tie this entire test to one architecture, I'd split the bit-field tests out into a separate file and pick an explicit triple that meets this assumption. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added a comment. Another ping. Anyone up for reviewing this patch? Repository: rC Clang https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.
ebevhan created this revision.
ebevhan added reviewers: krememek, rsmith.
Herald added a subscriber: cfe-commits.
The integer promotions apply to bitfields as well, but
rather oddly. If you have a bitfield with a lower width
than int, then the type of the member expression will
be int regardless of the type of the bitfield member.
This means that you can effectively get 'type demotion'
in a bitfield member expression.
However, when analyzing format string types, we
would look through argument promotions like integer
promotion. Looking through bitfield demotion means
that we would get the wrong type when analyzing,
hiding -Wformat issues.
This patch fixes this so that we only explicitly look
through integer and floating point promotion where
the result type is actually a promotion.
Repository:
rC Clang
https://reviews.llvm.org/D51211
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings.c
Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
// This caused crashes due to invalid casts.
printf(1 > 0); // expected-warning{{format string is not a string literal}}
expected-warning{{incompatible integer to pointer conversion}}
expected-note@format-strings.c:*{{passing argument to parameter here}}
expected-note{{to avoid this}}
}
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'int'}}
+ printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but
the argument has type 'int'}}
+ printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7582,6 +7582,26 @@
return std::make_pair(QualType(), StringRef());
}
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema , const ImplicitCastExpr *ICE) {
+ QualType From = ICE->getSubExpr()->getType();
+ QualType To = ICE->getType();
+ // It's a floating promotion if the source type is a lower rank.
+ if (ICE->getCastKind() == CK_FloatingCast &&
+ S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+ // It's an integer promotion if the destination type is the promoted
+ // source type.
+ if (ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To)
+return true;
+ return false;
+}
+
bool
CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier ,
const char *StartSpecifier,
@@ -7609,11 +7629,11 @@
// Look through argument promotions for our error message's reported type.
// This includes the integral and floating promotions, but excludes array
- // and function pointer decay; seeing that an argument intended to be a
- // string has type 'char [6]' is probably more confusing than 'char *'.
+ // and function pointer decay (seeing that an argument intended to be a
+ // string has type 'char [6]' is probably more confusing than 'char *') and
+ // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
E = ICE->getSubExpr();
ExprTy = E->getType();
Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
// This caused crashes due to invalid casts.
printf(1 > 0); // expected-warning{{format string is not a string literal}} expected-warning{{incompatible integer to pointer conversion}} expected-note@format-strings.c:*{{passing argument to parameter here}} expected-note{{to avoid this}}
}
+
+struct bitfields {
+ long a : 2;
+ unsigned long b : 2;
+ long c : 32; // assumes that int is 32 bits
+ unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+ printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+ printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' but
