Re: m68k structure packing

Re: m68k structure packing

[Mike Stump]

> Date: Tue, 30 Sep 1997 23:05:48 -0600
> From: Jeffrey A Law <law@hurl.cygnus.com>

>   In message <199710010315.UAA28203@kankakee.wrs.com>you write:
>   > Don't know how.  All I can do it try a couple of testcases that should
>   > expose the tricky parts, I have done that.
> Read the docs for PCC_BITFIELD_MATTERS.

Sorry I have given the impression that I haven't read that, that isn't
the case.

> It describes a very particular problem (bitfields crossing certain
> alignment boundaries).

Yes.  Again, sorry I gave the impression that I didn't read, understand
and try the case discussed, as that isn't the case.

> It even includes code which attempts to set up this situation;

Yes...

> Then look at the resulting output

I must admit I didn't do that before because I didn't see that it
could offer any additional insight.  But, since you think it has some
relevance, I took a look at the output (and a few variations on it)
and I still don't see how it can offer any additional insight.  I did
see what I consider some additional minor nits... like padding at the
end when :0 is used, that we may want to go ahead and remove (padding
on a packed structure isn't desired or useful), and undesired
alignment when :0 is used, and because of the presence of the
undesired alignment, aligned loads and stores are generated, since the
data is aligned.  This, while it is a bug, won't cause codegen
problems, just incompatibilities if/when we fix that problem.

> and verify that you don't have any unaligned loads/stores.

Sorry I gave the impression that I haven't tested it, as that is not true.

> Doesn't seem all that hard to me.

Nor to me, as I I have already done all the testing that you seem to
imply.  Let me put it another way, I am to the point of happiness with
the code, that I don't know of any additional way to test it by hand.

> I think if you do that and can show Jim that it works then you'll
> both be happy.

Ah, now here is something I didn't do.  I have not shown you all all
the testcases I have looked at, nor all the resulting code...  I'll
see about putting together another mail message with all that you
request, but in general this isn't how I normally operate.  I normally
keep that level of detail to myself, and when the code is as I want,
then I submit the patches I've developed enmass.

I don't think we should force authors of code to prove to kenner that
a change is good enough for the FSF's compiler, and then force them to
go through the pain again to get it into the egcs compiler.  I think
we risk faster divergence that way, and keeping them converged is
better.  I thought that kenner would be generally be harder to deal
with, and while I may still generally think that is true, it wasn't in
this case.

Re: m68k structure packing

[Joel Sherrill]

On Wed, 1 Oct 1997, Mike Stump wrote:

> I don't think we should force authors of code to prove to kenner that
> a change is good enough for the FSF's compiler, and then force them to
> go through the pain again to get it into the egcs compiler.  I think
> we risk faster divergence that way, and keeping them converged is
> better.  I thought that kenner would be generally be harder to deal
> with, and while I may still generally think that is true, it wasn't in
> this case.

Maybe it seemed harsh from your standpoint but fom the outside looking in,
it just looked like there was concern over correctness.   I think the
whole discussion on the list has taken place in less than 48 hours.  That
is really pretty quick to have been over as may technical points as you
guys have bounced back and forth.

Just my .02 worth.

--joel

Re: m68k structure packing

[Jim Wilson]

I think you are suffering from some misconceptions here.

	I don't think we should force authors of code to prove to kenner that
	a change is good enough for the FSF's compiler, and then force them to
	go through the pain again to get it into the egcs compiler.

We don't.  Code that is accepted by Kenner into the FSF's compiler is
automatically accepted into EGCS unless there is something obviously wrong
with it.  In this case, there was something wrong with your patch that was
obvious to me but which Kenner apparently missed.  I don't think it is
unreasonable for me to point out problems which Kenner has apparently missed.

It turns out that it was a documentation problem and not a problem with your
patch, but the fact remains that there was a real problem here that needed to
be addressed.

	I think
	we risk faster divergence that way, and keeping them converged is
	better.

There was no chance of a divergence here.  If there was a problem with your
patch, I would have reported it to Kenner so that gcc2 and egcs could stay
in sync.

	I thought that kenner would be generally be harder to deal
	with, and while I may still generally think that is true, it wasn't in
	this case.

I am trying to be cooperative.  I realized that you didn't fully understand
what I wanted to see, so as soon as I could I spent several hours investigating
the problem myself and I posted the results of what I found to the list.
It was only 24 hours after I brought up the issue before I resolved it.
I don't think that 24 hours is an unresonable delay for this patch.

Jim

Re: m68k structure packing

[Mike Stump]

> Date: Wed, 1 Oct 1997 11:52:44 -0400
> From: Peter Barada <pbarada@wavemark.com>
> To: wilson@cygnus.com

> If you have a short in the struct:

> struct {
> 	short b;
> 	char a;
> } z, list[10];

> then sizeof(z) could be 3 but is 4 due to alignment requirements(i.e
> malloc(n*sizeof(z))) and sizeof(list) is 40.

Is this fact, or theory?  On the sparc, which is unaffacted by my
change, with -fpack-struct we get 3 bytes.

struct foo1
{
  short b;
  char a;
} f[2] = {{ 1, 2}, {3, 4}}, b;

short i, *ip;

main () {
  ip = &f[1].b;
  i = *ip;
}

fails.  My patches may help the m68k fail in the same way I suspect.

> And obviously if you have a long, then the sizeof has to round up to a
> long.

That isn't what the code today does.

> Even if you pack the structure, you have to allow for alignment
> requirements.

That isn't what the code today does.


This issue is 100% independent of the issue I was interested in, but
since we are talking about it, it makes since to bring this issue up,
and see what others think about it.

Should the above code fail at runtime?  (You have to have a
STRICT_ALIGNMENT machine, pack a structure, and you may have to use
int or long or float or double to make it fail).  If not, should we
make it work by aligning it or by requiring what the pointer points to
is a packed short, then on dereference of a pointer to a packed short,
we could know to do byte accesses.

Re: m68k structure packing

[Peter Barada]

>> If you have a short in the struct:
>
>> struct {
>> 	short b;
>> 	char a;
>> } z, list[10];
>
>> then sizeof(z) could be 3 but is 4 due to alignment requirements(i.e
>> malloc(n*sizeof(z))) and sizeof(list) is 40.
>
>Is this fact, or theory?  On the sparc, which is unaffacted by my
>change, with -fpack-struct we get 3 bytes.

If you pack the structure on any hardware with strict alignment then
you have to access any non-byte element using ONLY byte accesses since
you can not assume the alignment of the address of the start of the structure.
Unfortunately on the 68k this produces some really slow code. Ex: 

struct foo {
	char c;
	short b;
} *p;

When foo is not packed, the size is 4, and the offset to b is 2.
compiling p->b += 1 when foo is not packed gives:

; for sake of theexample, I'll load p->b into a register.
;
move.l	 p,a0		; load p into a0
move.w	 2(a0),d0	; load short 2 off of a0 into d0 (ofset to b)
addq.w	 #1,d0		; increment d0 as a word
move.w	 d0,2(a0)	; store short d0 2 off of a0

This works since it is assumed thant any pointer to the structure is even.

When foo is packed, the size is 3, and offset to b is 1.
To compile p->b += 1 when foo is packed requires:

move.l	 p,a0		; load p into a0
move.b	 1(a0),d0	; load the high byte of p->b into d0
lsl.w	 #8,d0		; shift left into the high byte
move.b	 2(a0),d0	; load the low byte of p->b into d0
addq.w	 #1,d0		; increment as a word
move.b	 d0,2(a0)	; store low byte
lsr.w	 #8,d0		; shift high byte into low byte
move.b	 d0,1(a0)	; store high byte

Obviously this takes more instructions, and many more clock cycles.
On the 68000 it takes 4 clocks for each bit shift, so in this code the
shifts alone take up 64 clocks.  I don't think the tradeoff is worth
it here.

On other processors that allow unaligned acces, then the code is the
same in either case, and the only hit is having to double cycle the
bus.

>Should the above code fail at runtime?  (You have to have a
>STRICT_ALIGNMENT machine, pack a structure, and you may have to use
>int or long or float or double to make it fail).  If not, should we
>make it work by aligning it or by requiring what the pointer points to
>is a packed short, then on dereference of a pointer to a packed short,
>we could know to do byte accesses.

Again it all depends. If you pack and use short accesses, then yes I
would expect a SIGBUS error on the short load instruction. If you can
pack it and the compiler can produce code that uses only byte
accesses, then the bloated code will go dog slow but not SIGBUS.

If you want packed structures then they should be sized up to a short, and
anything that is not a char in a packed struct should be aligned on a
short boundary(BIGGEST_ALIGNMENT). But this is what happens on the 68k
anyway.

-- 
Peter Barada				pbarada@wavemark.com
Wizard					617-270-7098 x1226
WaveMark Technologies, Inc.		617-270-0193 (fax)

"Real men know that you should never attempt to accomplish with words
what you can do with a flame thrower" --Bruce Ferstein

Re: m68k structure packing

[Per Bothner]

> On the 68000 it takes 4 clocks for each bit shift, so in this code the
> shifts alone take up 64 clocks.  I don't think the tradeoff is worth
> it here.

Nobody claims it is "worth" it.  That is why byte-packing is not the
default.  It is an option, because some people want/need byte-packing,
perhaps to be compatible with some other architecture.  They are
willing to pay the price.

> Again it all depends. If you pack and use short accesses, then yes I
> would expect a SIGBUS error on the short load instruction. If you can
> pack it and the compiler can produce code that uses only byte
> accesses, then the bloated code will go dog slow but not SIGBUS.

Yep.

> If you want packed structures then they should be sized up to a short,

But then they wouldn't be packed, would they ...

Maybe what you are asking for is a "semi-packed" option.

	--Per Bothner
Cygnus Solutions     bothner@cygnus.com     http://www.cygnus.com/~bothner

Re: m68k structure packing

[Jim Wilson]

	Maybe what you are asking for is a "semi-packed" option.

You can get this by using something like
	__attribute__ ((packed, aligned(2)))
The attribute packed decreases alignment to 1 byte, and then the attribute
aligned increases it to 2 bytes.

Jim

Re: m68k structure packing

[Paul Koning]

>>>>> "Peter" == Peter Barada <pbarada@wavemark.com> writes:

 >> Should the above code fail at runtime?  (You have to have a
 >> STRICT_ALIGNMENT machine, pack a structure, and you may have to
 >> use int or long or float or double to make it fail).  If not,
 >> should we make it work by aligning it or by requiring what the
 >> pointer points to is a packed short, then on dereference of a
 >> pointer to a packed short, we could know to do byte accesses.

 Peter> Again it all depends. If you pack and use short accesses, then
 Peter> yes I would expect a SIGBUS error on the short load
 Peter> instruction. If you can pack it and the compiler can produce
 Peter> code that uses only byte accesses, then the bloated code will
 Peter> go dog slow but not SIGBUS.

I have a simple view on this: if the compiler generates code that
breaks at runtime, it's a compiler bug (except if I hack up pointers
in ways contrary to what the compiler was allowed to expect).

