[cfe-commits] PATCH: Large re-working of bitfield IR-gen, and a fix for PR13691

[Richard Smith]

On Tue, Sep 11, 2012 at 10:52 PM, John McCall <rjmccall at apple.com> wrote:

>
> On Sep 11, 2012, at 8:19 PM, Richard Smith wrote:
>
> On Tue, Sep 11, 2012 at 6:47 PM, John McCall <rjmccall at apple.com> wrote:
>
>> On Sep 11, 2012, at 6:29 PM, Chandler Carruth wrote:
>>
>> On Tue, Sep 11, 2012 at 6:09 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>>
>>> On Tue, Sep 11, 2012 at 6:04 PM, John McCall <rjmccall at apple.com> wrote:
>>>
>>>> On Sep 11, 2012, at 4:41 PM, Richard Smith wrote:
>>>> > On Tue, Sep 11, 2012 at 11:33 AM, John McCall <rjmccall at apple.com>
>>>> wrote:
>>>> > On Sep 11, 2012, at 11:07 AM, Chandler Carruth wrote:
>>>> > > On Fri, Aug 31, 2012 at 1:01 PM, Eli Friedman <
>>>> eli.friedman at gmail.com> wrote:
>>>> > >> Another nasty case I just thought of:
>>>> > >>
>>>> > >> struct x { int i : 24; };
>>>> > >> struct y { int i : 24; char j; };
>>>> > >> union z {
>>>> > >>   struct x x;
>>>> > >>   struct y y;
>>>> > >> };
>>>> > >> union z a;
>>>> > >> void f(int i) {
>>>> > >>   a.x.i = i;
>>>> > >> }
>>>> > >> void g(char j) {
>>>> > >>   a.y.j = j
>>>> > >> }
>>>> > >>
>>>> > >> The two writes are to separate memory locations. :)
>>>> > >
>>>> > > Wait, hold on... I'm deeply confused. Maybe because I don't know
>>>> how C11 unions work?
>>>> > >
>>>> > > With C++11, only one side of the union is allowed to be active, and
>>>> so I don't think they are separate memory locations?
>>>> >
>>>> > I agree that this isn't a problem, but the analysis is a bit more
>>>> complicated;
>>>> > it hinges on the fact that it's okay to *read* from an inactive union
>>>> member
>>>> > under the common-prefix exception, but it's not okay to *write* to
>>>> it.  The
>>>> > same analysis applies in both standards:
>>>> >
>>>> > Is this still OK if the extra union member is volatile? Chandler and
>>>> I have discussed this, and it's far from clear that it would be. (In
>>>> particular, we can conceive of a seemingly-reasonable case where the union
>>>> sits on an MMIO port, and only the fourth byte has volatile semantics.)
>>>>
>>>> I see no reason why making the member volatile changes anything.
>>>> Other members in the union can be assumed not to exist, because the
>>>> active union member *must* be the one we're assigning to — indeed,
>>>> in C such an assignment is how you change the active union member.
>>>>
>>>
>>> I'm talking about the load-widening case, not the store-widening. Given:
>>>
>>> union U {
>>>   struct X { int a : 24; volatile char b; } x;
>>>   struct Y { int c : 24; } y;
>>> };
>>>
>>> ... if x is the active member, and we load y.c, can we really load from
>>> the storage location containing x.b?
>>>
>>
>> After going back and forth a few times amongst the committee members
>> here, we propose this:
>>
>> This is a defect. We should disallow reading from the common prefix of a
>> union using a non-active member if there are any volatile members following
>> the common prefix. There doesn't appear to be any way to support these
>> union types and common-prefix reads and the committee's express desire that
>> load widening is a valid compiler optimization.
>>
>> Reasons why this truly seems like a defect:
>>
>> 1) We can't even load widen through the padding bytes:
>>
>> union U {
>>   struct X { int a : 24; volatile char b; } x;
>>   struct Y { int a : 24; int c; } y;
>> };
>>
>> 2) We can't even load widen when the next field in all members after the
>> common prefix is non-volatile:
>>
>> union U {
>>   struct X { int a : 33; char b; volatile char c; } x;
>>   struct Y { int a : 33; } y;
>> };
>>
>>
>> Seem plausible?
>>
>>
>> I could accept this as a defect.  I don't think it's required.
>>
>> For example, I claim that, if we need x and z, we are allowed to use a
>> 32-bit
>> load here (although, granted, it might not be a good idea):
>>   struct [[align(4)]] A {
>>     char x;
>>     volatile char y;
>>     char z;
>>   };
>>
>
> [The common initial sequence exemption only applies to structs as union
> members, so that case is fine. Assuming these are each wrapped in a
> struct...]
>
>
> I think you misread what I wrote or assumed it away.  I wrote, and meant,
> a struct;  there's no union here.
>

Yes, you're absolutely right, I misread the 'struct' as 'union'. My
apologies.


> I am saying that I believe it is legal to widen a load so that it spans
> other data that happens to include a volatile object.  That is, it is
> perfectly legal to touch a volatile object when not requested to, as long
> as you (1) implement the actual requested accesses exactly as the abstract
> machine specifies, (2) don't introduce data races as covered by
> [intro.memory], and (3) don't violate your implementation-defined semantics
> for volatile accesses.
>

I see. C++ does not include C's statement that "what constitutes an access
to an object that has volatile-qualified type is implementation-defined",
but I suppose we can assume it's intended, based on the non-normative text
in [dcl.type.cv]p7 which says that the intent is to match C.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120912/bab95c4f/attachment.html>