Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

t;>>>> the
>>>>>>> new field as bitfield while this wasn't deemed necessary for the 
>>>>>>> existing
>>>>>>> bools. My best guess is that that they didn't want to add 3 bytes of
>>>>>>> padding (due to the int field), which seems like a decent reason.
>>>>>>>
>>>>>>> Had the warning been in place when this code got written, I suppose
>>>>>>> they had used ": 1" instead. Does this make this code much better? It
>>>>>>> doesn't seem like it to me. So after doing a warning quality eval, I'd
>>>>>>> suggest to not emit the warning for bool bitfields if the bitfield size 
>>>>>>> is
>>>>>>> < 8. (But since the warning fires only very rarely, I don't feel very
>>>>>>> strongly about this.)
>>>>>>>
>>>>>>
>>>>>> I agree it doesn't make the code /much/ better. But if I were reading
>>>>>> that, I would certainly pause for a few moments wondering what the author
>>>>>> was thinking. I also don't feel especially strongly about this, but I 
>>>>>> don't
>>>>>> see a good rationale for warning on 'bool : 9' but not on 'bool : 5'.
>>>>>>
>>>>>
>>>>> I'm coming around to the opinion that we shouldn't give this warning
>>>>> on bool at all -- the point of the warning is to point out that an
>>>>> 'unsigned : 40;' bitfield can't hold 2**40 - 1, and values of that size
>>>>> will be truncated. There is no corresponding problematic case for bool, so
>>>>> we have a much weaker justification for warning in this case -- we have no
>>>>> idea what the user was trying to achieve, but we do not have a signal that
>>>>> their code is wrong.
>>>>>
>>>>> Thoughts?
>>>>>
>>>>
>>>> Makes sense to me :-) What about `bool : 16`?
>>>>
>>>
>>> I don't think it makes sense to treat bool : 3 and bool : 16
>>> differently. The fact that an unadorned bool would occupy 8 bits doesn't
>>> seem relevant to whether we should warn. Either we warn that there are
>>> padding bits, or we don't.
>>>
>>
>> Yup, makes sense.
>>
>>
>>>
>>>
>>>> , but it doesn't seem likely they got that effect. Would you be more
>>>>>>>> convinced if we amended the diagnostic to provide a fixit suggesting 
>>>>>>>> using
>>>>>>>> an anonymous bit-field to insert padding?
>>>>>>>>
>>>>>>>
>>>>>>> Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on
>>>>>>> the compiler to figure out padding?
>>>>>>>
>>>>>>
>>>>>> It depends; maybe the intent is to be compatible with some on-disk
>>>>>> format, and the explicit padding is important:
>>>>>>
>>>>>> struct X {
>>>>>>   int n : 3;
>>>>>>   bool b : 3;
>>>>>>   int n : 2;
>>>>>> };
>>>>>>
>>>>>> Changing the bool bit-field to 1 bit without inserting an anonymous
>>>>>> bit-field would change the struct layout.
>>>>>>
>>>>>>
>>>>>>> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <
>>>>>>>>> rich...@metafoo.co.uk> wrote:
>>>>>>>>>
>>>>>>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> As of DR262, the C standard clarified that the width of a
>>>>>>>>>>> bit-field can not exceed that of the specified type, and this 
>>>>>>>>>>> change was
>>>>>>>>>>> primarily to ensure that Clang correctly enforced this part of the
>>>>>>>>>>> standard. Looking at the C++11 standard again, it states that 
>>>>>>>>>>> although the
>>>>>>>>>>> specified width of a bit-field may exceed the number of bits in the 
>>>>>>>>>>> *object
>>>>>>>>>>> representation* (

Re: r247618 - C11 _Bool bitfield diagnostic

[Alexey Samsonov via cfe-commits]

this make this code much better? It
>>>>>> doesn't seem like it to me. So after doing a warning quality eval, I'd
>>>>>> suggest to not emit the warning for bool bitfields if the bitfield size 
>>>>>> is
>>>>>> < 8. (But since the warning fires only very rarely, I don't feel very
>>>>>> strongly about this.)
>>>>>>
>>>>>
>>>>> I agree it doesn't make the code /much/ better. But if I were reading
>>>>> that, I would certainly pause for a few moments wondering what the author
>>>>> was thinking. I also don't feel especially strongly about this, but I 
>>>>> don't
>>>>> see a good rationale for warning on 'bool : 9' but not on 'bool : 5'.
>>>>>
>>>>
>>>> I'm coming around to the opinion that we shouldn't give this warning on
>>>> bool at all -- the point of the warning is to point out that an 'unsigned :
>>>> 40;' bitfield can't hold 2**40 - 1, and values of that size will be
>>>> truncated. There is no corresponding problematic case for bool, so we have
>>>> a much weaker justification for warning in this case -- we have no idea
>>>> what the user was trying to achieve, but we do not have a signal that their
>>>> code is wrong.
>>>>
>>>> Thoughts?
>>>>
>>>
>>> Makes sense to me :-) What about `bool : 16`?
>>>
>>
>> I don't think it makes sense to treat bool : 3 and bool : 16 differently.
>> The fact that an unadorned bool would occupy 8 bits doesn't seem relevant
>> to whether we should warn. Either we warn that there are padding bits, or
>> we don't.
>>
>
> Yup, makes sense.
>
>
>>
>>
>>> , but it doesn't seem likely they got that effect. Would you be more
>>>>>>> convinced if we amended the diagnostic to provide a fixit suggesting 
>>>>>>> using
>>>>>>> an anonymous bit-field to insert padding?
>>>>>>>
>>>>>>
>>>>>> Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on
>>>>>> the compiler to figure out padding?
>>>>>>
>>>>>
>>>>> It depends; maybe the intent is to be compatible with some on-disk
>>>>> format, and the explicit padding is important:
>>>>>
>>>>> struct X {
>>>>>   int n : 3;
>>>>>   bool b : 3;
>>>>>   int n : 2;
>>>>> };
>>>>>
>>>>> Changing the bool bit-field to 1 bit without inserting an anonymous
>>>>> bit-field would change the struct layout.
>>>>>
>>>>>
>>>>>> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <
>>>>>>>> rich...@metafoo.co.uk> wrote:
>>>>>>>>
>>>>>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> As of DR262, the C standard clarified that the width of a
>>>>>>>>>> bit-field can not exceed that of the specified type, and this change 
>>>>>>>>>> was
>>>>>>>>>> primarily to ensure that Clang correctly enforced this part of the
>>>>>>>>>> standard. Looking at the C++11 standard again, it states that 
>>>>>>>>>> although the
>>>>>>>>>> specified width of a bit-field may exceed the number of bits in the 
>>>>>>>>>> *object
>>>>>>>>>> representation* (which includes padding bits) of the specified
>>>>>>>>>> type, the extra bits will not take any part in the bit-field's *value
>>>>>>>>>> representation*.
>>>>>>>>>>
>>>>>>>>>> Taking this into account, it seems that the correct way to
>>>>>>>>>> validate the width of a bit-field (ignoring the special case of MS 
>>>>>>>>>> in C
>>>>>>>>>> mode) would be to use getIntWidth in C mode, and getTypeSize in C++ 
>>>>>>>>>> mode.
>>>>>>>>>>
>>>>>>>>>> I would be happy create a patch to make this change tomorrow if
>>>>>>>>>

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