In particular, declaring a structure packed MUST have two effects:

1. There is NO padding whatsoever
2. The generated code allows for this and makes all necessary
adjustments for dealing with unaligned references

 Peter> If you want packed structures then they should be sized up to
 Peter> a short, and anything that is not a char in a packed struct
 Peter> should be aligned on a short boundary(BIGGEST_ALIGNMENT). But
 Peter> this is what happens on the 68k anyway.

NFW.  If I say "packed" it HAS to mean NO padding.  If I want padding
I'd either ask for not packed (in which case the normal pad rule
applies, i.e., each primitive element is aligned to its size and each
composite to the largest of the alignment of its components) or I can
ask for packed and align explicitly by manual padding.

If you pad packed structures you WILL break existing code.  (For
example, last year I wrote code that had all its protocol messages
defined as packed structs with explicit padding in a few places.  I
had to do the explicit padding because the Intel i960 compiler doesn't
implement the normal structure padding rules -- with a decent compiler
such as gcc it would been unnecessary.  Anyway, if you change the pad
rules then that code would emit invalid messages and would not conform
to the protocol standard.)

	paul

-- 
!-----------------------------------------------------------------------
! Paul Koning, NI1D, C-24183
! Xedia Corporation, 119 Russell Street, Littleton, MA 01460, USA
! phone: +1 508 952 6000 ext 115, fax: +1 508 952 6090
! email: pkoning@xedia.com
! Pgp:   27 81 A9 73 A6 0B B3 BE 18 A3 BF DD 1A 59 51 75
!-----------------------------------------------------------------------
! "The only purpose for which power can be rightfully exercised over 
!  any member of a civilized community, against his will, is to prevent
!  harm to others.  His own good, either physical or moral, is not
!  a sufficient warrant."    -- John Stuart Mill, "On Liberty" 1859

