Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jan Beulich]

For the documentation requirements (I'm still learning doing 'open
source', and previously I was used to there being someone else taking
care of documentation stuff, after all I'm not a technical writer but a
programmer): Can you suggest an obvious to use editor for the .texi
files (not a plain text one, so I won't have to twiddle the directives
in there a hundred times)?

For the unmentioned test case changes: Without them these tests fail if
you have a target that requires less than word alignment as the default
setting (which is what DEFAULT_PACK_STRUCT is used for). Thus for such
targets the packing value has to be set high enough (and thus the value
8 to accomodate all targets up to 64 bits). It seemed inappropriate to
add options for individual targets here when adding them globally does
not do any harm), but it could of course be done.

I know what your answer will be to the following, but it's really
becoming a pain to try to help solve issues with the compiler when all
you get back is complaints that this and that and a third rule isn't
being fulfilled. I got complaints before stating that I didn't run the
testsuites. I'm running them now. I got complaints that I just built
(not bootstrapped) the compiler. I'm bootstrapping it now. I got
complaints that I'm not diff-ing against the mainline. I'm doing so now.
I got complaints that I didn't write ChangeLog entries. I'm writing them
now. I maybe got other complaints I don't even recall at the moment (I
tried to avoid one by now also building at least all default languages).
No-one seems to care that all these rules require an awful lot of time
(bootstrapping the compiler takes hours, the testsuite runs take hours,
using the mainline is wasted time to me and my employer since we
obviously can't/don't want to use this for general consumption, so
importance to us have only changes on top of a released compiler), and
that instead of doing useful work I'm spending days on fulfilling just
these rules (this might be less extreme if I worked on just a single fix
or a couple of them, but over the last two or three years, prior to
having an FSF copyright assignment, a couple of dozen changes have piled
up and I'd really like to get them out rather than dropping them and
later running into the same problem again). As said above (and as got to
hear previously), everyone's (supposedly) playing by these rules.
Whether everyone's pleased by them doesn't matter. Whether this severly
limits productivity doesn't matter either.

Jan

>>> "Joseph S. Myers" <jsm@polyomino.org.uk> 07.07.04 14:07:45 >>>
On Wed, 7 Jul 2004, Jan Beulich wrote:

> 2004-07-07 Jan Beulich <jbeulich@novell.com>
> 
> 	PR c/7054
> 	* tree.h (initial_max_fld_align): Declare
> 	* stor-layout.c (initial_max_fld_align): Define and initialize.
> 	(maximum_field_alignment): Initialize to the same value.
> 	* common.opt: Add -fpack-struct= variant of switch.
> 	* opts.c: Handle -fpack-struct= variant of switch.
> 	* c-pragma.c: Change #pragma pack() handling so that is becomes
> 	compatible to other compilers: accept individual 'push'
> argument,
> 	make final pop restore (command line) default, correct
> interaction
> 	of push/pop and sole specification of a new alignment (so that
> the
> 	sequence #pragma pack(push) - #pragma pack(<n>) becomes
> identical
> 	to #pragma pack(push, <n>).
> 	* testsuite/gcc.dg/pack-test-2.c: Adjust to permit and check
> 	#pragma pack(push).

We can't consider patches without proper documentation.  This needs (a)
 
documentation for the new command-line option syntax, (b)
documentation
for the pragma changes (and it seems for the pragma itself, which as
you
evidently understand what it's meant to do at present you're best
placed
to write), (c) for what seems to be a new target macro
DEFAULT_PACK_STRUCT.

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08
> 04:32:11.000000000 +0100
> +++
> 2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27
> 10:57:09.000000000 +0200

Not mentioned in the ChangeLog entry.  Why is this testcase being
changed?

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10
> 01:04:50.000000000 +0200
> +++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26
> 15:38:24.000000000 +0200

Likewise.

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17
> 16:48:28.000000000 +0200
> +++
>
2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
> 17:19:15.000000000 +0200

Likewise.  It seems clearly inappropriate for standards testcases to
have
any special options like you're adding.  (But this testcase is expected
to
become obsolete, as the draft C99 TC2 (N1060) would remove the
defective
requirement from the standard.)

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/ 
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Paul Brook]

> I know what your answer will be to the following, but it's really
> becoming a pain to try to help solve issues with the compiler when all
> you get back is complaints that this and that and a third rule isn't
> being fulfilled. I got complaints before stating that I didn't run the
> testsuites. I'm running them now. I got complaints that I just built
> (not bootstrapped) the compiler. I'm bootstrapping it now. I got
> complaints that I'm not diff-ing against the mainline. I'm doing so now.
> I got complaints that I didn't write ChangeLog entries. I'm writing them
> now. I maybe got other complaints I don't even recall at the moment (I
> tried to avoid one by now also building at least all default languages).
> No-one seems to care that all these rules require an awful lot of time
> (bootstrapping the compiler takes hours, the testsuite runs take hours,
> using the mainline is wasted time to me and my employer since we
> obviously can't/don't want to use this for general consumption, so
> importance to us have only changes on top of a released compiler), and
> that instead of doing useful work I'm spending days on fulfilling just
> these rules (this might be less extreme if I worked on just a single fix
> or a couple of them, but over the last two or three years, prior to
> having an FSF copyright assignment, a couple of dozen changes have piled
> up and I'd really like to get them out rather than dropping them and
> later running into the same problem again). As said above (and as got to
> hear previously), everyone's (supposedly) playing by these rules.
> Whether everyone's pleased by them doesn't matter. Whether this severly
> limits productivity doesn't matter either.

All the requirements you mention are documented on the gcc webpages, so they 
shouldn't really come as a surprise: http://gcc.gnu.org/contribute.html

I disagree that these requirements take a lot of time. Yes a full bootstrap 
and test can take several hours of *machine* time.
However even if you run this manually it only takes five minutes to check out 
a clean tree, apply a patch and type "make bootstrap; make -k check". You can 
then go and do something more productive for a few hours, or just leave it 
running overnight. If you have lots of patches it's not that hard to write a 
script to iterate over all of them.

You've got to remember that there are dozens, maybe even hundreds of 
developers working from gcc HEAD. If a patch isn't sufficiently tested and 
breaks bootstrap then all these developers are effected.

Personally I am very pleased that we have a fairly rigorous patch acceptance 
policy. I think that the overall productivity of gcc developers is improved 
by this.

Adding new changes to a release branch without adding them to mainline only 
has very short-term benefit, and potentially causes extra problems on the 
(stable) release branch. This is why we don't allow it.
Once you have put in the effort to update everything to mainline your 
maintenance burden for future releases should be reduced.

Paul

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Paolo Bonzini]

> I know what your answer will be to the following, but it's really
> becoming a pain to try to help solve issues with the compiler when all
> you get back is complaints that this and that and a third rule isn't
> being fulfilled.

All this is explained in http://gcc.gnu.org/contribute.html which is 
prominently linked in the home page.

> (bootstrapping the compiler takes hours, the testsuite runs take hours,

You can do that at night.  Testing a pile of changes together is usually 
not a problem, especially if well separated.  Still, once they have been 
reviewed and approved, you should take your time to test them one at a 
time; but then once you got the approval, you can take all the time you 
need before actually committing the changes.

> using the mainline is wasted time to me and my employer since we
> obviously can't/don't want to use this for general consumption, so
> importance to us have only changes on top of a released compiler),

Sorry, but that's not a valid point.  I don't think you can believe that 
GCC developers should pick up your 3.4 work and do the port to mainline 
themselves.  No matter if paid or volunteering, they have more 
interesting things to do.  The best solution to this problem is to 
submit the changes just after the beginning of stage1.  I think most GCC 
developers end stage3 with a queue of at least a dozen polished patches.

The point is that you/your employer had done it wrong by letting these 
changes pile up for two years.

> Whether everyone's pleased by them doesn't matter.
> Whether this severly limits productivity doesn't matter either.

I understand your position.  But the point is that these rules help 
maintainers reviewing your patch, they help developers understand what 
others have done, and they help people lend a hand when you have done 
something wrong. Overall they help keeping GCC high quality (which is 
not always easy with the rules in place, either...); sooner or later 
you'll appreciate that at least your productivity is not being limited 
by mainline being broken every other day (at least on i686-linux :-).

If you need additional clarifications, of course, replies are welcome 
either privately or on the mailing list.

Paolo

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Paolo Bonzini]

> I know what your answer will be to the following, but it's really
> becoming a pain to try to help solve issues with the compiler when all
> you get back is complaints that this and that and a third rule isn't
> being fulfilled.

All this is explained in http://gcc.gnu.org/contribute.html which is 
prominently linked in the home page.

> (bootstrapping the compiler takes hours, the testsuite runs take hours,

You can do that at night.  Testing a pile of changes together is usually 
not a problem, especially if well separated.  Still, once they have been 
reviewed and approved, you should take your time to test them one at a 
time; but then once you got the approval, you can take all the time you 
need before actually committing the changes.

> using the mainline is wasted time to me and my employer since we
> obviously can't/don't want to use this for general consumption, so
> importance to us have only changes on top of a released compiler),

Sorry, but that's not a valid point.  I don't think you can believe that 
GCC developers should pick up your 3.4 work and do the port to mainline 
themselves.  No matter if paid or volunteering, they have more 
interesting things to do.  The best solution to this problem is to 
submit the changes just after the beginning of stage1.  I think most GCC 
developers end stage3 with a queue of at least a dozen polished patches.

The point is that you/your employer had done it wrong by letting these 
changes pile up for two years.

> Whether everyone's pleased by them doesn't matter.
> Whether this severly limits productivity doesn't matter either.

I understand your position.  But the point is that these rules help 
maintainers reviewing your patch, they help developers understand what 
others have done, and they help people lend a hand when you have done 
something wrong. Overall they help keeping GCC high quality (which is 
not always easy with the rules in place, either...); sooner or later 
you'll appreciate that at least your productivity is not being limited 
by mainline being broken every other day (at least on i686-linux :-).