>>
>>>
>>> I'm coming around to the opinion that we shouldn't give this warning on
>>> bool at all -- the point of the warning is to point out that an 'unsigned :
>>> 40;' bitfield can't hold 2**40 - 1, and values of that size will be
>>> truncated. There is no corresponding problematic case for bool, so we have
>>> a much weaker justification for warning in this case -- we have no idea
>>> what the user was trying to achieve, but we do not have a signal that their
>>> code is wrong.
>>>
>>> Thoughts?
>>>
>>
>> Makes sense to me :-) What about `bool : 16`?
>>
>
> I don't think it makes sense to treat bool : 3 and bool : 16 differently.
> The fact that an unadorned bool would occupy 8 bits doesn't seem relevant
> to whether we should warn. Either we warn that there are padding bits, or
> we don't.
>

Yup, makes sense.


>
>
>> , but it doesn't seem likely they got that effect. Would you be more
>>>>>> convinced if we amended the diagnostic to provide a fixit suggesting 
>>>>>> using
>>>>>> an anonymous bit-field to insert padding?
>>>>>>
>>>>>
>>>>> Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on the
>>>>> compiler to figure out padding?
>>>>>
>>>>
>>>> It depends; maybe the intent is to be compatible with some on-disk
>>>> format, and the explicit padding is important:
>>>>
>>>> struct X {
>>>>   int n : 3;
>>>>   bool b : 3;
>>>>   int n : 2;
>>>> };
>>>>
>>>> Changing the bool bit-field to 1 bit without inserting an anonymous
>>>> bit-field would change the struct layout.
>>>>
>>>>
>>>>> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk
>>>>>>> > wrote:
>>>>>>>
>>>>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> As of DR262, the C standard clarified that the width of a
>>>>>>>>> bit-field can not exceed that of the specified type, and this change 
>>>>>>>>> was
>>>>>>>>> primarily to ensure that Clang correctly enforced this part of the
>>>>>>>>> standard. Looking at the C++11 standard again, it states that 
>>>>>>>>> although the
>>>>>>>>> specified width of a bit-field may exceed the number of bits in the 
>>>>>>>>> *object
>>>>>>>>> representation* (which includes padding bits) of the specified
>>>>>>>>> type, the extra bits will not take any part in the bit-field's *value
>>>>>>>>> representation*.
>>>>>>>>>
>>>>>>>>> Taking this into account, it seems that the correct way to
>>>>>>>>> validate the width of a bit-field (ignoring the special case of MS in 
>>>>>>>>> C
>>>>>>>>> mode) would be to use getIntWidth in C mode, and getTypeSize in C++ 
>>>>>>>>> mode.
>>>>>>>>>
>>>>>>>>> I would be happy create a patch to make this change tomorrow if
>>>>>>>>> people are in agreement.
>>>>>>>>>
>>>>>>>> David Majnemer has already landed a couple of changes to fix this
>>>>>>>> up, so hopefully that won't be necessary. Thanks for working on this!
>>>>>>>>
>>>>>>>>> Rachel
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> [image: Inactive hide details for Nico Weber ---09/14/2015
>>>>>>>>> 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith
>>>>>>>>> <richard@metafo]Nico Weber ---09/14/2015 09:53:25 PM---On Mon,
>>>>>>>>> Sep 14, 2015 at 5:28 PM, Richard Smith <rich...@metafoo.co.uk>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> From: Nico Weber <tha...@chromium.org>
>>>>>>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>>>>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-comm

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

gt;>>>
>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>> wrote:
>>>>>
>>>>>> As of DR262, the C standard clarified that the width of a bit-field
>>>>>> can not exceed that of the specified type, and this change was primarily 
>>>>>> to
>>>>>> ensure that Clang correctly enforced this part of the standard. Looking 
>>>>>> at
>>>>>> the C++11 standard again, it states that although the specified width of 
>>>>>> a
>>>>>> bit-field may exceed the number of bits in the *object
>>>>>> representation* (which includes padding bits) of the specified type,
>>>>>> the extra bits will not take any part in the bit-field's *value
>>>>>> representation*.
>>>>>>
>>>>>> Taking this into account, it seems that the correct way to validate
>>>>>> the width of a bit-field (ignoring the special case of MS in C mode) 
>>>>>> would
>>>>>> be to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>>>>
>>>>>> I would be happy create a patch to make this change tomorrow if
>>>>>> people are in agreement.
>>>>>>
>>>>> David Majnemer has already landed a couple of changes to fix this up,
>>>>> so hopefully that won't be necessary. Thanks for working on this!
>>>>>
>>>>>> Rachel
>>>>>>
>>>>>>
>>>>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>>>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>>>>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, 
>>>>>> Richard
>>>>>> Smith <rich...@metafoo.co.uk> wrote:
>>>>>>
>>>>>> From: Nico Weber <tha...@chromium.org>
>>>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>>>>> cfe-commits@lists.llvm.org>
>>>>>> Date: 09/14/2015 09:53 PM
>>>>>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>>>>>> Sent by: tha...@google.com
>>>>>> --
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <
>>>>>> *rich...@metafoo.co.uk* <rich...@metafoo.co.uk>> wrote:
>>>>>>
>>>>>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>>>>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>>>>>   This also fires for bool in C++ files, even though the commit
>>>>>>   message saying C11 and _Bool. Given the test changes, I suppose 
>>>>>> that's
>>>>>>   intentional? This fires a lot on existing code, for example 
>>>>>> protobuf:
>>>>>>
>>>>>>   
>>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>>>>>   error: width of bit-field 'is_cleared' (4 bits) exceeds the width 
>>>>>> of its
>>>>>>   type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>>>>   bool is_cleared : 4;
>>>>>>^
>>>>>>   
>>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>>>>>   error: width of bit-field 'is_lazy' (4 bits) exceeds the width of 
>>>>>> its type;
>>>>>>   value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>>>>   bool is_lazy : 4;
>>>>>>^
>>>>>>
>>>>>>   Is this expected? Is this a behavior change, or did the
>>>>>>   truncation happen previously and it's now just getting warned on?
>>>>>>
>>>>>>The code previously assumed that MSVC used the C rules here; it
>>>>>>appears that's not true in all cases.
>>>>>>
>>>>>>
>>>>>> This was on a Mac bot…
>>>>>>
>>>>>>
>>>>>>
>>>>>>Can we just remove the &

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