Re: m68k structure packing

[Jim Wilson]

	If you pack the structure on any hardware with strict alignment then
	you have to access any non-byte element using ONLY byte accesses since
	you can not assume the alignment of the address of the start of the structure.
	Unfortunately on the 68k this produces some really slow code. Ex: 

The solution is simple.  If you care about performance, then don't pack your
structures.

	If you want packed structures then they should be sized up to a short, and
	anything that is not a char in a packed struct should be aligned on a
	short boundary(BIGGEST_ALIGNMENT). But this is what happens on the 68k
	anyway.

It makes no sense to disable structure packing because of performance
concerns.  People who are packing structures are doing it because they
actually need a particular structure layout.  If the programmer requests
packed structures, then the compiler should obey, and the compiler should
make structure packing work the same way for all targets.  Mike Stump's
patch is a step in this direction.

Jim

Re: m68k structure packing

[Jim Wilson]

	... code that sets a pointer to the address of an unaligned packed
	structure member...

	Should the above code fail at runtime? 

Yes, it should fail, because the address is not valid for its type.

Someday, we will be able to say something like:
	short *ip __attribute__ ((unaligned));
and then it will work, because gcc will know that it needs to emit special
code to derefence the pointer.  Meanwhile, you can't expect it to work
portably.

Jim

