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

[Chandler Carruth]

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120911/b06d640b/attachment.html>