ke bool bitfields 1 wide and rely on the
>>> compiler to figure out padding?
>>>
>>
>> It depends; maybe the intent is to be compatible with some on-disk
>> format, and the explicit padding is important:
>>
>> struct X {
>>   int n : 3;
>>   bool b : 3;
>>   int n : 2;
>> };
>>
>> Changing the bool bit-field to 1 bit without inserting an anonymous
>> bit-field would change the struct layout.
>>
>>
>>> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk>
>>>>> wrote:
>>>>>
>>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>>> wrote:
>>>>>>
>>>>>>> As of DR262, the C standard clarified that the width of a bit-field
>>>>>>> can not exceed that of the specified type, and this change was 
>>>>>>> primarily to
>>>>>>> ensure that Clang correctly enforced this part of the standard. Looking 
>>>>>>> at
>>>>>>> the C++11 standard again, it states that although the specified width 
>>>>>>> of a
>>>>>>> bit-field may exceed the number of bits in the *object
>>>>>>> representation* (which includes padding bits) of the specified
>>>>>>> type, the extra bits will not take any part in the bit-field's *value
>>>>>>> representation*.
>>>>>>>
>>>>>>> Taking this into account, it seems that the correct way to validate
>>>>>>> the width of a bit-field (ignoring the special case of MS in C mode) 
>>>>>>> would
>>>>>>> be to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>>>>>
>>>>>>> I would be happy create a patch to make this change tomorrow if
>>>>>>> people are in agreement.
>>>>>>>
>>>>>> David Majnemer has already landed a couple of changes to fix this up,
>>>>>> so hopefully that won't be necessary. Thanks for working on this!
>>>>>>
>>>>>>> Rachel
>>>>>>>
>>>>>>>
>>>>>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>>>>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>>>>>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, 
>>>>>>> Richard
>>>>>>> Smith <rich...@metafoo.co.uk> wrote:
>>>>>>>
>>>>>>> From: Nico Weber <tha...@chromium.org>
>>>>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>>>>>> cfe-commits@lists.llvm.org>
>>>>>>> Date: 09/14/2015 09:53 PM
>>>>>>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>>>>>>> Sent by: tha...@google.com
>>>>>>> --
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <
>>>>>>> *rich...@metafoo.co.uk* <rich...@metafoo.co.uk>> wrote:
>>>>>>>
>>>>>>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>>>>>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>>>>>>   This also fires for bool in C++ files, even though the commit
>>>>>>>   message saying C11 and _Bool. Given the test changes, I suppose 
>>>>>>> that's
>>>>>>>   intentional? This fires a lot on existing code, for example 
>>>>>>> protobuf:
>>>>>>>
>>>>>>>   
>>>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>>>>>>   error: width of bit-field 'is_cleared' (4 bits) exceeds the width 
>>>>>>> of its
>>>>>>>   type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>>>>>   bool is_cleared : 4;
>>>>>>>^
>>>>>>>   
>>>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>>>>&g

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

but we do not have a signal that their
>> code is wrong.
>>
>> Thoughts?
>>
>
> Makes sense to me :-) What about `bool : 16`?
>

I don't think it makes sense to treat bool : 3 and bool : 16 differently.
The fact that an unadorned bool would occupy 8 bits doesn't seem relevant
to whether we should warn. Either we warn that there are padding bits, or
we don't.


> , but it doesn't seem likely they got that effect. Would you be more
>>>>> convinced if we amended the diagnostic to provide a fixit suggesting using
>>>>> an anonymous bit-field to insert padding?
>>>>>
>>>>
>>>> Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on the
>>>> compiler to figure out padding?
>>>>
>>>
>>> It depends; maybe the intent is to be compatible with some on-disk
>>> format, and the explicit padding is important:
>>>
>>> struct X {
>>>   int n : 3;
>>>   bool b : 3;
>>>   int n : 2;
>>> };
>>>
>>> Changing the bool bit-field to 1 bit without inserting an anonymous
>>> bit-field would change the struct layout.
>>>
>>>
>>>> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk>
>>>>>> wrote:
>>>>>>
>>>>>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> As of DR262, the C standard clarified that the width of a bit-field
>>>>>>>> can not exceed that of the specified type, and this change was 
>>>>>>>> primarily to
>>>>>>>> ensure that Clang correctly enforced this part of the standard. 
>>>>>>>> Looking at
>>>>>>>> the C++11 standard again, it states that although the specified width 
>>>>>>>> of a
>>>>>>>> bit-field may exceed the number of bits in the *object
>>>>>>>> representation* (which includes padding bits) of the specified
>>>>>>>> type, the extra bits will not take any part in the bit-field's *value
>>>>>>>> representation*.
>>>>>>>>
>>>>>>>> Taking this into account, it seems that the correct way to validate
>>>>>>>> the width of a bit-field (ignoring the special case of MS in C mode) 
>>>>>>>> would
>>>>>>>> be to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>>>>>>
>>>>>>>> I would be happy create a patch to make this change tomorrow if
>>>>>>>> people are in agreement.
>>>>>>>>
>>>>>>> David Majnemer has already landed a couple of changes to fix this
>>>>>>> up, so hopefully that won't be necessary. Thanks for working on this!
>>>>>>>
>>>>>>>> Rachel
>>>>>>>>
>>>>>>>>
>>>>>>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>>>>>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith 
>>>>>>>> <richard@metafo]Nico
>>>>>>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, 
>>>>>>>> Richard
>>>>>>>> Smith <rich...@metafoo.co.uk> wrote:
>>>>>>>>
>>>>>>>> From: Nico Weber <tha...@chromium.org>
>>>>>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>>>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>>>>>>> cfe-commits@lists.llvm.org>
>>>>>>>> Date: 09/14/2015 09:53 PM
>>>>>>>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>>>>>>>> Sent by: tha...@google.com
>>>>>>>> --
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <
>>>>>>>> *rich...@metafoo.co.uk* <rich...@metafoo.co.uk>> wrote:
>>>>>>>>
>>>>>>>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>>>>>>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>>
>>>>>>

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