Re: m68k structure packing

[Richard Henderson]

> Someday, we will be able to say something like:
> 	short *ip __attribute__ ((unaligned));
> and then it will work, because gcc will know that it needs to emit special
> code to derefence the pointer.

Or something more general like

	int __attribute__((aligned(2))) * * ppui;
	int * __attribute__((aligned(1))) * pupi;

with the attribute at the type being modified, and arbitrary
alignment specifications.


r~

Re: m68k structure packing

[Jim Wilson]

	> Gcc knows that it needs to access bitfields differently when a structure
	> is packed.  It may be that gcc will generate correct code with your patch
	> for cases that would ordinarily fail ...

	Yes it will.  I've used this on a number of occasions to get
	machine independant unaligned loads.  It works on all machines,
	even if gcc has to resort to byte loads and shifts.

I know that bitfield references to packed structures work.  This has never
been in doubt.

However, there remains a specific question here as to how this affects the
m68k port with respect to the known and documented problems with
PCC_BITFIELD_TYPE_MATTERS and STRUCTURE_SIZE_BOUNDARY.  It is not obvious
what the interaction is.  But as I mentioned in my previous mail, I am starting
to suspect that improvements to the unaligned/packed structure field support
over the years has accidentally solved this known problem, in which case this
may now be a `historical' problem.  If you haven't done so already, and if
you really care about this problem, take a look at the
PCC_BITFIELD_TYPE_MATTERS documentation.

Jim

Re: m68k structure packing

[Kamil Iskra]

On Tue, 30 Sep 1997, Jim Wilson wrote:

> However, there remains a specific question here as to how this affects the
> m68k port with respect to the known and documented problems with
> PCC_BITFIELD_TYPE_MATTERS and STRUCTURE_SIZE_BOUNDARY.  It is not obvious
> what the interaction is.  But as I mentioned in my previous mail, I am starting
> to suspect that improvements to the unaligned/packed structure field support
> over the years has accidentally solved this known problem, in which case this
> may now be a `historical' problem.

This is also my assumption, based on the testing that I've done (more on
this later).

> If you haven't done so already, and if
> you really care about this problem, take a look at the
> PCC_BITFIELD_TYPE_MATTERS documentation.

I've read it at least dozen times, I could probably quote some parts from
memory :-).

I also asked these sort of questions quite a long time ago on gcc2, but no
one was able, or cared, to help. I'm happy that this issue was finally
given the attention it (IMHO) deserves :-).

Fo the m68k-amigaos port, I undefined STRUCTURE_SIZE_BOUNDARY, without
defining PCC_BITFIELD_TYPE_MATTERS. According to tm.texi and expmed.c
(store_fixed_bit_field), this is not supposed to work. Here's one comment
from the latter file:

  /* There is a case not handled here:
     a structure with a known alignment of just a halfword
     and a field split across two aligned halfwords within the structure.
     Or likewise a structure with a known alignment of just a byte
     and a field split across two bytes.
     Such cases are not supposed to be able to occur.  */

I made plenty of testing, varying from trivial cases to really
pathological ones, like operations on 64-bit bitfields in a structure
where the compiler could only assume 8-bit alignment. They compiled and
run fine, and I couldn't find a flaw by looking at the assembler output.
The compiler would simply use 1-byte loads and stores, shifts, ands and
ors. It was quite impressive, actually :-). These were clearly the cases
that, according to the comment above, were not supposed to work. Yet they
did.

We've also done a lot of "real life" tests, by simply using such a
compiler to rebuild complete source tree of GeekGadgets, a project
consisting of hundreds of megabytes of well-known free sources. We haven't
observed any decrease in the stability of the binary tree. Of course, most
of us don't use 68000 any more, but still some people do and they haven't
reported any problems after the switch to unaligned structures took place.

Actually, this problem would be hard to observe in the real life. That's
because most structures have "int" or pointer fields, thus imposing the
2-byte boundary. Only structures consisting of chars or bitfields only are
laid out differently, and such structures are rare. I analysed every
single standard header file looking for such structures, and I found just
one, if I remember correctly. Still, it contained an even number of chars
and no bitfields, so it made no difference :-). In order to ensure the
binary compatibility with the older software, I also compiled IXEmul, our
UNIX emulation software (something similar in principles to cygnus.dll in
cygwin32, I guess), with both "aligning" and "not aligning" compilers, and
byte-compared the resulting object files.  Again, of the several hundreds
of them, only one was different, because the not aligning compiler had to
be more pessimistic when accessing a bitfield of a structure passed to the
function by pointer. Still, the produced code was correct and backwards
compatible.