If you need additional clarifications, of course, replies are welcome 
either privately or on the mailing list.

Paolo

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Joseph S. Myers]

On Wed, 7 Jul 2004, Jan Beulich wrote:

> For the documentation requirements (I'm still learning doing 'open
> source', and previously I was used to there being someone else taking
> care of documentation stuff, after all I'm not a technical writer but a
> programmer): Can you suggest an obvious to use editor for the .texi
> files (not a plain text one, so I won't have to twiddle the directives
> in there a hundred times)?

As far as I know developers generally edit the files as plain text.  A
good discipline - though there's no requirement for this as long as the
final submitted patch follows all the requirements, and not all parts
necessarily apply to all patches - is:

* First write any documentation appropriate for the manuals for any new
features or interfaces: specify what the patch is intended to do.

* Then write the testcases for the features, by which you will know
whether the patch works and people later will know whether they've broken
it.  (The testcases should generally fail before you apply the patch, and
pass afterwards.)

* Then write the code itself; this may feed back to the previous parts if
special cases show up in the process that need documenting or testing.

* While you were doing these three, a baseline build can have been
running.  Apply the single patch to the baseline, and test the patched
tree (best to test with the *only* variable changed from the baseline
being the single patch, rather than using baseline results from a slightly
different tree or just eyeballing the test results rather than comparing
with a baseline).

* You can do something else while the tests of the patched tree are
running.

> For the unmentioned test case changes: Without them these tests fail if
> you have a target that requires less than word alignment as the default
> setting (which is what DEFAULT_PACK_STRUCT is used for). Thus for such
> targets the packing value has to be set high enough (and thus the value
> 8 to accomodate all targets up to 64 bits). It seemed inappropriate to
> add options for individual targets here when adding them globally does
> not do any harm), but it could of course be done.

I think you need more explanation of *why* the tests fail, and why they
can't be made more portable to systems with different alignment
requirements (while still testing what they're meant to test) rather than
needing special options.  And they still need including in the ChangeLog
entries.

> later running into the same problem again). As said above (and as got to
> hear previously), everyone's (supposedly) playing by these rules.
> Whether everyone's pleased by them doesn't matter. Whether this severly
> limits productivity doesn't matter either.

If anything, the rules should be even stricter, as the tree still seems to
be in an almost perpetual state of many regressions on multiple platforms.

Zack's paper
<http://www.codesourcery.com/public/publications/a_maintenance_programmers_view_of_gcc.pdf>
discusses some of the maintenance problems with GCC.  Weakening submission
requirements is not a useful solution if the tree is to be kept
high-quality.  Systematic cleanups (completing incomplete transitions, so
the existing code is more consistently a good example of how new code
should do things) may be.  In general contributors eventually learn to
follow the requirements automatically.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Roger Sayle]

On Wed, 7 Jul 2004, Jan Beulich wrote:
> I know what your answer will be to the following, but it's really
> becoming a pain to try to help solve issues with the compiler when all
> you get back is complaints that this and that and a third rule isn't
> being fulfilled.

Many GCC contributors have replied to your posting describing how
these bizarre rules are necessary to enable collaboration on an extremely
large and extremely complex piece of mission critical software like
GCC.  And indeed supporting a multi-million line code-base for nearly
twenty years is hard.  But little has been said on the intangible
benefits back to you as a GCC contributor.

	We choose to do ... these other things, not because they are
	easy, but because they are hard, because that goal will serve
	to organize and measure the best of our energies and skills.

Undoubtedly, GCC's software contribution policys and requirements are
some of the most stringent in the software development community.  As
a result, very few programmers ever attain the necessary discipline,
to contribute to GCC.  But those few software engineers that do, earn
the respect of their peers.

To use the "Cathedral and Bazaar" analogy of Eric Raymond, GCC developers
build cathedrals, awe inspiring edifices that often take generations
to construct, but the last for hundreds of years.

Very few developers that come upon GCC, state that the requirements are
"easier" or more "relaxed" than their in-house efforts.  The reason is
that the quality and level of ability demanded of GCC developers is
often far beyond what contributors experience at their work place or
in academia, or on other open source/free software projects.  As you've
mentioned, organizations often have specialist technical writers, Q&A
testers, software architects, build system administrators, platform
specialists and language specialists.  GCC however requires contributors
to be polymaths, able to write their own documentation, write their own
tests, and understand their effects on multiple architetures and at least
four different programming languages.  Not too mention the abilty to
communicate, get on well with others and resolve conflicts.  Most
organizations only have one or two people of the calibre to contribute
to GCC.


To quote Microsoft's David Gristwood's "21 Rules of Thumb - How Microsoft
develops its Software",
http://blogs.msdn.com/David_Gristwood/archive/2004/06/24/164849.aspx

	The complexity of mult-platform support is beyond the reach
	of most development organizations.


Obviously I'm biased, but I really believe that GCC's contributors are
amongst the best programmers on the planet, and I suspect I'm not alone.
I'd have no hesitation employing a GCC contributor, and I'm sure that
being able to say the same on my CV or resume helps my current employer
regard me as one of computer science's elite.


As for your recent almost unprecedented first-timer flurry of activity,
if your manage to survive the perils of rookie hazing and get even half
of your recent contributions into GCC, you'll have earned my respect.
Most new comers submit one or two patches at-at-time, and are able to
recoil/recover from reviewer comments, learn from their mistakes and
continue.


Dr Roger Sayle,
Ph.D. Computer Science,
GCC Maintainer.
--
Roger Sayle,                         E-mail: roger@eyesopen.com
OpenEye Scientific Software,         WWW: http://www.eyesopen.com/
Suite 1107, 3600 Cerrillos Road,     Tel: (+1) 505-473-7385
Santa Fe, New Mexico, 87507.         Fax: (+1) 505-473-0833

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Joseph S. Myers]

On Mon, 12 Jul 2004, Jan Beulich wrote:

> Second try, (hopefully) with all complaints addressed. Bootstrapped and
> tested on i686-pc-linux-gnu. Jan

I meant for the comments about why the testcases are changed to go as
comments in the testcases, rather than the ChangeLog.  The ChangeLog
entries should document what changed (i.e. that a certain option was added
to the testcase).  The comments in the testcase should explain why that
option is necessary, for future readers of the testcase.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2575 bytes --]

Second try, (hopefully) with all complaints addressed. Bootstrapped and
tested on i686-pc-linux-gnu. Jan

>>> "Joseph S. Myers" <jsm@polyomino.org.uk> 07.07.04 14:07:45 >>>
On Wed, 7 Jul 2004, Jan Beulich wrote:

> 2004-07-07 Jan Beulich <jbeulich@novell.com>
> 
> 	PR c/7054
> 	* tree.h (initial_max_fld_align): Declare
> 	* stor-layout.c (initial_max_fld_align): Define and initialize.
> 	(maximum_field_alignment): Initialize to the same value.
> 	* common.opt: Add -fpack-struct= variant of switch.
> 	* opts.c: Handle -fpack-struct= variant of switch.
> 	* c-pragma.c: Change #pragma pack() handling so that is becomes
> 	compatible to other compilers: accept individual 'push'
> argument,
> 	make final pop restore (command line) default, correct
> interaction
> 	of push/pop and sole specification of a new alignment (so that
> the
> 	sequence #pragma pack(push) - #pragma pack(<n>) becomes
> identical
> 	to #pragma pack(push, <n>).
> 	* testsuite/gcc.dg/pack-test-2.c: Adjust to permit and check
> 	#pragma pack(push).

We can't consider patches without proper documentation.  This needs (a)
 
documentation for the new command-line option syntax, (b)
documentation
for the pragma changes (and it seems for the pragma itself, which as
you
evidently understand what it's meant to do at present you're best
placed
to write), (c) for what seems to be a new target macro
DEFAULT_PACK_STRUCT.

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08
> 04:32:11.000000000 +0100
> +++
> 2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27
> 10:57:09.000000000 +0200

Not mentioned in the ChangeLog entry.  Why is this testcase being
changed?

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10
> 01:04:50.000000000 +0200
> +++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26
> 15:38:24.000000000 +0200

Likewise.

>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17
> 16:48:28.000000000 +0200
> +++
>
2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
> 17:19:15.000000000 +0200

Likewise.  It seems clearly inappropriate for standards testcases to
have
any special options like you're adding.  (But this testcase is expected
to
become obsolete, as the draft C99 TC2 (N1060) would remove the
defective
requirement from the standard.)

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/ 
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