ng the special case of MS in C mode) would
>>>>> be to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>>>
>>>>> I would be happy create a patch to make this change tomorrow if people
>>>>> are in agreement.
>>>>>
>>>> David Majnemer has already landed a couple of changes to fix this up,
>>>> so hopefully that won't be necessary. Thanks for working on this!
>>>>
>>>>> Rachel
>>>>>
>>>>>
>>>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>>>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
>>>>> Smith <rich...@metafoo.co.uk> wrote:
>>>>>
>>>>> From: Nico Weber <tha...@chromium.org>
>>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>>>> cfe-commits@lists.llvm.org>
>>>>> Date: 09/14/2015 09:53 PM
>>>>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>>>>> Sent by: tha...@google.com
>>>>> --
>>>>>
>>>>>
>>>>>
>>>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <
>>>>> *rich...@metafoo.co.uk* <rich...@metafoo.co.uk>> wrote:
>>>>>
>>>>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>>>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>>>>   This also fires for bool in C++ files, even though the commit
>>>>>   message saying C11 and _Bool. Given the test changes, I suppose 
>>>>> that's
>>>>>   intentional? This fires a lot on existing code, for example 
>>>>> protobuf:
>>>>>
>>>>>   
>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>>>>   error: width of bit-field 'is_cleared' (4 bits) exceeds the width 
>>>>> of its
>>>>>   type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>>>   bool is_cleared : 4;
>>>>>^
>>>>>   
>>>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>>>>   error: width of bit-field 'is_lazy' (4 bits) exceeds the width of 
>>>>> its type;
>>>>>   value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>>>   bool is_lazy : 4;
>>>>>^
>>>>>
>>>>>   Is this expected? Is this a behavior change, or did the
>>>>>   truncation happen previously and it's now just getting warned on?
>>>>>
>>>>>The code previously assumed that MSVC used the C rules here; it
>>>>>appears that's not true in all cases.
>>>>>
>>>>>
>>>>> This was on a Mac bot…
>>>>>
>>>>>
>>>>>
>>>>>Can we just remove the " || IsMsStruct
>>>>>|| Context.getTargetInfo().getCXXABI().isMicrosoft()"? Is there some 
>>>>> reason
>>>>>we need to prohibit overwide bitfields for MS bitfield layout, rather 
>>>>> than
>>>>>just warning on them? (Does record layout fail somehow?)
>>>>>On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
>>>>>   *cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>>
>>>>>   wrote:
>>>>>  Author: rcraik
>>>>>  Date: Mon Sep 14 16:27:36 2015
>>>>>  New Revision: 247618
>>>>>
>>>>>  URL:
>>>>>  *http://llvm.org/viewvc/llvm-project?rev=247618=rev*
>>>>>  <http://llvm.org/viewvc/llvm-project?rev=247618=rev>
>>>>>  Log:
>>>>>  C11 _Bool bitfield diagnostic
>>>>>
>>>>>  Summary: Implement DR262 (for C). This patch will mainly
>>>>>  affect bitfields of type _Bool
>>>>>
>>>>>  Reviewers: fraggamuffin, rsmith
>>>>>
>>>>>  Subscribers: hubert.reinterpretcast, cfe-commits
&g

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

On Tue, Sep 15, 2015 at 5:50 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:

> On Tue, Sep 15, 2015 at 12:38 PM, Nico Weber <tha...@chromium.org> wrote:
>
>> With this patch, we warn on `bool a : 4;`, yet we don't warn on `bool b`
>> (which has 8 bits storage, 1 bit value). Warning on `bool b` is silly of
>> course, but why is warning on `bool a : 4` useful? That's like 50% more
>> storage efficient than `bool b` ;-)
>>
>> It's possible that this is a good warning for some reason, but I don't
>> quite see why yet.
>>
>
> Why would we warn on "unsigned n : 57;"? The bit-field is wider than
> necessary, and we have no idea what the programmer was trying to do
>

Warning on this kind of makes sense to me, as the field is wider than the
default width of int. (Not warning on that doesn't seem terrible to me
either though.)

I'm only confused about the bool case with bitfield sizes < 8 I think. We
warn that the bitfield is wider than the value size, even though it's
smaller than the default storage size, and we don't warn on regular bools.

To get an idea how often this warning fires, I ran it on a large-ish open
source codebase I had flying around. The only place it fired on is one
header in protobuf (extension_set.h). I looked at the history of that file,
and it had a struct that used to look like

  struct Extension {
SomeEnum e;
bool a;
bool b;
bool c;
int d;
// ...some more stuff...
  };

Someone then added another field to this and for some reason decided to do
it like so:

  struct Extension {
SomeEnum e;
bool a;
bool b1 : 4;
bool b2 : 4;
bool c;
int d;
// ...some more stuff...
  };

Neither the commit message nor the review discussion mention the bitfield
at all as far as I can tell. Now, given that this isn't a small struct and
it has a bunch of normal bools, I don't know why they added the new field
as bitfield while this wasn't deemed necessary for the existing bools. My
best guess is that that they didn't want to add 3 bytes of padding (due to
the int field), which seems like a decent reason.

Had the warning been in place when this code got written, I suppose they
had used ": 1" instead. Does this make this code much better? It doesn't
seem like it to me. So after doing a warning quality eval, I'd suggest to
not emit the warning for bool bitfields if the bitfield size is < 8. (But
since the warning fires only very rarely, I don't feel very strongly about
this.)

, but it doesn't seem likely they got that effect. Would you be more
> convinced if we amended the diagnostic to provide a fixit suggesting using
> an anonymous bit-field to insert padding?
>

Isn't the Right Fix (tm) to make bool bitfields 1 wide and rely on the
compiler to figure out padding?


>
> On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk>
>> wrote:
>>
>>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com> wrote:
>>>
>>>> As of DR262, the C standard clarified that the width of a bit-field can
>>>> not exceed that of the specified type, and this change was primarily to
>>>> ensure that Clang correctly enforced this part of the standard. Looking at
>>>> the C++11 standard again, it states that although the specified width of a
>>>> bit-field may exceed the number of bits in the *object representation* 
>>>> (which
>>>> includes padding bits) of the specified type, the extra bits will not take
>>>> any part in the bit-field's *value representation*.
>>>>
>>>> Taking this into account, it seems that the correct way to validate the
>>>> width of a bit-field (ignoring the special case of MS in C mode) would be
>>>> to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>>
>>>> I would be happy create a patch to make this change tomorrow if people
>>>> are in agreement.
>>>>
>>> David Majnemer has already landed a couple of changes to fix this up, so
>>> hopefully that won't be necessary. Thanks for working on this!
>>>
>>>> Rachel
>>>>
>>>>
>>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
>>>> Smith <rich...@metafoo.co.uk> wrote:
>>>>
>>>> From: Nico Weber <tha...@chromium.org>
>>>> To: Richard Smith <rich...@metafoo.co.uk>
>>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>>> cfe-commits@lists.llvm.org>
>>>

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Tue, Sep 15, 2015 at 12:38 PM, Nico Weber <tha...@chromium.org> wrote:

> With this patch, we warn on `bool a : 4;`, yet we don't warn on `bool b`
> (which has 8 bits storage, 1 bit value). Warning on `bool b` is silly of
> course, but why is warning on `bool a : 4` useful? That's like 50% more
> storage efficient than `bool b` ;-)
>
> It's possible that this is a good warning for some reason, but I don't
> quite see why yet.
>

Why would we warn on "unsigned n : 57;"? The bit-field is wider than
necessary, and we have no idea what the programmer was trying to do, but it
doesn't seem likely they got that effect. Would you be more convinced if we
amended the diagnostic to provide a fixit suggesting using an anonymous
bit-field to insert padding?

