Richard Biener wrote:
> On Tue, Sep 3, 2019 at 3:09 PM Ulrich Weigand <uweig...@de.ibm.com> wrote:
> > > If you read the C standards fine-print then yes. But people (or
> > > even the C frontend!) hardly get that correct since for example
> > > for
> > >
> > > struct __attribute__((packed)) { int i; } s;
> > >
> > > void foo ()
> > > {
> > > __builtin_printf ("%p", &s.i);
> > > }
> > >
> > > the C fronted actually creates a int * typed pointer for the ADDR_EXPR
> > > (and not an int * variant with 1-byte alignment). And people use
> > > int * to pass such pointers everywhere.
> >
> > Well ... I'd say if you cast to int * and then use an atomic operation,
> > it's your own fault that it fails :-/ If the frontend itself uses
> > the wrong type, that is of course a problem.
>
> Yes. The C standard says that if you cast something to a pointer type
> the pointer has to be aligned according to the pointed-to type, otherwise
> undefined. But we have no chance to use this because of this kind of
> issues (and of course developer laziness...).
But as far as I can see, for *atomic* operations at least, we do make
that assumption. The x86 back-end for example just assumes that any
"int" or "long" object that is the target of an atomic operation is
naturally aligned, or else the generated code would just crash. So
if you used your example with a packed struct and passed that pointer
to an atomic, the back-end would still assume the alignment and the
code would crash. But I'd still consider this a perfectly reasonable
and expected behavior in this case ...
The only thing that is special on s390 is that for the 16-byte integer
type, the "natural" (mode) alignment is only 8 bytes, but for atomics
we require 16 bytes. But if you explicitly use a 16-byte aligned
pointer type to assert to the compiler that this object *is* aligned,
the compiler should be able to rely on that.
Of course if some part of the middle end get the alignment wrong, we
have a problem. But I'm not sure how this could happen here. I agree
that it might be the case that a user-specified *under*-alignment might
get lost somewhere (e.g. you have a packed int and somewhere in the
optimizers this gets accidentally cast to a normal int with the default
alignment). But in *this* case, what would have to happen is that the
middle-end accidentally casts something to a pointer with *higher*
than the default alignment for the type, even though no such pointer
cast is present in the source. Can this really happen?
> I'm not sure how it is done now but IIRC the users
> use __atomic_load (ptr) and then the frontend changes that
> to one of BUILT_IN_ATOMIC_LOAD_{N,1,2,4,8,16} based on
> some criteria (size mainly). I'm saying we should factor in
> alignment here, _not_ using say BUILT_IN_ATOMIC_LOAD_16
> if according to the C standard the data isn't aligned. Maybe we can
> ask the target whether the alignment according to C matches the
> requirement for _16 expansion. And if things are not fine
> the FE should instead use BUILT_IN_ATOMIC_LOAD_N
> which we later if the compiler can prove bigger alignment and N
> is constant could expand as one of the others.
>
> But safety first.
The problem with using the _N variant is that we then get a call
to the _n version of the library routine, right? This would actually
be wrong on s390. The problem is that all atomic operations on any
one single object need to be consistent: they either all use the
16-byte atomic instruction, or else they all protect the access with
a lock. If you have parts of the code use the lock and other parts
use the instruction, they're not actually protected against each other.
This is why the _16 version of the library routine does the runtime
alignment check, so that all accesses to actually 16-byte aligned
objects use the instruction, both in the library and in compiler-
generated code. The _n version doesn't do that.
So I guess we'd have to convert the _n version back to the _16
library call if the size is constant 16, or something?
> So yes, I'd say try tackling the issue in the frontend which is the
> last to know the alignment in the most optimistic way (according
> to the C standard). At RTL expansion time we have to be (very)
> conservative.
>
> Btw, the alternative is to add an extra alignment parameter
> to all of the atomic builtins where the FE could stick the alignment
> for us to use during RTL expansion. But given we have the _{1,2,4,8,16,N}
> variants it sounds easier to use those...
>
> Note we only document __atomic_load and __atomic_load_n so
> the _{1,2,4,8,16} variants seem to be implementation detail.
Adding the alignment parameter would work, I think.
Bye,
Ulrich
--
Dr. Ulrich Weigand
GNU/Linux compilers and toolchain
ulrich.weig...@de.ibm.com