[-- Attachment #2: gcc-mainline-pragma-pack.patch --]
[-- Type: application/octet-stream, Size: 18224 bytes --]

2004-07-07 Jan Beulich <jbeulich@novell.com>

	PR c/7054
	* defaults.h (TARGET_DEFAULT_PACK_STRUCT): Provide default.
	* tree.h (initial_max_fld_align): Declare
	* stor-layout.c (initial_max_fld_align): Define and initialize.
	(maximum_field_alignment): Initialize to the same value.
	* common.opt: Add -fpack-struct= variant of switch.
	* opts.c: Handle -fpack-struct= variant of switch.
	* c-pragma.c: Change #pragma pack() handling so that it becomes
	compatible to other compilers: accept individual 'push' argument,
	make final pop restore (command line) default, correct interaction
	of push/pop and sole specification of a new alignment (so that the
	sequence #pragma pack(push) - #pragma pack(<n>) becomes identical
	to #pragma pack(push, <n>).
	* doc/extend.texi: New node "Structure-Packing Pragmas" under
	"Pragmas", describing #pragma pack.
	* doc/invoke.texi: Document -fpack-struct=<n> variant of switch.
	* doc/tm.texi: Adjust description for HANDLE_PRAGMA_PACK_PUSH_POP.
	Document new TARGET_DEFAULT_PACK_STRUCT.

testsuite:
2004-07-07 Jan Beulich <jbeulich@novell.com>
	* gcc.dg/c99-flex-array-4.c: Add -fpack-struct=8 to provide a
	deterministic starting point for the alignment of structure fields,
	as this test may otherwise xpass.
	* gcc.dg/pack-test-2.c: Adjust to permit and check #pragma pack(push).
	* gcc.dg/Wpadded.c: Add -fpack-struct=8 to provide a deterministic
	starting point for the alignment of structure fields, as this test may
	otherwise fail.
	* g++.dg/abi/vbase10.C: Dito.

diff -Naur 2004-07-09.12.23/gcc/common.opt pragma-pack/gcc/common.opt
--- 2004-07-09.12.23/gcc/common.opt	2004-07-02 15:13:19.000000000 +0200
+++ pragma-pack/gcc/common.opt	2004-07-06 08:44:39.000000000 +0200
@@ -527,6 +527,10 @@
 Common Report Var(flag_pack_struct)
 Pack structure members together without holes
 
+fpack-struct=
+Common RejectNegative Joined UInteger
+-fpack-struct=<number>	Set initial maximum structure member alignment
+
 fpcc-struct-return
 Common Report Var(flag_pcc_struct_return,1) VarExists
 Return small aggregates in memory, not registers
diff -Naur 2004-07-09.12.23/gcc/c-pragma.c pragma-pack/gcc/c-pragma.c
--- 2004-07-09.12.23/gcc/c-pragma.c	2004-07-02 15:13:15.000000000 +0200
+++ pragma-pack/gcc/c-pragma.c	2004-07-06 08:44:39.000000000 +0200
@@ -42,7 +42,6 @@
 typedef struct align_stack GTY(())
 {
   int                  alignment;
-  unsigned int         num_pushes;
   tree                 id;
   struct align_stack * prev;
 } align_stack;
@@ -59,8 +58,9 @@
    happens, we restore the value to this, not to a value of 0 for
    maximum_field_alignment.  Value is in bits.  */
 static int default_alignment;
-#define SET_GLOBAL_ALIGNMENT(ALIGN) \
-  (default_alignment = maximum_field_alignment = (ALIGN))
+#define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment = *(alignment_stack == NULL \
+	? &default_alignment \
+	: &alignment_stack->alignment) = (ALIGN))
 
 static void push_alignment (int, tree);
 static void pop_alignment (tree);
@@ -69,31 +69,23 @@
 static void
 push_alignment (int alignment, tree id)
 {
-  if (alignment_stack == NULL
-      || alignment_stack->alignment != alignment
-      || id != NULL_TREE)
-    {
-      align_stack * entry;
-
-      entry = ggc_alloc (sizeof (* entry));
-
-      entry->alignment  = alignment;
-      entry->num_pushes = 1;
-      entry->id         = id;
-      entry->prev       = alignment_stack;
-      
-      /* The current value of maximum_field_alignment is not necessarily 
-	 0 since there may be a #pragma pack(<n>) in effect; remember it 
-	 so that we can restore it after the final #pragma pop().  */
-      if (alignment_stack == NULL)
-	default_alignment = maximum_field_alignment;
-      
-      alignment_stack = entry;
+  align_stack * entry;
 
-      maximum_field_alignment = alignment;
-    }
-  else
-    alignment_stack->num_pushes ++;
+  entry = ggc_alloc (sizeof (* entry));
+
+  entry->alignment  = alignment;
+  entry->id         = id;
+  entry->prev       = alignment_stack;
+       
+  /* The current value of maximum_field_alignment is not necessarily 
+     0 since there may be a #pragma pack(<n>) in effect; remember it 
+     so that we can restore it after the final #pragma pop().  */
+  if (alignment_stack == NULL)
+    default_alignment = maximum_field_alignment;
+ 
+  alignment_stack = entry;
+
+  maximum_field_alignment = alignment;
 }
 
 /* Undo a push of an alignment onto the stack.  */
@@ -103,12 +95,7 @@
   align_stack * entry;
       
   if (alignment_stack == NULL)
-    {
-      warning ("\
-#pragma pack (pop) encountered without matching #pragma pack (push, <n>)"
-	       );
-      return;
-    }
+    GCC_BAD("#pragma pack (pop) encountered without matching #pragma pack (push)");
 
   /* If we got an identifier, strip away everything above the target
      entry so that the next step will restore the state just below it.  */
@@ -117,27 +104,20 @@
       for (entry = alignment_stack; entry; entry = entry->prev)
 	if (entry->id == id)
 	  {
-	    entry->num_pushes = 1;
 	    alignment_stack = entry;
 	    break;
 	  }
       if (entry == NULL)
 	warning ("\
-#pragma pack(pop, %s) encountered without matching #pragma pack(push, %s, <n>)"
+#pragma pack(pop, %s) encountered without matching #pragma pack(push, %s)"
 		 , IDENTIFIER_POINTER (id), IDENTIFIER_POINTER (id));
     }
 
-  if (-- alignment_stack->num_pushes == 0)
-    {
-      entry = alignment_stack->prev;
+  entry = alignment_stack->prev;
 
-      if (entry == NULL)
-	maximum_field_alignment = default_alignment;
-      else
-	maximum_field_alignment = entry->alignment;
+  maximum_field_alignment = entry ? entry->alignment : default_alignment;
 
-      alignment_stack = entry;
-    }
+  alignment_stack = entry;
 }
 #else  /* not HANDLE_PRAGMA_PACK_PUSH_POP */
 #define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment = (ALIGN))
@@ -150,7 +130,9 @@
 /* #pragma pack ()
    #pragma pack (N)
    
+   #pragma pack (push)
    #pragma pack (push, N)
+   #pragma pack (push, ID)
    #pragma pack (push, ID, N)
    #pragma pack (pop)
    #pragma pack (pop, ID) */
@@ -169,7 +151,7 @@
   if (token == CPP_CLOSE_PAREN)
     {
       action = set;
-      align = 0;
+      align = initial_max_fld_align;
     }
   else if (token == CPP_NUMBER)
     {
@@ -180,8 +162,8 @@
     }
   else if (token == CPP_NAME)
     {
-#define GCC_BAD_ACTION do { if (action == push) \
-	  GCC_BAD ("malformed '#pragma pack(push[, id], <n>)' - ignored"); \
+#define GCC_BAD_ACTION do { if (action != pop) \
+	  GCC_BAD ("malformed '#pragma pack(push[, id][, <n>])' - ignored"); \
 	else \
 	  GCC_BAD ("malformed '#pragma pack(pop[, id])' - ignored"); \
 	} while (0)
@@ -194,31 +176,21 @@
       else
 	GCC_BAD2 ("unknown action '%s' for '#pragma pack' - ignored", op);
 
-      token = c_lex (&x);
-      if (token != CPP_COMMA && action == push)
-	GCC_BAD_ACTION;
-
-      if (token == CPP_COMMA)
+      while ((token = c_lex (&x)) == CPP_COMMA)
 	{
 	  token = c_lex (&x);
-	  if (token == CPP_NAME)
+	  if (token == CPP_NAME && id == 0)
 	    {
 	      id = x;
-	      if (action == push && c_lex (&x) != CPP_COMMA)
-		GCC_BAD_ACTION;
-	      token = c_lex (&x);
 	    }
-
-	  if (action == push)
+	  else if (token == CPP_NUMBER && action == push && align == -1)
 	    {
-	      if (token == CPP_NUMBER)
-		{
-		  align = TREE_INT_CST_LOW (x);
-		  token = c_lex (&x);
-		}
-	      else
-		GCC_BAD_ACTION;
+	      align = TREE_INT_CST_LOW (x);
+	      if (align == -1)
+		action = set;
 	    }
+	  else
+	    GCC_BAD_ACTION;
 	}
 
       if (token != CPP_CLOSE_PAREN)
@@ -231,6 +203,9 @@
   if (c_lex (&x) != CPP_EOF)
     warning ("junk at end of '#pragma pack'");
 
+  if (flag_pack_struct)
+    GCC_BAD ("#pragma pack has no effect with -fpack-struct - ignored");
+
   if (action != pop)
     switch (align)
       {
@@ -242,6 +217,12 @@
       case 16:
 	align *= BITS_PER_UNIT;
 	break;
+      case -1:
+	if (action == push)
+	  {
+	    align = maximum_field_alignment;
+	    break;
+	  }
       default:
 	GCC_BAD2 ("alignment must be a small power of two, not %d", align);
       }
diff -Naur 2004-07-09.12.23/gcc/defaults.h pragma-pack/gcc/defaults.h
--- 2004-07-09.12.23/gcc/defaults.h	2004-06-07 11:55:52.000000000 +0200
+++ pragma-pack/gcc/defaults.h	2004-07-09 14:53:15.638875688 +0200
@@ -461,6 +461,10 @@
 #define PREFERRED_STACK_BOUNDARY STACK_BOUNDARY
 #endif
 