On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk>
> wrote:
>
>> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com> wrote:
>>
>>> As of DR262, the C standard clarified that the width of a bit-field can
>>> not exceed that of the specified type, and this change was primarily to
>>> ensure that Clang correctly enforced this part of the standard. Looking at
>>> the C++11 standard again, it states that although the specified width of a
>>> bit-field may exceed the number of bits in the *object representation* 
>>> (which
>>> includes padding bits) of the specified type, the extra bits will not take
>>> any part in the bit-field's *value representation*.
>>>
>>> Taking this into account, it seems that the correct way to validate the
>>> width of a bit-field (ignoring the special case of MS in C mode) would be
>>> to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>>
>>> I would be happy create a patch to make this change tomorrow if people
>>> are in agreement.
>>>
>> David Majnemer has already landed a couple of changes to fix this up, so
>> hopefully that won't be necessary. Thanks for working on this!
>>
>>> Rachel
>>>
>>>
>>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
>>> Smith <rich...@metafoo.co.uk> wrote:
>>>
>>> From: Nico Weber <tha...@chromium.org>
>>> To: Richard Smith <rich...@metafoo.co.uk>
>>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>>> cfe-commits@lists.llvm.org>
>>> Date: 09/14/2015 09:53 PM
>>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>>> Sent by: tha...@google.com
>>> --
>>>
>>>
>>>
>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <*rich...@metafoo.co.uk*
>>> <rich...@metafoo.co.uk>> wrote:
>>>
>>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>>   This also fires for bool in C++ files, even though the commit
>>>   message saying C11 and _Bool. Given the test changes, I suppose that's
>>>   intentional? This fires a lot on existing code, for example protobuf:
>>>
>>>   ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>>   error: width of bit-field 'is_cleared' (4 bits) exceeds the width of 
>>> its
>>>   type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>   bool is_cleared : 4;
>>>^
>>>   ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>>   error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its 
>>> type;
>>>   value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>>   bool is_lazy : 4;
>>>^
>>>
>>>   Is this expected? Is this a behavior change, or did the
>>>   truncation happen previously and it's now just getting warned on?
>>>
>>>The code previously assumed that MSVC used the C rules here; it
>>>appears that's not true in all cases.
>>>
>>>
>>> This was on a Mac bot…
>>>
>>>
>>>
>>>Can we just remove the " || IsMsStruct
>>>|| Context.getTargetInfo().getCXXABI().isMicrosoft()"? Is there some 
>>> reason
>>>we need to prohibit overwide bitfields for MS bitfield layout, rather 
>>> than
>>&g

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

With this patch, we warn on `bool a : 4;`, yet we don't warn on `bool b`
(which has 8 bits storage, 1 bit value). Warning on `bool b` is silly of
course, but why is warning on `bool a : 4` useful? That's like 50% more
storage efficient than `bool b` ;-)

It's possible that this is a good warning for some reason, but I don't
quite see why yet.

On Mon, Sep 14, 2015 at 11:06 PM, Richard Smith <rich...@metafoo.co.uk>
wrote:

> On Mon, Sep 14, 2015 at 7:07 PM, Rachel Craik <rcr...@ca.ibm.com> wrote:
>
>> As of DR262, the C standard clarified that the width of a bit-field can
>> not exceed that of the specified type, and this change was primarily to
>> ensure that Clang correctly enforced this part of the standard. Looking at
>> the C++11 standard again, it states that although the specified width of a
>> bit-field may exceed the number of bits in the *object representation* (which
>> includes padding bits) of the specified type, the extra bits will not take
>> any part in the bit-field's *value representation*.
>>
>> Taking this into account, it seems that the correct way to validate the
>> width of a bit-field (ignoring the special case of MS in C mode) would be
>> to use getIntWidth in C mode, and getTypeSize in C++ mode.
>>
>> I would be happy create a patch to make this change tomorrow if people
>> are in agreement.
>>
> David Majnemer has already landed a couple of changes to fix this up, so
> hopefully that won't be necessary. Thanks for working on this!
>
>> Rachel
>>
>>
>> [image: Inactive hide details for Nico Weber ---09/14/2015 09:53:25
>> PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <richard@metafo]Nico
>> Weber ---09/14/2015 09:53:25 PM---On Mon, Sep 14, 2015 at 5:28 PM, Richard
>> Smith <rich...@metafoo.co.uk> wrote:
>>
>> From: Nico Weber <tha...@chromium.org>
>> To: Richard Smith <rich...@metafoo.co.uk>
>> Cc: Rachel Craik/Toronto/IBM@IBMCA, cfe-commits <
>> cfe-commits@lists.llvm.org>
>> Date: 09/14/2015 09:53 PM
>> Subject: Re: r247618 - C11 _Bool bitfield diagnostic
>> Sent by: tha...@google.com
>> --
>>
>>
>>
>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith <*rich...@metafoo.co.uk*
>> <rich...@metafoo.co.uk>> wrote:
>>
>>On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>>*cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>   This also fires for bool in C++ files, even though the commit
>>   message saying C11 and _Bool. Given the test changes, I suppose that's
>>   intentional? This fires a lot on existing code, for example protobuf:
>>
>>   ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>   error: width of bit-field 'is_cleared' (4 bits) exceeds the width of 
>> its
>>   type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>   bool is_cleared : 4;
>>^
>>   ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>   error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its 
>> type;
>>   value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>   bool is_lazy : 4;
>>^
>>
>>   Is this expected? Is this a behavior change, or did the truncation
>>   happen previously and it's now just getting warned on?
>>
>>The code previously assumed that MSVC used the C rules here; it
>>appears that's not true in all cases.
>>
>>
>> This was on a Mac bot…
>>
>>
>>
>>Can we just remove the " || IsMsStruct
>>|| Context.getTargetInfo().getCXXABI().isMicrosoft()"? Is there some 
>> reason
>>we need to prohibit overwide bitfields for MS bitfield layout, rather than
>>just warning on them? (Does record layout fail somehow?)
>>On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
>>   *cfe-commits@lists.llvm.org* <cfe-commits@lists.llvm.org>> wrote:
>>  Author: rcraik
>>  Date: Mon Sep 14 16:27:36 2015
>>  New Revision: 247618
>>
>>  URL: *http://llvm.org/viewvc/llvm-project?rev=247618=rev*
>>  <http://llvm.org/viewvc/llvm-project?rev=247618=rev>
>>  Log:
>>  C11 _Bool bitfield diagnostic
>>
>>  Summary: Implement DR262 (for C). This patch will mainly affect
>>  bitfields of type _Bool
>>
>>  Reviewers: fraggamuffin, rsmith
>>
>&

Re: r247618 - C11 _Bool bitfield diagnostic

[Nico Weber via cfe-commits]