/ Kamil Iskra - AMIGA 1200, 68030 50MHz, HDD 1.6 GB, 18 MB RAM \
| iskra@student.uci.agh.edu.pl  kiskra@ernie.icslab.agh.edu.pl |
| http://student.uci.agh.edu.pl/~iskra                         |
\ PGP public key available via Finger or WWW                   /

Re: m68k structure packing

[Mike Stump]

> Date: Tue, 30 Sep 1997 19:57:52 -0700
> From: Jim Wilson <wilson@cygnus.com>

> It may be that gcc will generate correct code with your patch
> [ ... ] because the structure is marked as packed.  Can you please
> check?

Don't know how.  All I can do it try a couple of testcases that should
expose the tricky parts, I have done that.  The code works flawlessly.
Storing a bitfield, loading from a bitfield, incing a bitfield....
a long long bitfield, an int bitfield...  I cannot get it to fail,
therefore I cannot fix it.  My working assumption is there are
no bugs.

If we find one, I know how to fix it, but I just need a pointer to
the failure.  I have generally reviewed the code, and it all seems
reasonable.

Re: m68k structure packing

[Jeffrey A Law]

  In message < 199710010315.UAA28203@kankakee.wrs.com >you write:
  > Don't know how.  All I can do it try a couple of testcases that should
  > expose the tricky parts, I have done that.
Read the docs for PCC_BITFIELD_MATTERS.  It describes a very particular
problem (bitfields crossing certain alignment boundaries).

It even includes code which attempts to set up this situation;
so you can compile that sample code with -m68000.

Then look at the resulting output and verify that you don't have
any unaligned loads/stores.  Doesn't seem all that hard to me.
I think if you do that and can show Jim that it works then
you'll both be happy.

jeff

Re: m68k structure packing

[Mike Stump]

> Date: Tue, 30 Sep 1997 14:55:51 -0700
> From: Jim Wilson <wilson@cygnus.com>

> With this patch, gcc will pack structures regardless of what
> STRUCTURE_SIZE_BOUNDARY is set to, but will now in some cases
> silently generate code that fails at run time.

I would be interested in any such cases that you know of.  This cannot
happen on the arm, if we are to believe the arm coff or netbsd ports
for the arm, as they reset STRUCTURE_SIZE_BOUNDARY to 8.  This cannot
happen on the sh, if we are to believe the sh ports, as
STRUCTURE_SIZE_BOUNDARY defaults to 8.  Only with -mpadstruct option
does the value change.  elxsi doesn't count because it was copied
wholesale from the VAX, and I suspect at the time, vax had it as 32,
also it doesn't count because I don't think a machine is left in the
world that can run.  dsp16xx, fx80, mn10200, pyr, spur, tahoe and
we32k I cannot comment on, other than to say I think we32k is dead.

That leaves m68k.  On the m68k the compiler did seem to go out of it's
way to generate correct code.  Further, Kamil has claimed that the
amiga m68k port works fine with STRUCTURE_SIZE_BOUNDARY as 8.

Why do you think generated code will fail?  Because of unaligned reads
and writes?  I see many popular machines, and ones that I know have
been tested well, that define STRICT_ALIGNMENT and have
STRUCTURE_SIZE_BOUNDARY set to 8.  Maybe if you describe that case I
can hunt for it.

If one goes back though the old ChangeLogs, one sees that many ports
used to have values other than 8, but over time, they were changed to
8.

I think that in the absence a bug that can be reproduced, and since it
has been accepted by the FSF for inclusion in the next FSF gcc
release, that it be included in egcs.