+#ifndef TARGET_DEFAULT_PACK_STRUCT
+#define TARGET_DEFAULT_PACK_STRUCT 0
+#endif
+
 /* By default, the C++ compiler will use function addresses in the
    vtable entries.  Setting this nonzero tells the compiler to use
    function descriptors instead.  The value of this macro says how
diff -Naur 2004-07-09.12.23/gcc/doc/extend.texi pragma-pack/gcc/doc/extend.texi
--- 2004-07-09.12.23/gcc/doc/extend.texi	2004-07-09 12:17:18.000000000 +0200
+++ pragma-pack/gcc/doc/extend.texi	2004-07-09 12:45:45.850820240 +0200
@@ -7210,6 +7210,7 @@
 * RS/6000 and PowerPC Pragmas::
 * Darwin Pragmas::
 * Symbol-Renaming Pragmas::
+* Structure-Packing Pragmas::
 @end menu
 
 @node ARM Pragmas
@@ -7355,6 +7356,30 @@
 way of knowing that that happened.)
 @end enumerate
 
+@node Structure-Packing Pragmas
+@subsection Structure-Packing Pragmas
+
+For compatibility with Win32, GCC supports as set of @code{#pragma}
+directives which change the maximum alignment of members of structures,
+unions, and classes subsequently defined.  The @var{n} value below always
+is required to be a small power of two and specifies the new alignment
+in bytes.
+
+@enumerate
+@item @code{#pragma pack(@var{n})} simply sets the new alignment.
+@item @code{#pragma pack()} sets the alignment to the one that was in
+effect when compilation started (see also command line option
+@option{-fpack-struct[=<n>]} @pxref{Code Gen Options}).
+@item @code{#pragma pack(push[,@var{n}])} pushes the current alignment
+setting on an internal stack and then optionally sets the new alignment.
+@item @code{#pragma pack(pop)} restores the alignment setting to the one
+saved at the top of the internal stack (and removes that stack entry).
+Note that @code{#pragma pack([@var{n}])} does not influence this internal
+stack; thus it is possible to have @code{#pragma pack(push)} followed by
+multiple @code{#pragma pack(@var{n})} instances and finalized by a single
+@code{#pragma pack(pop)}.
+@end enumerate
+
 @node Unnamed Fields
 @section Unnamed struct/union fields within structs/unions.
 @cindex struct
diff -Naur 2004-07-09.12.23/gcc/doc/invoke.texi pragma-pack/gcc/doc/invoke.texi
--- 2004-07-09.12.23/gcc/doc/invoke.texi	2004-07-09 12:17:18.000000000 +0200
+++ pragma-pack/gcc/doc/invoke.texi	2004-07-09 12:45:45.930808080 +0200
@@ -689,7 +689,7 @@
 -fpcc-struct-return  -fpic  -fPIC -fpie -fPIE @gol
 -freg-struct-return  -fshared-data  -fshort-enums @gol
 -fshort-double  -fshort-wchar @gol
--fverbose-asm  -fpack-struct  -fstack-check @gol
+-fverbose-asm  -fpack-struct[=@var{n}]  -fstack-check @gol
 -fstack-limit-register=@var{reg}  -fstack-limit-symbol=@var{sym} @gol
 -fargument-alias  -fargument-noalias @gol
 -fargument-noalias-global  -fleading-underscore @gol
@@ -11476,9 +11476,13 @@
 This flag does not have a negative form, because it specifies a
 three-way choice.
 
-@item -fpack-struct
+@item -fpack-struct[=@var{n}]
 @opindex fpack-struct
-Pack all structure members together without holes.
+Without a value specified, pack all structure members together without
+holes. When a value is specified (which must be a small power of two), pack
+structure members according to this value, representing the maximum
+alignment (that is, objects with default alignment requirements larger than
+this will be output potentially unaligned at the next fitting location.
 
 @strong{Warning:} the @option{-fpack-struct} switch causes GCC to generate
 code that is not binary compatible with code generated without that switch.
diff -Naur 2004-07-09.12.23/gcc/doc/tm.texi pragma-pack/gcc/doc/tm.texi
--- 2004-07-09.12.23/gcc/doc/tm.texi	2004-07-07 15:59:38.000000000 +0200
+++ pragma-pack/gcc/doc/tm.texi	2004-07-08 16:51:18.000000000 +0200
@@ -8974,16 +8974,23 @@
 @findex pragma
 @defmac HANDLE_PRAGMA_PACK_PUSH_POP
 Define this macro (to a value of 1) if you want to support the Win32
-style pragmas @samp{#pragma pack(push,@var{n})} and @samp{#pragma
-pack(pop)}.  The @samp{pack(push,@var{n})} pragma specifies the maximum alignment
-(in bytes) of fields within a structure, in much the same way as the
-@samp{__aligned__} and @samp{__packed__} @code{__attribute__}s do.  A
+style pragmas @samp{#pragma pack(push[,@var{n}])} and @samp{#pragma
+pack(pop)}.  The @samp{pack(push,[@var{n}])} pragma specifies the maximum
+alignment (in bytes) of fields within a structure, in much the same way as
+the @samp{__aligned__} and @samp{__packed__} @code{__attribute__}s do.  A
 pack value of zero resets the behavior to the default.  Successive
 invocations of this pragma cause the previous values to be stacked, so
 that invocations of @samp{#pragma pack(pop)} will return to the previous
 value.
 @end defmac
 
+@defmac TARGET_DEFAULT_PACK_STRUCT
+If your target requires a structure packing default other than 0 (meaning
+the machine default), define this macro the the necessary value (in bytes).
+This must be a value that would also valid to be used with
+@samp{#pragma pack()} (that is, a small power of two).
+@end defmac
+
 @defmac DOLLARS_IN_IDENTIFIERS
 Define this macro to control use of the character @samp{$} in
 identifier names for the C family of languages.  0 means @samp{$} is
diff -Naur 2004-07-09.12.23/gcc/opts.c pragma-pack/gcc/opts.c
--- 2004-07-09.12.23/gcc/opts.c	2004-07-09 12:17:13.000000000 +0200
+++ pragma-pack/gcc/opts.c	2004-07-09 12:45:45.999797592 +0200
@@ -790,6 +790,13 @@
       pp_set_line_maximum_length (global_dc->printer, value);
       break;
 
+    case OPT_fpack_struct_:
+      if (value <= 0 || (value & (value - 1)) || value > 16)
+	error("structure alignment must be a small power of two, not %d", value);
+      else
+	maximum_field_alignment = initial_max_fld_align = value * BITS_PER_UNIT;
+      break;
+
     case OPT_fpeel_loops:
       flag_peel_loops_set = true;
       break;
diff -Naur 2004-07-09.12.23/gcc/stor-layout.c pragma-pack/gcc/stor-layout.c
--- 2004-07-09.12.23/gcc/stor-layout.c	2004-07-05 09:18:04.000000000 +0200
+++ pragma-pack/gcc/stor-layout.c	2004-07-08 13:27:05.000000000 +0200
@@ -48,7 +48,9 @@
 
 /* If nonzero, this is an upper limit on alignment of structure fields.
    The value is measured in bits.  */
-unsigned int maximum_field_alignment;
+unsigned int maximum_field_alignment = TARGET_DEFAULT_PACK_STRUCT * BITS_PER_UNIT;
+/* ... and the original value of it, specified via -fpack-struct=<value>. */
+unsigned int initial_max_fld_align = TARGET_DEFAULT_PACK_STRUCT * BITS_PER_UNIT;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in bits.
    May be overridden by front-ends.  */
diff -Naur 2004-07-09.12.23/gcc/testsuite/gcc.dg/c99-flex-array-4.c pragma-pack/gcc/testsuite/gcc.dg/c99-flex-array-4.c
--- 2004-07-09.12.23/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17 16:48:28.000000000 +0200
+++ pragma-pack/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26 17:19:15.000000000 +0200
@@ -5,7 +5,7 @@
    from Tony Finch <dot@dotat.at>, adapted to a testcase by Joseph Myers
    <jsm28@cam.ac.uk>.  See also WG14 reflector messages 9571-3.  */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1999 -fpack-struct=8 -pedantic-errors" } */
 
 #include <stddef.h>
 
diff -Naur 2004-07-09.12.23/gcc/testsuite/gcc.dg/pack-test-2.c pragma-pack/gcc/testsuite/gcc.dg/pack-test-2.c
--- 2004-07-09.12.23/gcc/testsuite/gcc.dg/pack-test-2.c	2000-12-13 21:16:33.000000000 +0100
+++ pragma-pack/gcc/testsuite/gcc.dg/pack-test-2.c	2004-05-18 16:17:53.000000000 +0200
@@ -3,9 +3,11 @@
 
 /* { dg-do compile { target *-*-linux* *-*-cygwin* powerpc*-*-eabi* } } */
 
-#pragma pack(push)              /* { dg-error "malformed" } */
 #pragma pack(pop)               /* { dg-error "without matching" } */
 
+#pragma pack(push)
+#pragma pack(pop)               /* reset */
+
 #pragma pack(push, foo, 1)
 #pragma pack(pop, foo, 1)       /* { dg-error "malformed" } (/
 #pragma pack(pop)               /* reset */
diff -Naur 2004-07-09.12.23/gcc/testsuite/gcc.dg/Wpadded.c pragma-pack/gcc/testsuite/gcc.dg/Wpadded.c
--- 2004-07-09.12.23/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10 01:04:50.000000000 +0200
+++ pragma-pack/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26 15:38:24.000000000 +0200
@@ -1,7 +1,7 @@
 /* Source: EMC.  */
 
 /* { dg-do compile } */
-/* { dg-options "-Wpadded" } */
+/* { dg-options "-Wpadded -fpack-struct=8" } */
 
 struct foo {
   char bar;
diff -Naur 2004-07-09.12.23/gcc/testsuite/g++.dg/abi/vbase10.C pragma-pack/gcc/testsuite/g++.dg/abi/vbase10.C
--- 2004-07-09.12.23/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08 04:32:11.000000000 +0100
+++ pragma-pack/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27 10:57:09.000000000 +0200
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options "-Wabi -fabi-version=1" }
+// { dg-options "-Wabi -fabi-version=1 -fpack-struct=8" }
 // On ARM processors, the alignment of B will be 4 even though it
 // contains only a single "char".  That would avoids the situation
 // that the warning below is designed to catch.  We therefore