This also fires for bool in C++ files, even though the commit message
saying C11 and _Bool. Given the test changes, I suppose that's intentional?
This fires a lot on existing code, for example protobuf:

../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
error: width of bit-field 'is_cleared' (4 bits) exceeds the width of its
type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
bool is_cleared : 4;
 ^
../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its type;
value will be truncated to 1 bit [-Werror,-Wbitfield-width]
bool is_lazy : 4;
 ^

Is this expected? Is this a behavior change, or did the truncation happen
previously and it's now just getting warned on?



On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rcraik
> Date: Mon Sep 14 16:27:36 2015
> New Revision: 247618
>
> URL: http://llvm.org/viewvc/llvm-project?rev=247618=rev
> Log:
> C11 _Bool bitfield diagnostic
>
> Summary: Implement DR262 (for C). This patch will mainly affect bitfields
> of type _Bool
>
> Reviewers: fraggamuffin, rsmith
>
> Subscribers: hubert.reinterpretcast, cfe-commits
>
> Differential Revision: http://reviews.llvm.org/D10018
>
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/test/CodeGen/bitfield-2.c
> cfe/trunk/test/CodeGenCXX/warn-padded-packed.cpp
> cfe/trunk/test/Misc/warning-flags.c
> cfe/trunk/test/Sema/bitfield.c
> cfe/trunk/test/SemaCXX/bitfield-layout.cpp
> cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
> cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
> cfe/trunk/test/SemaCXX/ms_wide_bitfield.cpp
> cfe/trunk/test/SemaObjC/class-bitfield.m
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=247618=247617=247618=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Sep 14 16:27:36
> 2015
> @@ -32,6 +32,7 @@ def AutoImport : DiagGroup<"auto-import"
>  def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
>  def GNUCompoundLiteralInitializer :
> DiagGroup<"gnu-compound-literal-initializer">;
>  def BitFieldConstantConversion :
> DiagGroup<"bitfield-constant-conversion">;
> +def BitFieldWidth : DiagGroup<"bitfield-width">;
>  def ConstantConversion :
>DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
>  def LiteralConversion : DiagGroup<"literal-conversion">;
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=247618=247617=247618=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Sep 14
> 16:27:36 2015
> @@ -4314,20 +4314,21 @@ def err_bitfield_has_negative_width : Er
>  def err_anon_bitfield_has_negative_width : Error<
>"anonymous bit-field has negative width (%0)">;
>  def err_bitfield_has_zero_width : Error<"named bit-field %0 has zero
> width">;
> -def err_bitfield_width_exceeds_type_size : Error<
> -  "size of bit-field %0 (%1 bits) exceeds size of its type (%2 bits)">;
> -def err_anon_bitfield_width_exceeds_type_size : Error<
> -  "size of anonymous bit-field (%0 bits) exceeds size of its type (%1
> bits)">;
> +def err_bitfield_width_exceeds_type_width : Error<
> +  "width of bit-field %0 (%1 bits) exceeds width of its type (%2
> bit%s2)">;
> +def err_anon_bitfield_width_exceeds_type_width : Error<
> +  "width of anonymous bit-field (%0 bits) exceeds width of its type "
> +  "(%1 bit%s1)">;
>  def err_incorrect_number_of_vector_initializers : Error<
>"number of elements must be either one or match the size of the
> vector">;
>
>  // Used by C++ which allows bit-fields that are wider than the type.
> -def warn_bitfield_width_exceeds_type_size: Warning<
> -  "size of bit-field %0 (%1 bits) exceeds the size of its type; value
> will be "
> -  "truncated to %2 bits">;
> -def warn_anon_bitfield_width_exceeds_type_size : Warning<
> -  "size of anonymous bit-field (%0 bits) exceeds size of its type; value
> will "
> -  "be truncated to %1 bits">;
> +def warn_bitfield_width_exceeds_type_width: Warning<
> +  "width of bit-field %0 (%1 bits) exceeds the width of its type; value
> will "
> +  "be truncated to %2 bit%s2">, InGroup;
> +def warn_anon_bitfield_width_exceeds_type_width : Warning<
> +  "width of anonymous bit-field (%0 bits) exceeds width of its type;
> value "

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 5:55 PM, David Majnemer 
wrote:

> On Mon, Sep 14, 2015 at 5:40 PM, Richard Smith 
> wrote:
>
>> On Mon, Sep 14, 2015 at 5:31 PM, David Majnemer > > wrote:
>>
>>> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> This also fires for bool in C++ files, even though the commit message
> saying C11 and _Bool. Given the test changes, I suppose that's 
> intentional?
> This fires a lot on existing code, for example protobuf:
>
> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
> error: width of bit-field 'is_cleared' (4 bits) exceeds the width of its
> type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
> bool is_cleared : 4;
>  ^
> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
> error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its 
> type;
> value will be truncated to 1 bit [-Werror,-Wbitfield-width]
> bool is_lazy : 4;
>  ^
>
> Is this expected? Is this a behavior change, or did the truncation
> happen previously and it's now just getting warned on?
>

 The code previously assumed that MSVC used the C rules here; it appears
 that's not true in all cases.

 Can we just remove the " || IsMsStruct || 
 Context.getTargetInfo().getCXXABI().isMicrosoft()"?
 Is there some reason we need to prohibit overwide bitfields for MS bitfield
 layout, rather than just warning on them? (Does record layout fail 
 somehow?)

>>>
>>> Record layout will fail for stuff like 'int x : 33' but will not fail
>>> for stuff like '{bool,_Bool} y : 2'; the bitfield layout algorithm cares
>>> about the storage size (which, of course, includes padding).  This patch
>>> makes 'bool y : 2' an error which is a behavior change, I'm working on
>>> patch to address this.
>>>
>>
>> Do we need to be compatible with this MSVC bug? It would seem feasible to
>> teach our MS record layout to do the right thing with over-wide bitfields
>> and downgrade this to the normal warning in C++.
>>
>
> It is possible to add support for over-wide bitfields but this is not
> completely trivial.  There is a fair amount of code in Clang which deals
> with emission of bitfields (RecordLayoutBuilder,
> CGRecordLayoutBuilder, ConstStructBuilder) and I don't think I have the
> bandwidth to audit the impact of such a change right now.
>

This all already works for Itanium record layout, so presumably only the
MS-layout-specific side would need to be implemented?

