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:
> > ```
> > <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.


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

Reply via email to