diff -Naur 2004-07-09.12.23/gcc/tree.h pragma-pack/gcc/tree.h
--- 2004-07-09.12.23/gcc/tree.h	2004-07-09 12:17:14.000000000 +0200
+++ pragma-pack/gcc/tree.h	2004-07-09 12:45:46.064787712 +0200
@@ -3040,8 +3040,10 @@
    + (BITS_PER_UNIT > 8) + (BITS_PER_UNIT > 16) + (BITS_PER_UNIT > 32) \
    + (BITS_PER_UNIT > 64) + (BITS_PER_UNIT > 128) + (BITS_PER_UNIT > 256))
 
-/* If nonzero, an upper limit on alignment of structure fields, in bits.  */
+/* If nonzero, an upper limit on alignment of structure fields, in bits,  */
 extern unsigned int maximum_field_alignment;
+/* and the original value of it, specified via -fpack-struct=<value>. */
+extern unsigned int initial_max_fld_align;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in bits.  */
 extern unsigned int set_alignment;

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Joseph S. Myers]

On Fri, 9 Jul 2004, Jan Beulich wrote:

> 2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
> >> 17:19:15.000000000 +0200
> >
> >Likewise.  It seems clearly inappropriate for standards testcases to
> have
> >any special options like you're adding.  (But this testcase is
> expected to
> >become obsolete, as the draft C99 TC2 (N1060) would remove the
> defective
> >requirement from the standard.)
> 
> Same as above.

I presume you mean it XPASSes without the change, rather than it starts
failing?

> Generally, I'd like to understand the action I'm expected to take:
> Should I (a) leave the changes as-is, just documenting them in the
> ChangeLog, (b) conditionally pass options for *-*-netware*, or (c) xfail
> these tests for *-*-netware*. I chose (a) originally because the
> modifications aren't really target specific, since any target could
> choose to use other than word alignment as the default alignment.

I'd currently suggest (a) - with specific comments added to the testcases
explaining the presence of the option - but cannot speak for whoever may
actually review the revised patch.

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jan Beulich]

>>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08
>> 04:32:11.000000000 +0100
>> +++
>> 2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27
>> 10:57:09.000000000 +0200
>
>Not mentioned in the ChangeLog entry.  Why is this testcase being
changed?

Because it makes assumptions about the default alignment, without
enforcing a certain default alignment.

>>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10
>> 01:04:50.000000000 +0200
>> +++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26
>> 15:38:24.000000000 +0200
>
>Likewise.

Similarly, it expects behavior that you can't expect when the structure
packing is too low. Thus I was forcing the packing higher.

>>
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17
>> 16:48:28.000000000 +0200
>> +++
>>
2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
>> 17:19:15.000000000 +0200
>
>Likewise.  It seems clearly inappropriate for standards testcases to
have
>any special options like you're adding.  (But this testcase is
expected to
>become obsolete, as the draft C99 TC2 (N1060) would remove the
defective
>requirement from the standard.)

Same as above.

Generally, I'd like to understand the action I'm expected to take:
Should I (a) leave the changes as-is, just documenting them in the
ChangeLog, (b) conditionally pass options for *-*-netware*, or (c) xfail
these tests for *-*-netware*. I chose (a) originally because the
modifications aren't really target specific, since any target could
choose to use other than word alignment as the default alignment.

Thanks, Jan

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jason Merrill]

On Wed, 07 Jul 2004 17:40:45 +0200, "Jan Beulich" <JBeulich@novell.com> wrote:

> One thing I simply forgot to add before: With all these nice rules, why
> is it then that, in order to be sure my changes don't add regression
> failures, I have to first bootstrap an unmodified compiler and run the
> testsuite on it (so to identify all current failures), in order to then
> run the same stuff again to finally compare the set of failures with my
> changes to that without.

Yes, this has been an annoyance for quite a while.  At the GCC Summit in
June we agreed that the steady state should be to have no failures in the
testsuite; if we consciously decide to leave a test failing for more than a
couple of days, it should be xfailed and a bug filed in bugzilla.

This hasn't happened yet, but it is the goal, and should avoid the need for
a baseline run.

Jason

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Paolo Bonzini]

> One thing I simply forgot to add before: With all these nice rules, why
> is it then that, in order to be sure my changes don't add regression
> failures, I have to first bootstrap an unmodified compiler and run the
> testsuite on it (so to identify all current failures), in order to then
> run the same stuff again to finally compare the set of failures with my
> changes to that without.

Almost often you just end up memorizing the failures on an unmodified 
compiler.  If you have some surprise, just check on gcc-testresults if 
somebody else had the same failure on i686-pc-linux-gnu.  If not, 
something wrong's with your patch, so you either debug the failure, or 
run the unpatched compiler.

> Certain tests appear to take quite long to compile (I measured 4 minutes
> on an otherwise idle system),

Ah, that's 20001226-1.c :-)

> then such tests are prone to time out (and thus
> fail).

No, if you find that the particular test is prone to time out, just run 
that test on its own, and see if it fails again.  Occasionally, failures 
may be introduced because of flaky RAMs or (especially on laptops) high 
CPU temperature.  Then run the single test, and if it is a small one you 
can run valgrind on cc1 to look for uninitialized memory -- if there's 
none, and it passes 2-3 times, you can be confident with your patch.

However bad this sentence may look, it's just a matter of getting 
accustomed to it.  More expert contributors have faster review times 
also because they know how to please the reviewers (splitting patches, 
writing good descriptions and preparing good changelogs).  Contributing 
to a big piece of open source software like GCC is harder despite the 
non-trivial amount of time that the global maintainers put in helping 
volunteers like us.  Suffice it to look at the volume of the GCC mailing 
lists.

Really, there's nothing we can do about it except listen for alternative 
proposals.  As long as they can handle a project with about 200 
developers, with a changes log taking a printed page per day (if you're 
lucky), and as incredibly data-driven (and thus prone to exposing latent 
bugs) as GCC.

Read Zack Weinberg's paper about GCC maintainance on 
www.codesourcery.com and you'll see that people often do not like the 
rules, but recognize that each one of them is necessary and ends up 
*saving* their time rather than wasting it.

Paolo

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Diego Novillo]

On Wed, 2004-07-07 at 11:40, Jan Beulich wrote:
> One thing I simply forgot to add before: With all these nice rules, why
> is it then that, in order to be sure my changes don't add regression
> failures, I have to first bootstrap an unmodified compiler and run the
> testsuite on it (so to identify all current failures), in order to then
> run the same stuff again to finally compare the set of failures with my
> changes to that without.
> 
You are joking, right?  How else do you propose to find regressions? 
There's a reason they are called 'regression tests'.

> Note that while this is 'only' machine time, there is an implied
> restriction on the number of things you can do on a single machine:
> Certain tests appear to take quite long to compile (I measured 4 minutes
> on an otherwise idle system), and thus when I run something else equally
> computation intensive (i.e. another bootstrap or testsuite) on the same
> machine (of which I have only one or at best two that I can afford doing
> this sort of stuff), then such tests are prone to time out (and thus
> fail). As a consequence, this has to be done serially, which in turn
> collides with the intention to do all this against sufficiently recent
> shnapshots of the cvs.
> 
Yes.  It's a problem.  We all live with it.  If you find an alternate
approach, you'll be very popular.

The problem is two-fold: on one hand, the compiler has been getting
slower through a combination of more features, language support, and
some implementation problems.  We are trying to fix the latter, but
there's little we can do about the first two.

On the other hand, we don't really have enough tests in our testsuite.  
As time goes by, the testsuite will only grow (fixed bugs often become a
new test).  By its very nature, a regression testsuite forces you to run
both a clean and a patched compiler and compare the before and after
results.

There's always the chance that your patch didn't introduce regressions,
but a 2nd patch committed between your test and commit combined with
your patch introduce a new regression.  It's happened before and we just
deal with that problem afterward.  Sometimes by reverting one of the two
patches, more often by adding a 3rd patch to fix the new problem.

There are other kinds of regressions that your patch may need to be
tested against: compile and/or runtime regressions.  Those are sometimes
trickier to catch and we don't have good automatic ways of checking
those.  There are some performance testers running SPEC daily, but
coverage is spotty at best.

Yes, it's like herding cats.


Diego.

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jan Beulich]

One thing I simply forgot to add before: With all these nice rules, why
is it then that, in order to be sure my changes don't add regression
failures, I have to first bootstrap an unmodified compiler and run the
testsuite on it (so to identify all current failures), in order to then
run the same stuff again to finally compare the set of failures with my
changes to that without.

Note that while this is 'only' machine time, there is an implied
restriction on the number of things you can do on a single machine:
Certain tests appear to take quite long to compile (I measured 4 minutes
on an otherwise idle system), and thus when I run something else equally
computation intensive (i.e. another bootstrap or testsuite) on the same
machine (of which I have only one or at best two that I can afford doing
this sort of stuff), then such tests are prone to time out (and thus
fail). As a consequence, this has to be done serially, which in turn
collides with the intention to do all this against sufficiently recent
shnapshots of the cvs.

Jan

>>> Paolo Bonzini <bonzini@gnu.org> 07.07.04 17:20:18 >>>
> I know what your answer will be to the following, but it's really
> becoming a pain to try to help solve issues with the compiler when
all
> you get back is complaints that this and that and a third rule isn't
> being fulfilled.

All this is explained in http://gcc.gnu.org/contribute.html which is 
prominently linked in the home page.

> (bootstrapping the compiler takes hours, the testsuite runs take
hours,

You can do that at night.  Testing a pile of changes together is
usually 
not a problem, especially if well separated.  Still, once they have
been 
reviewed and approved, you should take your time to test them one at a

time; but then once you got the approval, you can take all the time you

need before actually committing the changes.

> using the mainline is wasted time to me and my employer since we
> obviously can't/don't want to use this for general consumption, so
> importance to us have only changes on top of a released compiler),

Sorry, but that's not a valid point.  I don't think you can believe
that 
GCC developers should pick up your 3.4 work and do the port to mainline

themselves.  No matter if paid or volunteering, they have more 
interesting things to do.  The best solution to this problem is to 
submit the changes just after the beginning of stage1.  I think most
GCC 
developers end stage3 with a queue of at least a dozen polished
patches.