On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rcraik
>> Date: Mon Sep 14 16:27:36 2015
>> New Revision: 247618
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=247618=rev
>> Log:
>> C11 _Bool bitfield diagnostic
>>
>> Summary: Implement DR262 (for C). This patch will mainly affect
>> bitfields of type _Bool
>>
>> Reviewers: fraggamuffin, rsmith
>>
>> Subscribers: hubert.reinterpretcast, cfe-commits
>>
>> Differential Revision: http://reviews.llvm.org/D10018
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaDecl.cpp
>> cfe/trunk/test/CodeGen/bitfield-2.c
>> cfe/trunk/test/CodeGenCXX/warn-padded-packed.cpp
>> cfe/trunk/test/Misc/warning-flags.c
>> cfe/trunk/test/Sema/bitfield.c
>> cfe/trunk/test/SemaCXX/bitfield-layout.cpp
>> cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
>> cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
>> cfe/trunk/test/SemaCXX/ms_wide_bitfield.cpp
>> cfe/trunk/test/SemaObjC/class-bitfield.m
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=247618=247617=247618=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Sep 14
>> 16:27:36 2015
>> @@ -32,6 +32,7 @@ def AutoImport : DiagGroup<"auto-import"
>>  def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
>>  def GNUCompoundLiteralInitializer :
>> DiagGroup<"gnu-compound-literal-initializer">;
>>  def BitFieldConstantConversion :
>> DiagGroup<"bitfield-constant-conversion">;
>> +def BitFieldWidth : DiagGroup<"bitfield-width">;
>>  def ConstantConversion :
>>

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 6:10 PM, David Majnemer 
wrote:

> On Mon, Sep 14, 2015 at 6:01 PM, Richard Smith 
> wrote:
>
>> On Mon, Sep 14, 2015 at 5:55 PM, David Majnemer > > wrote:
>>
>>> On Mon, Sep 14, 2015 at 5:40 PM, Richard Smith 
>>> wrote:
>>>
 On Mon, Sep 14, 2015 at 5:31 PM, David Majnemer <
 david.majne...@gmail.com> wrote:

> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> This also fires for bool in C++ files, even though the commit
>>> message saying C11 and _Bool. Given the test changes, I suppose that's
>>> intentional? This fires a lot on existing code, for example protobuf:
>>>
>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>> error: width of bit-field 'is_cleared' (4 bits) exceeds the width of its
>>> type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>> bool is_cleared : 4;
>>>  ^
>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>> error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its 
>>> type;
>>> value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>> bool is_lazy : 4;
>>>  ^
>>>
>>> Is this expected? Is this a behavior change, or did the truncation
>>> happen previously and it's now just getting warned on?
>>>
>>
>> The code previously assumed that MSVC used the C rules here; it
>> appears that's not true in all cases.
>>
>> Can we just remove the " || IsMsStruct || 
>> Context.getTargetInfo().getCXXABI().isMicrosoft()"?
>> Is there some reason we need to prohibit overwide bitfields for MS 
>> bitfield
>> layout, rather than just warning on them? (Does record layout fail 
>> somehow?)
>>
>
> Record layout will fail for stuff like 'int x : 33' but will not fail
> for stuff like '{bool,_Bool} y : 2'; the bitfield layout algorithm cares
> about the storage size (which, of course, includes padding).  This patch
> makes 'bool y : 2' an error which is a behavior change, I'm working on
> patch to address this.
>

 Do we need to be compatible with this MSVC bug? It would seem feasible
 to teach our MS record layout to do the right thing with over-wide
 bitfields and downgrade this to the normal warning in C++.

>>>
>>> It is possible to add support for over-wide bitfields but this is not
>>> completely trivial.  There is a fair amount of code in Clang which deals
>>> with emission of bitfields (RecordLayoutBuilder,
>>> CGRecordLayoutBuilder, ConstStructBuilder) and I don't think I have the
>>> bandwidth to audit the impact of such a change right now.
>>>
>>
>> This all already works for Itanium record layout, so presumably only the
>> MS-layout-specific side would need to be implemented?
>>
>
> Bitfield management extends beyond ASTRecordLayout,
> CGRecordLowering::accumulateBitFIelds is entirely different when
> CGRecordLowering::isDiscreteBitFieldABI() is true.
>

OK, it sounds like that change would be more trouble than it's worth for
now.


> On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: rcraik
 Date: Mon Sep 14 16:27:36 2015
 New Revision: 247618

 URL: http://llvm.org/viewvc/llvm-project?rev=247618=rev
 Log:
 C11 _Bool bitfield diagnostic

 Summary: Implement DR262 (for C). This patch will mainly affect
 bitfields of type _Bool

 Reviewers: fraggamuffin, rsmith

 Subscribers: hubert.reinterpretcast, cfe-commits

 Differential Revision: http://reviews.llvm.org/D10018

 Modified:
 cfe/trunk/include/clang/Basic/DiagnosticGroups.td
 cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
 cfe/trunk/lib/Sema/SemaDecl.cpp
 cfe/trunk/test/CodeGen/bitfield-2.c
 cfe/trunk/test/CodeGenCXX/warn-padded-packed.cpp
 cfe/trunk/test/Misc/warning-flags.c
 cfe/trunk/test/Sema/bitfield.c
 cfe/trunk/test/SemaCXX/bitfield-layout.cpp
 cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
 cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
 cfe/trunk/test/SemaCXX/ms_wide_bitfield.cpp
 cfe/trunk/test/SemaObjC/class-bitfield.m

 Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=247618=247617=247618=diff

 

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 5:31 PM, David Majnemer 
wrote:

> On Mon, Sep 14, 2015 at 5:28 PM, Richard Smith via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> This also fires for bool in C++ files, even though the commit message
>>> saying C11 and _Bool. Given the test changes, I suppose that's intentional?
>>> This fires a lot on existing code, for example protobuf:
>>>
>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
>>> error: width of bit-field 'is_cleared' (4 bits) exceeds the width of its
>>> type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>> bool is_cleared : 4;
>>>  ^
>>> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
>>> error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its type;
>>> value will be truncated to 1 bit [-Werror,-Wbitfield-width]
>>> bool is_lazy : 4;
>>>  ^
>>>
>>> Is this expected? Is this a behavior change, or did the truncation
>>> happen previously and it's now just getting warned on?
>>>
>>
>> The code previously assumed that MSVC used the C rules here; it appears
>> that's not true in all cases.
>>
>> Can we just remove the " || IsMsStruct || 
>> Context.getTargetInfo().getCXXABI().isMicrosoft()"?
>> Is there some reason we need to prohibit overwide bitfields for MS bitfield
>> layout, rather than just warning on them? (Does record layout fail somehow?)
>>
>
> Record layout will fail for stuff like 'int x : 33' but will not fail for
> stuff like '{bool,_Bool} y : 2'; the bitfield layout algorithm cares about
> the storage size (which, of course, includes padding).  This patch makes
> 'bool y : 2' an error which is a behavior change, I'm working on patch to
> address this.
>

Do we need to be compatible with this MSVC bug? It would seem feasible to
teach our MS record layout to do the right thing with over-wide bitfields
and downgrade this to the normal warning in C++.


