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: > > > ``` > > > <source>: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'}} > > > | ~^ ~~~~ > > > | | | > > > | int long int > > > ``` > > > But in Clang, we get a cast to `int`: > > > ``` > > > | `-ImplicitCastExpr 0xd190748 <col:17, col:20> 'int' <IntegralCast> > > > | `-ImplicitCastExpr 0xd190730 <col:17, col:20> 'long' > > > <LValueToRValue> > > > | `-MemberExpr 0xd190618 <col:17, col:20> 'long' lvalue bitfield > > > .a 0xd18f790 > > > | `-DeclRefExpr 0xd1905f8 <col:17> '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