If people that have access to m68000 (not a 030 or a 020) could try
out -fpack-struct (with -O, without -O) with my changes, and try and
make it fail, that would be a big help in ensuring that no bug is
introduced.  It is probably imporant that the code run on a m68k that
requires alignment (if I understand Jim's concerns).

Re: m68k structure packing

[Jim Wilson]

	I would be interested in any such cases that you know of.

It will happen for the m68k ports, of course, which are the ones that you
are trying to fix.

This problem has been discussed before, and is well known.  There are many
places in the code that document it.  See in particular the discussion
of PCC_BITFIELD_TYPE_MATTERS in the tm.texi file.

Note that most m68k ports do not define PCC_BITFIELD_TYPE_MATTERS, and hence
must define STRUCTURE_SIZE_BOUNDARY to >=16, and hence must not allow structure
packing.

The m68kemb.h file goes the other way, defining PCC_BITFIELD_TYPE_MATTERS,
and defining STRUCTURE_SIZE_BOUNDARY to 8, and hence does allow structure
packing.  If you want structure packing, this is what you should do.

	Why do you think generated code will fail? 

Because I have seen the problem before, and I have seen it so many times
that it falls into the category of things that are obvious to me, but which
I can't easily explain because I forgot the reasons for it long ago.

My recollection is that it will only fail if you compile for the 68000,
because the 68000 has no bit field instructions, and requires 2 byte alignment
for non-byte loads/stores.  (As per the PCC_BITFIELD_TYPE_MATTERS
documentation.) Most other 68k family members do not have both these
restrictions.

	Further, Kamil has claimed that the
	amiga m68k port works fine with STRUCTURE_SIZE_BOUNDARY as 8.

It will appear to work fine until you trigger the bug.  Obviously, he did
not try any testcase that would trigger the bug.  Probably because he did
not test on a m68000.

	Maybe if you describe that case I
	can hunt for it.

I believe it has already been described:

>From: Andreas Schwab <schwab@issan.informatik.uni-dortmund.de>
>The only place where it can fail is on a STRICT_ALIGNMENT target when
>using a packed structure in an array.

	I think that in the absence a bug that can be reproduced,

Unfortunately, you are putting the burden in the wrong place.  I am much
busier than you, and do not have time to search for a testcase to reproduce
a bug that I already know exists.  I would prefer that you put some effort into
demonstrating that the known bug does not exist with your patch.  It
should be easy to prove or disprove this with some simple experimentation.

First demonstrate the problem that exists as per the PCC_BITFIELD_TYPE_MATTERS
documentation.  Then check to see if your patch triggers this bug.  I believe
that it does.

Because the patch is already in the FSF gcc sources, it will probably also
be added to the EGCS sources.  However, I think it is important for you
to understand that this is not the `bug fix' patch that you think it is.
What this patch does is trade one set of complex problems for a different
set of complex problems, and it is not clear which set of problems is worse.
After we do install the patch, we may end up needing to remove it if the new
set of problems are worse than the old set.

Jim

Re: m68k structure packing

[Peter Barada]

>My recollection is that it will only fail if you compile for the 68000,
>because the 68000 has no bit field instructions, and requires 2 byte alignment
>for non-byte loads/stores.  (As per the PCC_BITFIELD_TYPE_MATTERS
>documentation.) Most other 68k family members do not have both these
>restrictions.

Yes, the 68000 did not have bit field instructions, and everything
besides a byte access had to be even aligned(including the stack).

It will also fail on the AMD29000 since it too does not have bit field
insns and can only access memory on word boundaries.

I don't see any trouble with a structure that contains only bytes
being sized and aligned on byte boundaries:

struct {
	char a,b,c;
} z, list[10];

sizeof(z) would be 3, and sizeof(list) would be 30.

If you have a short in the struct:

struct {
	short b;
	char a;
} z, list[10];

then sizeof(z) could be 3 but is 4 due to alignment requirements(i.e
malloc(n*sizeof(z))) and sizeof(list) is 40.

And obviously if you have a long, then the sizeof has to round up to a
long.

Even if you pack the structure, you have to allow for alignment
requirements.
	
>Because I have seen the problem before, and I have seen it so many times
>that it falls into the category of things that are obvious to me, but which
>I can't easily explain because I forgot the reasons for it long ago.

Yes, I agree its obvious, and the answer is that the sizeof a
structure has to be rounded up to the most strict alignment of any of
its elements to allow an array of these objects to have each element
conform to the alignment requirements.

If any 'improvement' breaks any port, then the 'improvement' is not,
and has to be fixed.

-- 
Peter Barada				pbarada@wavemark.com
Wizard					617-270-7098 x1226
WaveMark Technologies, Inc.		617-270-0193 (fax)

"Real men know that you should never attempt to accomplish with words
what you can do with a flame thrower" --Bruce Ferstein

Re: m68k structure packing

[Mike Stump]

> To: mrs@wrs.com (Mike Stump)
> From: mycroft@mit.edu (Charles M. Hannum)
> Date: 30 Sep 1997 16:01:24 -0400

> Um, doesn't this *severely* break compatibility?

I guess it depends upon how you use the words and what they mean to
you.  Yes, this work causes the compiler to not have a bug that it had
before, and lack of that bug makes some objects not binary compatible
with prior objects on some systems.  For completeness the systems are:

config/arm/arm.h:#define STRUCTURE_SIZE_BOUNDARY 32
config/dsp16xx/dsp16xx.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/elxsi/elxsi.h:#define STRUCTURE_SIZE_BOUNDARY 32
config/fx80/fx80.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/3b1g.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/a-ux.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/altos3068.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/apollo68.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/ccur-GAS.h:#define STRUCTURE_SIZE_BOUNDARY 16 
config/m68k/crds.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/hp2bsd.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/hp320.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/hp3bsd.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/hp3bsd44.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/isi.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/lynx-ng.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/lynx.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/mot3300.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/netbsd.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/next.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/pbb.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/plexus.h:#define STRUCTURE_SIZE_BOUNDARY 16 /* for compatibility with cc */
config/m68k/sun2.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/sun3.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/tower.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/m68k/vxm68k.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/mn10200/mn10200.h:#define STRUCTURE_SIZE_BOUNDARY 16
config/pyr/pyr.h:#define STRUCTURE_SIZE_BOUNDARY 32
config/sh/sh.h:#define STRUCTURE_SIZE_BOUNDARY (TARGET_PADSTRUCT ? 32 : 8)
config/spur/spur.h:#define STRUCTURE_SIZE_BOUNDARY 32
config/tahoe/tahoe.h:#define STRUCTURE_SIZE_BOUNDARY 32
config/we32k/we32k.h:#define STRUCTURE_SIZE_BOUNDARY 32

Note that only code that specifies -fpack-struct, or attribute
((packed)) on only the above systems is broken by the change, code
that does not, cannot be broken.  The code that specifies either of
these two mechanisms, is now more compatible across platforms
(desirable), where as before, the compiler would just do the wrong
thing on some platforms.  I think the consistency across platforms,
and providing a way for a programmer to customize structure layout to
his liking is more important than allowing this bug to continue to
live.  Be aware, that the people using -fpack-struct or attribute
((packed)) are exactly the people that would want this problem fixed,
so to them, they can tolerate the incompatibility.

Do you use -fpack-struct or attribute ((packed))?

Re: m68k structure packing

[Jim Wilson]

Without this patch, gcc will always generate working code, but will not
pack structures as expected if STRUCTURE_SIZE_BOUNDARY != 8.

With this patch, gcc will pack structures regardless of what
STRUCTURE_SIZE_BOUNDARY is set to, but will now in some cases silently
generate code that fails at run time.

This does not seem like much of an improvement to me.  I think the former
problem is easier to deal with than the latter problem.

Jim

m68k structure packing

[Mike Stump]

Could you fold in this work from the FSF sources?  Thanks.


Fri Sep 26 14:06:45 1997  Mike Stump  <mrs@wrs.com>

	* c-decl.c (start_struct): Ensure that structs with forward
	declarations are in fact packed when -fpack-struct is given.

Wed Sep 24 11:31:24 1997  Mike Stump  <mrs@wrs.com>

	* stor-layout.c (layout_record): Ignore STRUCTURE_SIZE_BOUNDARY if
	we are packing a structure.  This allows a structure with only
	bytes to be aligned on a byte boundary and have no padding on a
	m68k.

Index: stor-layout.c
===================================================================
RCS file: /folk/mrs/.cvsroot/egcs/gcc/stor-layout.c,v
retrieving revision 1.1.1.1
retrieving revision 1.2
diff -c -p -r1.1.1.1 -r1.2
*** stor-layout.c	1997/08/15 18:56:53	1.1.1.1
--- stor-layout.c	1997/09/24 18:41:48	1.2
*************** layout_record (rec)
*** 306,316 ****
       tree rec;
  {
    register tree field;
- #ifdef STRUCTURE_SIZE_BOUNDARY
-   unsigned record_align = MAX (STRUCTURE_SIZE_BOUNDARY, TYPE_ALIGN (rec));
- #else
    unsigned record_align = MAX (BITS_PER_UNIT, TYPE_ALIGN (rec));
- #endif
    /* These must be laid out *after* the record is.  */
    tree pending_statics = NULL_TREE;
    /* Record size so far is CONST_SIZE + VAR_SIZE bits,
--- 306,312 ----
*************** layout_record (rec)
*** 324,329 ****
--- 320,330 ----
       that we know VAR_SIZE has.  */
    register int var_align = BITS_PER_UNIT;
  
+ #ifdef STRUCTURE_SIZE_BOUNDARY
+   /* Packed structures don't need to have minimum size.  */
+   if (! TYPE_PACKED (rec))
+     record_align = MAX (record_align, STRUCTURE_SIZE_BOUNDARY);
+ #endif
  
    for (field = TYPE_FIELDS (rec); field; field = TREE_CHAIN (field))
      {
*************** layout_union (rec)
*** 563,579 ****
       tree rec;
  {
    register tree field;
- #ifdef STRUCTURE_SIZE_BOUNDARY
-   unsigned union_align = STRUCTURE_SIZE_BOUNDARY;
- #else
    unsigned union_align = BITS_PER_UNIT;
- #endif
  
    /* The size of the union, based on the fields scanned so far,
       is max (CONST_SIZE, VAR_SIZE).
       VAR_SIZE may be null; then CONST_SIZE by itself is the size.  */
    register int const_size = 0;
    register tree var_size = 0;
  
    /* If this is a QUAL_UNION_TYPE, we want to process the fields in
       the reverse order in building the COND_EXPR that denotes its
--- 564,582 ----
       tree rec;
  {
    register tree field;
    unsigned union_align = BITS_PER_UNIT;
  
    /* The size of the union, based on the fields scanned so far,
       is max (CONST_SIZE, VAR_SIZE).
       VAR_SIZE may be null; then CONST_SIZE by itself is the size.  */
    register int const_size = 0;
    register tree var_size = 0;
+ 
+ #ifdef STRUCTURE_SIZE_BOUNDARY
+   /* Packed structures don't need to have minimum size.  */
+   if (! TYPE_PACKED (rec))
+     union_align = STRUCTURE_SIZE_BOUNDARY;
+ #endif
  
    /* If this is a QUAL_UNION_TYPE, we want to process the fields in
       the reverse order in building the COND_EXPR that denotes its
Index: c-decl.c
===================================================================
RCS file: /folk/mrs/.cvsroot/egcs/gcc/c-decl.c,v
retrieving revision 1.1.1.4
retrieving revision 1.2
diff -c -p -r1.1.1.4 -r1.2
*** c-decl.c	1997/09/16 02:07:07	1.1.1.4
--- c-decl.c	1997/09/26 21:09:25	1.2
*************** start_struct (code, name)
*** 5527,5532 ****
--- 5527,5533 ----
    if (ref && TREE_CODE (ref) == code)
      {
        C_TYPE_BEING_DEFINED (ref) = 1;
+       TYPE_PACKED (ref) = flag_pack_struct;
        if (TYPE_FIELDS (ref))
  	error ((code == UNION_TYPE ? "redefinition of `union %s'"
  		: "redefinition of `struct %s'"),
