Re: Fix 21166

[Nathan Sidwell <nathan at codesourcery dot com>]

Richard,

ok, now we get to 'what does DECL_PACKED actually mean?'

/* In a FIELD_DECL, indicates this field should be bit-packed.  */
#define DECL_PACKED(NODE) (FIELD_DECL_CHECK (NODE)->decl.regdecl_flag)

What does 'bit-packed' here mean?  It surely doesn't mean that
	struct S1 { int somebits : 1;
	   	    int m __attribute((packed)); };
has S1::m starting at bit 1 of byte zero.  It must mean byte-packed
(well strictly speaking unit-packed).

So, it's a request to stor-layout to pack the field as close as
possible to its predecessor.  What should it mean after layout
is complete?  Should it mean
1) the user asked for this to be packed densely, or
2) this field is not necessarily aligned

1 and 2 have no difference on the actual layout of the structure, but
the differ in how the field might be treated later on.  The current
behaviour is #1, but that doesn't really have any meaning for an
object that can be placed at any alignment.  In C++ it is so very
easy to take references to packed fields *without* the programmer
noticing -- simply pass such a field to std::min for instance.  The
FE deals with the subsequent alignment problem by invoking a copy ctor
to create a properly aligned object.  The trouble with that is that
C++ only permits you to do it for constant references, and the PR
here is quite rightly complaining that the field is not misaligned
anyway.  It seems wrong for the FE to have to overrule DECL_PACKED
and look at the natural alignment of the field's type. It's also
a maintainance issue, as we'd have to find all the places where
it matters.  (this is the rationale for patching stor-layout, rather
than the C++ FE.)

IMO the real problem is that we don't propagate unalignedness in the
type system correctly (we should treat it in a way similar to constness).
The type of the field which is packed should be changed from plain 'T' to
'T __attribute((aligned(1)))' (and of course not adding that attribute if
the alignment of T was already 1).  If we did that, DECL_PACKED would go
away.

> I think you mean TYPE_ALIGN, but even then I'm not 100% sure.
I'm not sure either :)  What does it mean to say of a field
	int m __attribute ((packed,aligned(8));
gcc places m according to the specified alignment.  Shouldn't DECL_PACKED
be clear in this case?   My patch is not correct in this regard.  For
	int m __attribute ((packed,aligned(2));
we place it at alignment 2.  Because this is less than the natural
alignment, we should keep DECL_PACKED set.

>   struct s1 {
> 	int c __attribute__((packed));
>   } __attribute__((packed));
>
>   struct s2 {
> 	struct s1 s __attribute__((packed));
>   } __attribute__((packed));
I believe s1::c should have DECL_PACKED set, but s2::s should have it
clear.  Current behaviour has DECL_PACKED set in both cases, this patch
changes it to only set it for s1::c.

nathan

--
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk