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

[Richard Smith]

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...]

If 'y' is the active member of your union, then the intent of the standard
certainly seems to be that an access to 'x' or 'z' actually accesses the
object denoted by 'y'. Hence such an access is to a volatile object, but
the access is through a non-volatile glvalue, so behavior is undefined by
7.1.6.1/6. So we can load widen here when accessing 'x' or 'z'.

The same analysis can be applied to my example (and Chandler's examples)
too, with a bit of work. The standard doesn't explicitly say which bytes
are accessed by an lvalue-to-rvalue conversion on a non-class type, but the
implication is that a read of y.c (y.a in Chandler's examples) is permitted
to the entirety of the memory location containing it. So if we define the
memory location of y.c to be widenend to include padding (which is exactly
what I believe Chandler's current patch does), then a load of y.c has
undefined behavior if the active member of the union contains a volatile
subobject overlapping y.c, and in any other case, the widened load is
unobservable. We can use the same argument to widen loads of arbitrary
struct fields into padding, but not into adjacent fields (because memory
locations for fields cannot overlap).


> We are allowed to do this by dint of declaring "our implementation supports
> memory-mapped I/O ports but does not define semantics for aggregates
> which partially overlap them".  Voilà, the widened load can be assumed to
> have no side effects and is therefore permitted.
>

I'm not convinced by your argument: as-if doesn't apply to the observable
behavior of the program, so we can't assume the widened load has no
side-effects. However, since I've reached the same conclusion via a
different route, I don't think it's worth debating.
-- 
Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120911/f50074c2/attachment.html>