------

Re: m68k structure packing

[Charles M. Hannum]

mrs@wrs.com (Mike Stump) writes:

> 
> Wed Sep 24 11:31:24 1997  Mike Stump  <mrs@wrs.com>
> 
> 	* stor-layout.c (layout_record): Ignore STRUCTURE_SIZE_BOUNDARY if
> 	we are packing a structure.  This allows a structure with only
> 	bytes to be aligned on a byte boundary and have no padding on a
> 	m68k.

Um, doesn't this *severely* break compatibility?

Re: m68k structure packing

[Jim Wilson]

Thinking about this some more, I am not sure that my assumptions are right.

Gcc knows that it needs to access bitfields differently when a structure
is packed.  It may be that gcc will generate correct code with your patch
for cases that would ordinarily fail (as per the PCC_BITFIELD_TYPE_MATTERS
documentation) because the structure is marked as packed.  Can you please
check?

Jim

Re: m68k structure packing

[Richard Henderson]

> Gcc knows that it needs to access bitfields differently when a structure
> is packed.  It may be that gcc will generate correct code with your patch
> for cases that would ordinarily fail ...

Yes it will.  I've used this on a number of occasions to get 
machine independant unaligned loads.  It works on all machines,
even if gcc has to resort to byte loads and shifts.


r~