The point is that you/your employer had done it wrong by letting these

changes pile up for two years.

> Whether everyone's pleased by them doesn't matter.
> Whether this severly limits productivity doesn't matter either.

I understand your position.  But the point is that these rules help 
maintainers reviewing your patch, they help developers understand what

others have done, and they help people lend a hand when you have done 
something wrong. Overall they help keeping GCC high quality (which is 
not always easy with the rules in place, either...); sooner or later 
you'll appreciate that at least your productivity is not being limited

by mainline being broken every other day (at least on i686-linux :-).

If you need additional clarifications, of course, replies are welcome 
either privately or on the mailing list.

Paolo

Re: make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Joseph S. Myers]

On Wed, 7 Jul 2004, Jan Beulich wrote:

> 2004-07-07 Jan Beulich <jbeulich@novell.com>
> 
> 	PR c/7054
> 	* tree.h (initial_max_fld_align): Declare
> 	* stor-layout.c (initial_max_fld_align): Define and initialize.
> 	(maximum_field_alignment): Initialize to the same value.
> 	* common.opt: Add -fpack-struct= variant of switch.
> 	* opts.c: Handle -fpack-struct= variant of switch.
> 	* c-pragma.c: Change #pragma pack() handling so that is becomes
> 	compatible to other compilers: accept individual 'push'
> argument,
> 	make final pop restore (command line) default, correct
> interaction
> 	of push/pop and sole specification of a new alignment (so that
> the
> 	sequence #pragma pack(push) - #pragma pack(<n>) becomes
> identical
> 	to #pragma pack(push, <n>).
> 	* testsuite/gcc.dg/pack-test-2.c: Adjust to permit and check
> 	#pragma pack(push).

We can't consider patches without proper documentation.  This needs (a)  
documentation for the new command-line option syntax, (b) documentation
for the pragma changes (and it seems for the pragma itself, which as you
evidently understand what it's meant to do at present you're best placed
to write), (c) for what seems to be a new target macro
DEFAULT_PACK_STRUCT.

> /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08
> 04:32:11.000000000 +0100
> +++
> 2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27
> 10:57:09.000000000 +0200

Not mentioned in the ChangeLog entry.  Why is this testcase being changed?

> /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10
> 01:04:50.000000000 +0200
> +++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26
> 15:38:24.000000000 +0200

Likewise.

> /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17
> 16:48:28.000000000 +0200
> +++
> 2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
> 17:19:15.000000000 +0200

Likewise.  It seems clearly inappropriate for standards testcases to have
any special options like you're adding.  (But this testcase is expected to
become obsolete, as the draft C99 TC2 (N1060) would remove the defective
requirement from the standard.)

-- 
Joseph S. Myers               http://www.srcf.ucam.org/~jsm28/gcc/
    jsm@polyomino.org.uk (personal mail)
    jsm28@gcc.gnu.org (Bugzilla assignments and CCs)

make #pragma pack() implementation consistent with other compilers (PR c/7054)

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 11637 bytes --]

2004-07-07 Jan Beulich <jbeulich@novell.com>

	PR c/7054
	* tree.h (initial_max_fld_align): Declare
	* stor-layout.c (initial_max_fld_align): Define and initialize.
	(maximum_field_alignment): Initialize to the same value.
	* common.opt: Add -fpack-struct= variant of switch.
	* opts.c: Handle -fpack-struct= variant of switch.
	* c-pragma.c: Change #pragma pack() handling so that is becomes
	compatible to other compilers: accept individual 'push'
argument,
	make final pop restore (command line) default, correct
interaction
	of push/pop and sole specification of a new alignment (so that
the
	sequence #pragma pack(push) - #pragma pack(<n>) becomes
identical
	to #pragma pack(push, <n>).
	* testsuite/gcc.dg/pack-test-2.c: Adjust to permit and check
	#pragma pack(push).

---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/c-pragma.c	2004-07-02
15:13:15.000000000 +0200
+++ 2004-07-05.10.09/gcc/c-pragma.c	2004-07-06 08:44:38.943513912
+0200
@@ -42,7 +42,6 @@
 typedef struct align_stack GTY(())
 {
   int                  alignment;
-  unsigned int         num_pushes;
   tree                 id;
   struct align_stack * prev;
 } align_stack;
@@ -59,8 +58,9 @@
    happens, we restore the value to this, not to a value of 0 for
    maximum_field_alignment.  Value is in bits.  */
 static int default_alignment;
-#define SET_GLOBAL_ALIGNMENT(ALIGN) \
-  (default_alignment = maximum_field_alignment = (ALIGN))
+#define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment =
*(alignment_stack == NULL \
+	? &default_alignment \
+	: &alignment_stack->alignment) = (ALIGN))
 
 static void push_alignment (int, tree);
 static void pop_alignment (tree);
@@ -69,31 +69,23 @@
 static void
 push_alignment (int alignment, tree id)
 {
-  if (alignment_stack == NULL
-      || alignment_stack->alignment != alignment
-      || id != NULL_TREE)
-    {
-      align_stack * entry;
-
-      entry = ggc_alloc (sizeof (* entry));
-
-      entry->alignment  = alignment;
-      entry->num_pushes = 1;
-      entry->id         = id;
-      entry->prev       = alignment_stack;
-      
-      /* The current value of maximum_field_alignment is not
necessarily 
-	 0 since there may be a #pragma pack(<n>) in effect; remember it

-	 so that we can restore it after the final #pragma pop().  */
-      if (alignment_stack == NULL)
-	default_alignment = maximum_field_alignment;
-      
-      alignment_stack = entry;
+  align_stack * entry;
 
-      maximum_field_alignment = alignment;
-    }
-  else
-    alignment_stack->num_pushes ++;
+  entry = ggc_alloc (sizeof (* entry));
+
+  entry->alignment  = alignment;
+  entry->id         = id;
+  entry->prev       = alignment_stack;
+       
+  /* The current value of maximum_field_alignment is not necessarily 
+     0 since there may be a #pragma pack(<n>) in effect; remember it 
+     so that we can restore it after the final #pragma pop().  */
+  if (alignment_stack == NULL)
+    default_alignment = maximum_field_alignment;
+ 
+  alignment_stack = entry;
+
+  maximum_field_alignment = alignment;
 }
 
 /* Undo a push of an alignment onto the stack.  */
@@ -103,12 +95,7 @@
   align_stack * entry;
       
   if (alignment_stack == NULL)
-    {
-      warning ("\
-#pragma pack (pop) encountered without matching #pragma pack (push,
<n>)"
-	       );
-      return;
-    }
+    GCC_BAD("#pragma pack (pop) encountered without matching #pragma
pack (push)");
 
   /* If we got an identifier, strip away everything above the target
      entry so that the next step will restore the state just below it.
 */
@@ -117,27 +104,20 @@
       for (entry = alignment_stack; entry; entry = entry->prev)
 	if (entry->id == id)
 	  {
-	    entry->num_pushes = 1;
 	    alignment_stack = entry;
 	    break;
 	  }
       if (entry == NULL)
 	warning ("\
-#pragma pack(pop, %s) encountered without matching #pragma pack(push,
%s, <n>)"
+#pragma pack(pop, %s) encountered without matching #pragma pack(push,
%s)"
 		 , IDENTIFIER_POINTER (id), IDENTIFIER_POINTER (id));
     }
 
-  if (-- alignment_stack->num_pushes == 0)
-    {
-      entry = alignment_stack->prev;
+  entry = alignment_stack->prev;
 
-      if (entry == NULL)
-	maximum_field_alignment = default_alignment;
-      else
-	maximum_field_alignment = entry->alignment;
+  maximum_field_alignment = entry ? entry->alignment :
default_alignment;
 
-      alignment_stack = entry;
-    }
+  alignment_stack = entry;
 }
 #else  /* not HANDLE_PRAGMA_PACK_PUSH_POP */
 #define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment =
(ALIGN))
@@ -150,7 +130,9 @@
 /* #pragma pack ()
    #pragma pack (N)
    
+   #pragma pack (push)
    #pragma pack (push, N)
+   #pragma pack (push, ID)
    #pragma pack (push, ID, N)
    #pragma pack (pop)
    #pragma pack (pop, ID) */