> On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: rcraik
 Date: Mon Sep 14 16:27:36 2015
 New Revision: 247618

 URL: http://llvm.org/viewvc/llvm-project?rev=247618=rev
 Log:
 C11 _Bool bitfield diagnostic

 Summary: Implement DR262 (for C). This patch will mainly affect
 bitfields of type _Bool

 Reviewers: fraggamuffin, rsmith

 Subscribers: hubert.reinterpretcast, cfe-commits

 Differential Revision: http://reviews.llvm.org/D10018

 Modified:
 cfe/trunk/include/clang/Basic/DiagnosticGroups.td
 cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
 cfe/trunk/lib/Sema/SemaDecl.cpp
 cfe/trunk/test/CodeGen/bitfield-2.c
 cfe/trunk/test/CodeGenCXX/warn-padded-packed.cpp
 cfe/trunk/test/Misc/warning-flags.c
 cfe/trunk/test/Sema/bitfield.c
 cfe/trunk/test/SemaCXX/bitfield-layout.cpp
 cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
 cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
 cfe/trunk/test/SemaCXX/ms_wide_bitfield.cpp
 cfe/trunk/test/SemaObjC/class-bitfield.m

 Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=247618=247617=247618=diff

 ==
 --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
 +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Sep 14
 16:27:36 2015
 @@ -32,6 +32,7 @@ def AutoImport : DiagGroup<"auto-import"
  def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
  def GNUCompoundLiteralInitializer :
 DiagGroup<"gnu-compound-literal-initializer">;
  def BitFieldConstantConversion :
 DiagGroup<"bitfield-constant-conversion">;
 +def BitFieldWidth : DiagGroup<"bitfield-width">;
  def ConstantConversion :
DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
  def LiteralConversion : DiagGroup<"literal-conversion">;

 Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=247618=247617=247618=diff

 ==
 --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
 +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Sep 14
 16:27:36 2015
 @@ -4314,20 +4314,21 @@ def err_bitfield_has_negative_width : Er
  def err_anon_bitfield_has_negative_width : Error<
"anonymous bit-field has negative width (%0)">;
  def err_bitfield_has_zero_width : Error<"named bit-field %0 has zero

Re: r247618 - C11 _Bool bitfield diagnostic

[Richard Smith via cfe-commits]

On Mon, Sep 14, 2015 at 5:18 PM, Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> This also fires for bool in C++ files, even though the commit message
> saying C11 and _Bool. Given the test changes, I suppose that's intentional?
> This fires a lot on existing code, for example protobuf:
>
> ../../third_party/protobuf/src/google/protobuf/extension_set.h:465:10:
> error: width of bit-field 'is_cleared' (4 bits) exceeds the width of its
> type; value will be truncated to 1 bit [-Werror,-Wbitfield-width]
> bool is_cleared : 4;
>  ^
> ../../third_party/protobuf/src/google/protobuf/extension_set.h:472:10:
> error: width of bit-field 'is_lazy' (4 bits) exceeds the width of its type;
> value will be truncated to 1 bit [-Werror,-Wbitfield-width]
> bool is_lazy : 4;
>  ^
>
> Is this expected? Is this a behavior change, or did the truncation happen
> previously and it's now just getting warned on?
>

The code previously assumed that MSVC used the C rules here; it appears
that's not true in all cases.

Can we just remove the " || IsMsStruct ||
Context.getTargetInfo().getCXXABI().isMicrosoft()"?
Is there some reason we need to prohibit overwide bitfields for MS bitfield
layout, rather than just warning on them? (Does record layout fail somehow?)

On Mon, Sep 14, 2015 at 2:27 PM, Rachel Craik via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rcraik
>> Date: Mon Sep 14 16:27:36 2015
>> New Revision: 247618
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=247618=rev
>> Log:
>> C11 _Bool bitfield diagnostic
>>
>> Summary: Implement DR262 (for C). This patch will mainly affect bitfields
>> of type _Bool
>>
>> Reviewers: fraggamuffin, rsmith
>>
>> Subscribers: hubert.reinterpretcast, cfe-commits
>>
>> Differential Revision: http://reviews.llvm.org/D10018
>>
>> Modified:
>> cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> cfe/trunk/lib/Sema/SemaDecl.cpp
>> cfe/trunk/test/CodeGen/bitfield-2.c
>> cfe/trunk/test/CodeGenCXX/warn-padded-packed.cpp
>> cfe/trunk/test/Misc/warning-flags.c
>> cfe/trunk/test/Sema/bitfield.c
>> cfe/trunk/test/SemaCXX/bitfield-layout.cpp
>> cfe/trunk/test/SemaCXX/constant-expression-cxx11.cpp
>> cfe/trunk/test/SemaCXX/constant-expression-cxx1y.cpp
>> cfe/trunk/test/SemaCXX/ms_wide_bitfield.cpp
>> cfe/trunk/test/SemaObjC/class-bitfield.m
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=247618=247617=247618=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Sep 14 16:27:36
>> 2015
>> @@ -32,6 +32,7 @@ def AutoImport : DiagGroup<"auto-import"
>>  def GNUBinaryLiteral : DiagGroup<"gnu-binary-literal">;
>>  def GNUCompoundLiteralInitializer :
>> DiagGroup<"gnu-compound-literal-initializer">;
>>  def BitFieldConstantConversion :
>> DiagGroup<"bitfield-constant-conversion">;
>> +def BitFieldWidth : DiagGroup<"bitfield-width">;
>>  def ConstantConversion :
>>DiagGroup<"constant-conversion", [ BitFieldConstantConversion ] >;
>>  def LiteralConversion : DiagGroup<"literal-conversion">;
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=247618=247617=247618=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Sep 14
>> 16:27:36 2015
>> @@ -4314,20 +4314,21 @@ def err_bitfield_has_negative_width : Er
>>  def err_anon_bitfield_has_negative_width : Error<
>>"anonymous bit-field has negative width (%0)">;
>>  def err_bitfield_has_zero_width : Error<"named bit-field %0 has zero
>> width">;
>> -def err_bitfield_width_exceeds_type_size : Error<
>> -  "size of bit-field %0 (%1 bits) exceeds size of its type (%2 bits)">;
>> -def err_anon_bitfield_width_exceeds_type_size : Error<
>> -  "size of anonymous bit-field (%0 bits) exceeds size of its type (%1
>> bits)">;
>> +def err_bitfield_width_exceeds_type_width : Error<
>> +  "width of bit-field %0 (%1 bits) exceeds width of its type (%2
>> bit%s2)">;
>> +def err_anon_bitfield_width_exceeds_type_width : Error<
>> +  "width of anonymous bit-field (%0 bits) exceeds width of its type "
>> +  "(%1 bit%s1)">;
>>  def err_incorrect_number_of_vector_initializers : Error<
>>"number of elements must be either one or match the size of the
>> vector">;
>>
>>  // Used by C++ which allows bit-fields that are wider than the type.
>> -def warn_bitfield_width_exceeds_type_size: Warning<
>> -  "size of bit-field %0 (%1 bits) exceeds the size of