Re: m68k structure packing

[Jim Wilson]

You are confusing the issues a bit.  There is a real problem, and I have
already described it, see the PCC_BITFIELD_TYPE_MATTERS documentation.
It should be easy to reproduce this problem.  Note that this problem has
nothing to do with your patch or structure packing which is where I think
you are getting confused.

My supposition was that your patch might cause this same problem to appear
when someone uses -fpack-struct because it is roughly equivalent to not
defining STRUCTURE_SIZE_BOUNDARY, and it is known that this case does fail.
This is the part that you haven't addressed.  It would be useful if
you could show that your patch does not cause this problem, for instance
by showing that a testcase that does fail when STRUCTURE_SIZE_BOUNDARY is not
defined does not fail when -fpack-struct is used with your patch.

It is of course not possible for you to prove that your patch has no bugs,
but I haven't asked you to do that.  All I have asked for is some evidence
that your patch does not accidentally trigger a known problem.

In any case, I am now leaning towards the opinion that the patch will work
OK, because gcc knows how to handle packed structures in general, so I will
stop bothering you about this.  In fact, I am even starting to wonder if the
PCC_BITFIELD_TYPE_MATTERS problem still exists; it may be that the support
for unaligned structure fields is general enough that this problem doesn't
exist anymore.

Jim

Re: m68k structure packing

[Jim Wilson]

I took a closer look at the PCC_BITFIELD_TYPE_MATTERS/STRUCTURE_SIZE_BOUNDARY
problem.  I have no trouble reproducing this problem using an old version
of gcc (gcc 2.2.2); the same testcases do not reproduce this problem using
current versions of gcc.  Stepping through them in the debugger, it is clear
that the changes that were made to support packed structures have solved the
problem as a side-effect.  Thus the PCC_BITFIELD_TYPE_MATTERS documentation
is wrong, as it documents a historical problem that does not exist anymore,
and this will not be a problem with your patch.

Jim