@@ -169,7 +151,7 @@
   if (token == CPP_CLOSE_PAREN)
     {
       action = set;
-      align = 0;
+      align = initial_max_fld_align;
     }
   else if (token == CPP_NUMBER)
     {
@@ -180,8 +162,8 @@
     }
   else if (token == CPP_NAME)
     {
-#define GCC_BAD_ACTION do { if (action == push) \
-	  GCC_BAD ("malformed '#pragma pack(push[, id], <n>)' -
ignored"); \
+#define GCC_BAD_ACTION do { if (action != pop) \
+	  GCC_BAD ("malformed '#pragma pack(push[, id][, <n>])' -
ignored"); \
 	else \
 	  GCC_BAD ("malformed '#pragma pack(pop[, id])' - ignored"); \
 	} while (0)
@@ -194,31 +176,21 @@
       else
 	GCC_BAD2 ("unknown action '%s' for '#pragma pack' - ignored",
op);
 
-      token = c_lex (&x);
-      if (token != CPP_COMMA && action == push)
-	GCC_BAD_ACTION;
-
-      if (token == CPP_COMMA)
+      while ((token = c_lex (&x)) == CPP_COMMA)
 	{
 	  token = c_lex (&x);
-	  if (token == CPP_NAME)
+	  if (token == CPP_NAME && id == 0)
 	    {
 	      id = x;
-	      if (action == push && c_lex (&x) != CPP_COMMA)
-		GCC_BAD_ACTION;
-	      token = c_lex (&x);
 	    }
-
-	  if (action == push)
+	  else if (token == CPP_NUMBER && action == push && align ==
-1)
 	    {
-	      if (token == CPP_NUMBER)
-		{
-		  align = TREE_INT_CST_LOW (x);
-		  token = c_lex (&x);
-		}
-	      else
-		GCC_BAD_ACTION;
+	      align = TREE_INT_CST_LOW (x);
+	      if (align == -1)
+		action = set;
 	    }
+	  else
+	    GCC_BAD_ACTION;
 	}
 
       if (token != CPP_CLOSE_PAREN)
@@ -231,6 +203,9 @@
   if (c_lex (&x) != CPP_EOF)
     warning ("junk at end of '#pragma pack'");
 
+  if (flag_pack_struct)
+    GCC_BAD ("#pragma pack has no effect with -fpack-struct -
ignored");
+
   if (action != pop)
     switch (align)
       {
@@ -242,6 +217,12 @@
       case 16:
 	align *= BITS_PER_UNIT;
 	break;
+      case -1:
+	if (action == push)
+	  {
+	    align = maximum_field_alignment;
+	    break;
+	  }
       default:
 	GCC_BAD2 ("alignment must be a small power of two, not %d",
align);
       }
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/common.opt	2004-07-02
15:13:19.000000000 +0200
+++ 2004-07-05.10.09/gcc/common.opt	2004-07-06 08:44:38.935515128
+0200
@@ -527,6 +527,10 @@
 Common Report Var(flag_pack_struct)
 Pack structure members together without holes
 
+fpack-struct=
+Common RejectNegative Joined UInteger
+-fpack-struct=<number>	Set initial maximum structure member
alignment
+
 fpcc-struct-return
 Common Report Var(flag_pcc_struct_return,1) VarExists
 Return small aggregates in memory, not registers
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/opts.c	2004-07-02
15:13:53.000000000 +0200
+++ 2004-07-05.10.09/gcc/opts.c	2004-07-06 08:44:38.969509960
+0200
@@ -790,6 +790,13 @@
       pp_set_line_maximum_length (global_dc->printer, value);
       break;
 
+    case OPT_fpack_struct_:
+      if (value <= 0 || (value & (value - 1)) || value > 16)
+	error("structure alignment must be a small power of two, not
%d", value);
+      else
+	maximum_field_alignment = initial_max_fld_align = value *
BITS_PER_UNIT;
+      break;
+
     case OPT_fpeel_loops:
       flag_peel_loops_set = true;
       break;
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/stor-layout.c	2004-07-05
09:18:04.000000000 +0200
+++ 2004-07-05.10.09/gcc/stor-layout.c	2004-07-06 08:44:38.986507376
+0200
@@ -48,7 +48,12 @@
 
 /* If nonzero, this is an upper limit on alignment of structure
fields.
    The value is measured in bits.  */
-unsigned int maximum_field_alignment;
+#ifndef DEFAULT_PACK_STRUCT
+#define DEFAULT_PACK_STRUCT 0
+#endif
+unsigned int maximum_field_alignment = DEFAULT_PACK_STRUCT *
BITS_PER_UNIT;
+/* ... and the original value of it, specified via
-fpack-struct=<value>. */
+unsigned int initial_max_fld_align = DEFAULT_PACK_STRUCT *
BITS_PER_UNIT;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in
bits.
    May be overridden by front-ends.  */
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08
04:32:11.000000000 +0100
+++
2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27
10:57:09.000000000 +0200
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options "-Wabi -fabi-version=1" }
+// { dg-options "-Wabi -fabi-version=1 -fpack-struct=8" }
 // On ARM processors, the alignment of B will be 4 even though it
 // contains only a single "char".  That would avoids the situation
 // that the warning below is designed to catch.  We therefore
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10
01:04:50.000000000 +0200
+++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26
15:38:24.000000000 +0200
@@ -1,7 +1,7 @@
 /* Source: EMC.  */
 
 /* { dg-do compile } */
-/* { dg-options "-Wpadded" } */
+/* { dg-options "-Wpadded -fpack-struct=8" } */
 
 struct foo {
   char bar;
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17
16:48:28.000000000 +0200
+++
2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26
17:19:15.000000000 +0200
@@ -5,7 +5,7 @@
    from Tony Finch <dot@dotat.at>, adapted to a testcase by Joseph
Myers
    <jsm28@cam.ac.uk>.  See also WG14 reflector messages 9571-3.  */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1999 -fpack-struct=8 -pedantic-errors" }
*/
 
 #include <stddef.h>
 
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/pack-test-2.c	2000-12-13
21:16:33.000000000 +0100
+++
2004-07-05.10.09/gcc/testsuite/gcc.dg/pack-test-2.c	2004-05-18
16:17:53.000000000 +0200
@@ -3,9 +3,11 @@
 
 /* { dg-do compile { target *-*-linux* *-*-cygwin* powerpc*-*-eabi* }
} */
 
-#pragma pack(push)              /* { dg-error "malformed" } */
 #pragma pack(pop)               /* { dg-error "without matching" } */
 
+#pragma pack(push)
+#pragma pack(pop)               /* reset */
+
 #pragma pack(push, foo, 1)
 #pragma pack(pop, foo, 1)       /* { dg-error "malformed" } (/
 #pragma pack(pop)               /* reset */
---
/home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/tree.h	2004-07-05
09:18:05.000000000 +0200
+++ 2004-07-05.10.09/gcc/tree.h	2004-07-06 08:44:39.045498408
+0200
@@ -3026,8 +3026,10 @@
    + (BITS_PER_UNIT > 8) + (BITS_PER_UNIT > 16) + (BITS_PER_UNIT > 32)
\
    + (BITS_PER_UNIT > 64) + (BITS_PER_UNIT > 128) + (BITS_PER_UNIT >
256))
 
-/* If nonzero, an upper limit on alignment of structure fields, in
bits.  */
+/* If nonzero, an upper limit on alignment of structure fields, in
bits,  */
 extern unsigned int maximum_field_alignment;
+/* and the original value of it, specified via -fpack-struct=<value>.
*/
+extern unsigned int initial_max_fld_align;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in
bits.  */
 extern unsigned int set_alignment;


[-- Attachment #2: gcc-mainline-pragma-pack.patch --]
[-- Type: application/octet-stream, Size: 11636 bytes --]

2004-07-07 Jan Beulich <jbeulich@novell.com>

	PR c/7054
	* tree.h (initial_max_fld_align): Declare
	* stor-layout.c (initial_max_fld_align): Define and initialize.
	(maximum_field_alignment): Initialize to the same value.
	* common.opt: Add -fpack-struct= variant of switch.
	* opts.c: Handle -fpack-struct= variant of switch.
	* c-pragma.c: Change #pragma pack() handling so that is becomes
	compatible to other compilers: accept individual 'push' argument,
	make final pop restore (command line) default, correct interaction
	of push/pop and sole specification of a new alignment (so that the
	sequence #pragma pack(push) - #pragma pack(<n>) becomes identical
	to #pragma pack(push, <n>).
	* testsuite/gcc.dg/pack-test-2.c: Adjust to permit and check
	#pragma pack(push).

--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/c-pragma.c	2004-07-02 15:13:15.000000000 +0200
+++ 2004-07-05.10.09/gcc/c-pragma.c	2004-07-06 08:44:38.943513912 +0200
@@ -42,7 +42,6 @@
 typedef struct align_stack GTY(())
 {
   int                  alignment;
-  unsigned int         num_pushes;
   tree                 id;
   struct align_stack * prev;
 } align_stack;
@@ -59,8 +58,9 @@
    happens, we restore the value to this, not to a value of 0 for
    maximum_field_alignment.  Value is in bits.  */
 static int default_alignment;
-#define SET_GLOBAL_ALIGNMENT(ALIGN) \
-  (default_alignment = maximum_field_alignment = (ALIGN))
+#define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment = *(alignment_stack == NULL \
+	? &default_alignment \
+	: &alignment_stack->alignment) = (ALIGN))
 
 static void push_alignment (int, tree);
 static void pop_alignment (tree);
@@ -69,31 +69,23 @@
 static void
 push_alignment (int alignment, tree id)
 {
-  if (alignment_stack == NULL
-      || alignment_stack->alignment != alignment
-      || id != NULL_TREE)
-    {
-      align_stack * entry;
-
-      entry = ggc_alloc (sizeof (* entry));
-
-      entry->alignment  = alignment;
-      entry->num_pushes = 1;
-      entry->id         = id;
-      entry->prev       = alignment_stack;
-      
-      /* The current value of maximum_field_alignment is not necessarily 
-	 0 since there may be a #pragma pack(<n>) in effect; remember it 
-	 so that we can restore it after the final #pragma pop().  */
-      if (alignment_stack == NULL)
-	default_alignment = maximum_field_alignment;
-      
-      alignment_stack = entry;
+  align_stack * entry;
 
-      maximum_field_alignment = alignment;
-    }
-  else
-    alignment_stack->num_pushes ++;
+  entry = ggc_alloc (sizeof (* entry));
+
+  entry->alignment  = alignment;
+  entry->id         = id;
+  entry->prev       = alignment_stack;
+       
+  /* The current value of maximum_field_alignment is not necessarily 
+     0 since there may be a #pragma pack(<n>) in effect; remember it 
+     so that we can restore it after the final #pragma pop().  */
+  if (alignment_stack == NULL)
+    default_alignment = maximum_field_alignment;
+ 
+  alignment_stack = entry;
+
+  maximum_field_alignment = alignment;
 }
 
 /* Undo a push of an alignment onto the stack.  */
@@ -103,12 +95,7 @@
   align_stack * entry;
       
   if (alignment_stack == NULL)
-    {
-      warning ("\
-#pragma pack (pop) encountered without matching #pragma pack (push, <n>)"
-	       );
-      return;
-    }
+    GCC_BAD("#pragma pack (pop) encountered without matching #pragma pack (push)");
 
   /* If we got an identifier, strip away everything above the target
      entry so that the next step will restore the state just below it.  */
@@ -117,27 +104,20 @@
       for (entry = alignment_stack; entry; entry = entry->prev)
 	if (entry->id == id)
 	  {
-	    entry->num_pushes = 1;
 	    alignment_stack = entry;
 	    break;
 	  }
       if (entry == NULL)
 	warning ("\
-#pragma pack(pop, %s) encountered without matching #pragma pack(push, %s, <n>)"
+#pragma pack(pop, %s) encountered without matching #pragma pack(push, %s)"
 		 , IDENTIFIER_POINTER (id), IDENTIFIER_POINTER (id));
     }
 
-  if (-- alignment_stack->num_pushes == 0)
-    {
-      entry = alignment_stack->prev;
+  entry = alignment_stack->prev;
 
-      if (entry == NULL)
-	maximum_field_alignment = default_alignment;
-      else
-	maximum_field_alignment = entry->alignment;
+  maximum_field_alignment = entry ? entry->alignment : default_alignment;
 
-      alignment_stack = entry;
-    }
+  alignment_stack = entry;
 }
 #else  /* not HANDLE_PRAGMA_PACK_PUSH_POP */
 #define SET_GLOBAL_ALIGNMENT(ALIGN) (maximum_field_alignment = (ALIGN))
@@ -150,7 +130,9 @@
 /* #pragma pack ()
    #pragma pack (N)
    
+   #pragma pack (push)
    #pragma pack (push, N)
+   #pragma pack (push, ID)
    #pragma pack (push, ID, N)
    #pragma pack (pop)
    #pragma pack (pop, ID) */
@@ -169,7 +151,7 @@
   if (token == CPP_CLOSE_PAREN)
     {
       action = set;
-      align = 0;
+      align = initial_max_fld_align;
     }
   else if (token == CPP_NUMBER)
     {
@@ -180,8 +162,8 @@
     }
   else if (token == CPP_NAME)
     {
-#define GCC_BAD_ACTION do { if (action == push) \
-	  GCC_BAD ("malformed '#pragma pack(push[, id], <n>)' - ignored"); \
+#define GCC_BAD_ACTION do { if (action != pop) \
+	  GCC_BAD ("malformed '#pragma pack(push[, id][, <n>])' - ignored"); \
 	else \
 	  GCC_BAD ("malformed '#pragma pack(pop[, id])' - ignored"); \
 	} while (0)
@@ -194,31 +176,21 @@
       else
 	GCC_BAD2 ("unknown action '%s' for '#pragma pack' - ignored", op);
 
-      token = c_lex (&x);
-      if (token != CPP_COMMA && action == push)
-	GCC_BAD_ACTION;
-
-      if (token == CPP_COMMA)
+      while ((token = c_lex (&x)) == CPP_COMMA)
 	{
 	  token = c_lex (&x);
-	  if (token == CPP_NAME)
+	  if (token == CPP_NAME && id == 0)
 	    {
 	      id = x;
-	      if (action == push && c_lex (&x) != CPP_COMMA)
-		GCC_BAD_ACTION;
-	      token = c_lex (&x);
 	    }
-
-	  if (action == push)
+	  else if (token == CPP_NUMBER && action == push && align == -1)
 	    {
-	      if (token == CPP_NUMBER)
-		{
-		  align = TREE_INT_CST_LOW (x);
-		  token = c_lex (&x);
-		}
-	      else
-		GCC_BAD_ACTION;
+	      align = TREE_INT_CST_LOW (x);
+	      if (align == -1)
+		action = set;
 	    }
+	  else
+	    GCC_BAD_ACTION;
 	}
 
       if (token != CPP_CLOSE_PAREN)
@@ -231,6 +203,9 @@
   if (c_lex (&x) != CPP_EOF)
     warning ("junk at end of '#pragma pack'");
 
+  if (flag_pack_struct)
+    GCC_BAD ("#pragma pack has no effect with -fpack-struct - ignored");
+
   if (action != pop)
     switch (align)
       {
@@ -242,6 +217,12 @@
       case 16:
 	align *= BITS_PER_UNIT;
 	break;
+      case -1:
+	if (action == push)
+	  {
+	    align = maximum_field_alignment;
+	    break;
+	  }
       default:
 	GCC_BAD2 ("alignment must be a small power of two, not %d", align);
       }
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/common.opt	2004-07-02 15:13:19.000000000 +0200
+++ 2004-07-05.10.09/gcc/common.opt	2004-07-06 08:44:38.935515128 +0200
@@ -527,6 +527,10 @@
 Common Report Var(flag_pack_struct)
 Pack structure members together without holes
 
+fpack-struct=
+Common RejectNegative Joined UInteger
+-fpack-struct=<number>	Set initial maximum structure member alignment
+
 fpcc-struct-return
 Common Report Var(flag_pcc_struct_return,1) VarExists
 Return small aggregates in memory, not registers
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/opts.c	2004-07-02 15:13:53.000000000 +0200
+++ 2004-07-05.10.09/gcc/opts.c	2004-07-06 08:44:38.969509960 +0200
@@ -790,6 +790,13 @@
       pp_set_line_maximum_length (global_dc->printer, value);
       break;
 
+    case OPT_fpack_struct_:
+      if (value <= 0 || (value & (value - 1)) || value > 16)
+	error("structure alignment must be a small power of two, not %d", value);
+      else
+	maximum_field_alignment = initial_max_fld_align = value * BITS_PER_UNIT;
+      break;
+
     case OPT_fpeel_loops:
       flag_peel_loops_set = true;
       break;
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/stor-layout.c	2004-07-05 09:18:04.000000000 +0200
+++ 2004-07-05.10.09/gcc/stor-layout.c	2004-07-06 08:44:38.986507376 +0200
@@ -48,7 +48,12 @@
 
 /* If nonzero, this is an upper limit on alignment of structure fields.
    The value is measured in bits.  */
-unsigned int maximum_field_alignment;
+#ifndef DEFAULT_PACK_STRUCT
+#define DEFAULT_PACK_STRUCT 0
+#endif
+unsigned int maximum_field_alignment = DEFAULT_PACK_STRUCT * BITS_PER_UNIT;
+/* ... and the original value of it, specified via -fpack-struct=<value>. */
+unsigned int initial_max_fld_align = DEFAULT_PACK_STRUCT * BITS_PER_UNIT;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in bits.
    May be overridden by front-ends.  */
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-01-08 04:32:11.000000000 +0100
+++ 2004-07-05.10.09/gcc/testsuite/g++.dg/abi/vbase10.C	2004-05-27 10:57:09.000000000 +0200
@@ -1,5 +1,5 @@
 // { dg-do compile }
-// { dg-options "-Wabi -fabi-version=1" }
+// { dg-options "-Wabi -fabi-version=1 -fpack-struct=8" }
 // On ARM processors, the alignment of B will be 4 even though it
 // contains only a single "char".  That would avoids the situation
 // that the warning below is designed to catch.  We therefore
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2003-06-10 01:04:50.000000000 +0200
+++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/Wpadded.c	2004-05-26 15:38:24.000000000 +0200
@@ -1,7 +1,7 @@
 /* Source: EMC.  */
 
 /* { dg-do compile } */
-/* { dg-options "-Wpadded" } */
+/* { dg-options "-Wpadded -fpack-struct=8" } */
 
 struct foo {
   char bar;
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2002-08-17 16:48:28.000000000 +0200
+++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/c99-flex-array-4.c	2004-05-26 17:19:15.000000000 +0200
@@ -5,7 +5,7 @@
    from Tony Finch <dot@dotat.at>, adapted to a testcase by Joseph Myers
    <jsm28@cam.ac.uk>.  See also WG14 reflector messages 9571-3.  */
 /* { dg-do compile } */
-/* { dg-options "-std=iso9899:1999 -pedantic-errors" } */
+/* { dg-options "-std=iso9899:1999 -fpack-struct=8 -pedantic-errors" } */
 
 #include <stddef.h>
 
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/testsuite/gcc.dg/pack-test-2.c	2000-12-13 21:16:33.000000000 +0100
+++ 2004-07-05.10.09/gcc/testsuite/gcc.dg/pack-test-2.c	2004-05-18 16:17:53.000000000 +0200
@@ -3,9 +3,11 @@
 
 /* { dg-do compile { target *-*-linux* *-*-cygwin* powerpc*-*-eabi* } } */
 
-#pragma pack(push)              /* { dg-error "malformed" } */
 #pragma pack(pop)               /* { dg-error "without matching" } */
 
+#pragma pack(push)
+#pragma pack(pop)               /* reset */
+
 #pragma pack(push, foo, 1)
 #pragma pack(pop, foo, 1)       /* { dg-error "malformed" } (/
 #pragma pack(pop)               /* reset */
--- /home/jbeulich/src/gcc/mainline/2004-07-05.10.09/gcc/tree.h	2004-07-05 09:18:05.000000000 +0200
+++ 2004-07-05.10.09/gcc/tree.h	2004-07-06 08:44:39.045498408 +0200
@@ -3026,8 +3026,10 @@
    + (BITS_PER_UNIT > 8) + (BITS_PER_UNIT > 16) + (BITS_PER_UNIT > 32) \
    + (BITS_PER_UNIT > 64) + (BITS_PER_UNIT > 128) + (BITS_PER_UNIT > 256))
 
-/* If nonzero, an upper limit on alignment of structure fields, in bits.  */
+/* If nonzero, an upper limit on alignment of structure fields, in bits,  */
 extern unsigned int maximum_field_alignment;
+/* and the original value of it, specified via -fpack-struct=<value>. */
+extern unsigned int initial_max_fld_align;
 
 /* If nonzero, the alignment of a bitstring or (power-)set value, in bits.  */
 extern unsigned int set_alignment;