Re: #pragma push breaks bootstrap?

Re: #pragma push breaks bootstrap?

[Nick Clifton]

Hi Robert,


Just to clarify, there are two kinds of #pragma pack:

The SystemV kind which look like:

	#pragma pack(<n>)

and which sets structure packing to <n> bytes until overriden by
another occurance of #pragma pack (<n>) or an occurance of #pragma
pack() which turns off structure packing.

The Microsoft extensions to the syntax which look like:

	#pragma pack (push, <n>)

which does the same thing as #pragma pack(<n>), except that it can
later be undo by:

	#pragma pack (pop)

which resets the packing back to the previous level.
	

: > 	a. #pragma pack is being used by your system header files
: 
: Yes.
: 
: I can send you samples if you like, but they essentially look something
: like:
: 
: #ifndef _STDIO_H
: #define _STDIO_H
: 
: #pragma comment(exestr, "xpg4plus @(#) stdio.h 20.2 95/12/11 ")
: 
: #pragma pack(4)
: 
: #ifdef __cplusplus
: extern "C" {
: #endif
: 
: [ meat of header munched ] 
: 
: #pragma pack()
: 
: #endif /* _STDIO_H */
: #endif

Ahh, this is starting to make a lot more sense now.  My guess is that
the i386 toolchain does not define HANDLE_SYSV_PRAGMA, and so until
now all occurances of #pragma pack have been ignored!  (You can test
this for yourself by compiling with an older toolchain and with
--Wunknown-pragmas specified).

With the new code in it, defining HANDLE_PRAGMA_PACK_PUSH_POP
automatically enables HANDLE_PRAGMA_SYSV (well actually just the
handling of #pragma pack(<n>), since the Microsoft syntax is an
extension of the basic syntax.
 
: > and	b. If so, is there any documentation on how the pragma is
: > 	supposed to behave.
: 
: I thought it was a pretty typical implementation of pragma pack, but
: I'm surely biased by a decade of SCO experience. :-)

No, you are right.  I think that it is just that until now, the i386
toolchains have been ignoring all occurances of #pragma pack.


So, how to fix the situation ?

Here I am a bit stumped.  Your system header files specify that the
structures are packed, but when gcc starts using the packed structures
things explode.  Hmmm...

You know far about OpenServer than I do.  Does my explanation above
suggest a course of action for you ?

Cheers
	Nick

Re: #pragma push breaks bootstrap?

[J. Kean Johnston]

On Fri, Oct 02, 1998 at 03:23:36PM -0700, Nick Clifton wrote:
> Ahh, this is starting to make a lot more sense now.  My guess is that
> the i386 toolchain does not define HANDLE_SYSV_PRAGMA, and so until
> now all occurances of #pragma pack have been ignored!  (You can test
> this for yourself by compiling with an older toolchain and with
> --Wunknown-pragmas specified).
Wrong guess. From config/i386/sco5.h (the OpenServer header file):
sco5.h:#define HANDLE_SYSV_PRAGMA 1

Kean.

Re: #pragma push breaks bootstrap?

[Robert Lipe]

Hi, Nick.

> The problem is that insert_pack_attributes() is being called for
> #pragma pack, (as well as #pragma pack (push,...) which is a change
> from the previous behaviour.  

That sounds plausable.

> Could you try applying the patch below
> and see if it foxes the problem for you.  If it does, I will tidy it
> up and submit it to egcs-patches.

Yes, this patch does indeed allow a bootstrap to proceed past the first
use of a freshly generated file.   

Note careful use of weasel words to not say "it successfully bootstraps"
or "it passes the testsuite", but only to mean, "I believe this does
indeed fix the problems we can readily pin to new pragma handline. :-)

I think you nailed it.  Thanx for the help, Nick!

RJL


>
>
> Index: c-pragma.c
> ======================================================================
> RCS file: /egcs/carton/cvsfiles/egcs/gcc/c-pragma.c,v retrieving
> revision 1.9 diff -p -w -r1.9 c-pragma.c *** c-pragma.c 1998/10/01
> 10:50:06 1.9 --- c-pragma.c 1998/10/05 20:40:45 ***************
> insert_pack_attributes (node, attributes *** 146,152 **** tree a;
>
>     /* If we are not packing, then there is nothing to do. */ ! if
> (maximum_field_alignment == 0) return;
>
>     /* We are only interested in fields. */ --- 146,153 ---- tree a;
>
>     /* If we are not packing, then there is nothing to do. */ ! if
> (maximum_field_alignment == 0 ! || alignment_stack == NULL) return;
>
>     /* We are only interested in fields. */
>
>
> Cheers Nick

Re: #pragma push breaks bootstrap?

[Nick Clifton]

Hi Robert,

: Before we chase this a lot further, can I ask a silly question?  Since
: insert_pack_attributes() has a non-zero cost (breakpoints show it being
: called many times for even the simple example) and presumably provides
: benefits only to those that need the Windows-ism 'pragma push' should
: we move the setting of this out of i386.h and into the target-specific
: files for those targets that care about that extension? (winnt.h,
: go32.h, cygwin32, mingw, etc.)

Good point, I had not considered compilation times when writing the
patch.  Moving H_P_P_P into those OS specific header files would be a
good idea.

Actually it turns out that this was the clue that I needed.  The
problem is that insert_pack_attributes() is being called for #pragma
pack, (as well as #pragma pack (push,...) which is a change from the
previous behaviour.  Could you try applying the patch below and see if
it foxes the problem for you.  If it does, I will tidy it up and
submit it to egcs-patches.


Index: c-pragma.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/c-pragma.c,v
retrieving revision 1.9
diff -p -w -r1.9 c-pragma.c
*** c-pragma.c	1998/10/01 10:50:06	1.9
--- c-pragma.c	1998/10/05 20:40:45
*************** insert_pack_attributes (node, attributes
*** 146,152 ****
    tree a;
  
    /* If we are not packing, then there is nothing to do.  */
!   if (maximum_field_alignment == 0)
      return;
  
    /* We are only interested in fields.  */
--- 146,153 ----
    tree a;
  
    /* If we are not packing, then there is nothing to do.  */
!   if (maximum_field_alignment == 0
!       || alignment_stack == NULL)
      return;
  
    /* We are only interested in fields.  */


Cheers
	Nick

Re: #pragma push breaks bootstrap?

[Robert Lipe]

Hi, Nick.

Thanx for the quick reply.   Comments blow.

> : In this example, "stage1/" holds a "bad" compiler and "stage2/" holds
> : one that works.  As a sanity check, I show the results from the system
> : 
> : 
> : (robertl) rjlhome:/play/negcs/gcc
> : $ stage1/xgcc -Bstage1/ /tmp/p.c && ./a.out
> : 10 18
> : 18
> : (robertl) rjlhome:/play/negcs/gcc
> : $ stage2/xgcc -Bstage2/ /tmp/p.c && ./a.out
> : 10 10
> : 10
> : (robertl) rjlhome:/play/negcs/gcc
> : $ /bin/cc /tmp/p.c && ./a.out
> : 10 10
> : 10

[ Note to the reader, these numbers are in hex, not decimal ] 

> Is stage1/xgcc the one built when HANDLE_PRAGMA_PACK_PUSH_POP is
> enabled or disabled ?

Stage1/xgcc has it enabled.   It's the one that disagrees with the
native tools about the size of the structure in question.

> You see with the new code defining either (or both of)
> HANDLE_SYSV_PRAGMA or HANDLE_PRAGMA_PACK_PUSH_POP in a target header
> file will cause HANDLE_PRAGMA_PACK to be defined in c-pragma.h, which
> will enable the parsing of #pragma pack(<n>) in c-pragma.c.  So even

Yes, now that I have things locked in the debugger, that's exactly
what I'm seeing.   Watchpoints on maximum_field_alignment show it
toggling in the same way at the same time with or without H_P_P_P_P
so that doesn't seem to be the issue.

> if HANDLE_PRAGMA_PACK_PUSH_POP is *not* defined but HANDLE_SYSV_PRAGMA
> *is* defined then the (rearranged) code for handling #pragma pack(<n>)
> should be used.  And since gcc appears to work when H_P_P_P_P is not

Yes.  I can see the same code is indeed used for parsing both cases.  I
see that in the testcases, push_alignment is never called.   So I don't
think that it's falsely confusing the two variants.

> defined, it would appear that the code is working....  

For whatever reason that we haven't found yet, egcs with H_P_P_P_P
enabled and every other compiler I have ready access to on OpenServer
are disagreeing about the size of this structure.  (Four compilers by
three distinctly different vendors not counting other GCC derivitaves.)
The default alignment is documented as '4' on the ones from Intel, SCO,
and Microsoft.  I can't find a documented default for EGCS.

The difference seems to be that they disagree about how to handle the
following case:

#pragma pack(4)
struct blah { char a1; char a2; char a3};

Compiler balloting (yes, i understand the danger of that :-) says that
sizeof (struct blah) == 3.  Without H_P_P_P_P, egcs says that the
size is 3.  With H_P_P_P_P_P, egcs says the size is 12.  If we remove
the 'pragma pack' line (which doesn't make a difference on the others
becuase that's already the documented default) H_P_P_P_P_P agrees with
the others.

Unfortunately, I don't understand enough about trees to trace into
the guts of insert_pack_attributes() and debug it in any meaningful
way.

> [All of this assumes that HANDLE_SYSV_PRAGMA is defined in your
> toiolchain.  Could you just check this to make sure].

config/i386/sco5.h definitely sets HANDLE_SYSV_PRAGMA.   This has been
working fine in the OpenServer port until very recently.

Before we chase this a lot further, can I ask a silly question?  Since
insert_pack_attributes() has a non-zero cost (breakpoints show it being
called many times for even the simple example) and presumably provides
benefits only to those that need the Windows-ism 'pragma push' should
we move the setting of this out of i386.h and into the target-specific
files for those targets that care about that extension? (winnt.h,
go32.h, cygwin32, mingw, etc.)

Thanx for any guidance you can offer and the help so far.
RJL

Re: #pragma push breaks bootstrap?

[Robert Lipe]

Nick Clifton wrote:
> Hi Robert,
> 
> : I walked through the patches of the last 24 hours and one of the most
> : benign looking of the lot seems to be thing that triggers it.  If I
> : revert i386/i386.h to -D981001 so that HANDLE_PRAGMA_PACK_PUSH_POP isn't
> : set and redo the bootstrap, it works.  If I walk that lone file forward
> : to the -D981002 tag and redo the bootstrap, it does the wrong thing.  The
> : whole thing just reeks of a memory overwrite somewhere.
> 
> I agree, although I am stumped as to where the bug might lie.
> 
> I eyeballed my code, and I do not see how it could be affecting you.

I eyeballed your code, too.  In light of this message, it occurs to me
that I was looking for a forest instead of trees.  I was looking for 
malloc() bozos, off-by-ones and the like.   It didn't occur to me that
the new code might actually be _used_ :-)


> (I am assuming that none of your system header files actually use
> #pragma pack(..).  If they do, then maybe this might be the cause).

Only about 1300 times.

(robertl) rjlhome:/usr/include 
$ grep 'pragma.*pack' `find . -name \*.h -print` | wc -l
   1324

> : The difference between the two versions is in the .s files.   When
> : PRAGMA_PUSH and friends are turned on, egcs emits wrong offsets into
> : the __iob array that is used by things like the 'fflush(stdout)' that

> Hmmm, I take it back - it does look like your headers use #pragma
> pack push(...).  The effect of the pragma is to change the packing of

It honestly didn't occur to me that it was packing the iob structure.
This certainly explains things.

I see zero occurrences of 'pragma.*pack.*push' in system headers.

> fields inside structures.  The point of this patch was to support the
> way Microsoft's Visual C compiler implememnts the pragma, and so allow
> Cygwin and pals to use MS system header files.

Since some of the OpenServer headers have Microsoft heritage and SCO
used to use the Microsoft compilers, it is likely that these have common
genealogy.

> My guess is that <stdio.h> or one of its descendants is defining
> #pragma pack, and that either a) the HANDLE_PRAGMA_PACK_PUSH_POP code
> is not handling the pragma correctly, or b) there is difference in the
> behaviour of #pragma pack between OpenServer and Win32.

Yes, this is starting to make much more sense now.

> 	a. #pragma pack is being used by your system header files

Yes.

I can send you samples if you like, but they essentially look something
like:

#ifndef _STDIO_H
#define _STDIO_H

#pragma comment(exestr, "xpg4plus @(#) stdio.h 20.2 95/12/11 ")

#pragma pack(4)

#ifdef __cplusplus
extern "C" {
#endif

[ meat of header munched ] 

#pragma pack()

#endif /* _STDIO_H */
#endif



> and	b. If so, is there any documentation on how the pragma is
> 	supposed to behave.

I thought it was a pretty typical implementation of pragma pack, but
I'm surely biased by a decade of SCO experience. :-)

This document contains a pretty decent defintion of how these tools 
work.

	http://www.sco.com/cgi-bin/ssl_reference?100379

Without an actual examaination of c-pragma.c, I'm wonder if it's
matching 'pragma pack('s that don't have the 'push' or 'pop' keywords.


Thanx for the help.
RJL

Re: #pragma push breaks bootstrap?

[Nick Clifton]

Hi Robert,

: So this certainly seems to explain things.   Note that this target is one
: of the relatively few that defines  'HANDLE_SYSV_PRAGMA' so this means that
: HANDLE_PRAGMA_PACK will be turned on by c-pragma.h.

Ohh, I was assuming that the target did not define HANDLE_SYSV_PRAGMA.

: Should the two forms of PRAGMA_PACK play nicely with each other?

Yes they should.  (That was certainly my intention).

In your example:

: In this example, "stage1/" holds a "bad" compiler and "stage2/" holds
: one that works.  As a sanity check, I show the results from the system
: compiler, too.
: 
: 
: (robertl) rjlhome:/play/negcs/gcc
: $ stage1/xgcc -Bstage1/ /tmp/p.c && ./a.out
: 10 18
: 18
: (robertl) rjlhome:/play/negcs/gcc
: $ stage2/xgcc -Bstage2/ /tmp/p.c && ./a.out
: 10 10
: 10
: (robertl) rjlhome:/play/negcs/gcc
: $ /bin/cc /tmp/p.c && ./a.out
: 10 10
: 10

Is stage1/xgcc the one built when HANDLE_PRAGMA_PACK_PUSH_POP is
enabled or disabled ?

You see with the new code defining either (or both of)
HANDLE_SYSV_PRAGMA or HANDLE_PRAGMA_PACK_PUSH_POP in a target header
file will cause HANDLE_PRAGMA_PACK to be defined in c-pragma.h, which
will enable the parsing of #pragma pack(<n>) in c-pragma.c.  So even
if HANDLE_PRAGMA_PACK_PUSH_POP is *not* defined but HANDLE_SYSV_PRAGMA
*is* defined then the (rearranged) code for handling #pragma pack(<n>)
should be used.  And since gcc appears to work when H_P_P_P_P is not
defined, it would appear that the code is working....  [All of this
assumes that HANDLE_SYSV_PRAGMA is defined in your toiolchain.  Could
you just check this to make sure].

Cheers
	Nick

#pragma push breaks bootstrap?

[Robert Lipe]

This is the kind of problem that makes one down his own sanity.  Before
this message is over, I fully expect at least a few of you to question
my sanity, too.

In the last 24 hours, EGCS has quit bootstrapping on OpenServer.
Jean-Pierre was the one that reported this to me.  I tracked it down to
to a generated file that was being misgenerated and I've even tracked
it down to one source line I can change to make the problem go away.
But this is making less and less sense as I go along.  Yes, I'm running
exactly what's in the repository.  I've reverted the i386.md stuff from
yesterday.   Certainly J-P wasn't running anything that experimental and
he saw this before I did.


Basically, gencodes.o is being miscompiled by the stage1 compiler.
It's trying to do a negative lseek on stdout.  This works OK if you
just type 'gencodes ..blah../i386.md' you can see the outpu just fine.
(Seeks don't work on tty devices.)  When you direct it to a file (as
the build procedure does) this drives the libraries crazy and you don't
get the end of the file redirected into insn-codes.h.  Poking around in
gencodes.c, I found I could "fix" the problem by adding almost any code
to it that touched low numbered file descriptors.

I walked through the patches of the last 24 hours and one of the most
benign looking of the lot seems to be thing that triggers it.  If I
revert i386/i386.h to -D981001 so that HANDLE_PRAGMA_PACK_PUSH_POP isn't
set and redo the bootstrap, it works.  If I walk that lone file forward
to the -D981002 tag and redo the bootstrap, it does the wrong thing.  The
whole thing just reeks of a memory overwrite somewhere.

For "ease" of testing, I've populated stage1/ with a gcc built with
the bad version of i386.h and stage2/ with a gcc built with the good
version.  If I use each with --save-temps to build gencodes.o, the
resulting preprocessor files are identical.  

(robertl) rjlhome:/play/negcs/gcc
$ diff gencodes.1.i gencodes.2.i
(robertl) rjlhome:/play/negcs/gcc
$ sum gencodes.1.i gencodes.2.i
6639 143 gencodes.1.i
6639 143 gencodes.2.i


The difference between the two versions is in the .s files.   When
PRAGMA_PUSH and friends are turned on, egcs emits wrong offsets into
the __iob array that is used by things like the 'fflush(stdout)' that
appears in the gencodes source.

$ diff gencodes.1.s gencodes.2.s
125c125
<       pushl $__iob+48
---
>       pushl $__iob+32
132c132
<       pushl $__iob+48
---
>       pushl $__iob+32
136c136
<       pushl $__iob+48
---
>       pushl $__iob+32
306c306
<       pushl $__iob+24
---
>       pushl $__iob+16
309c309
<       movb __iob+36,%al
---
>       movb __iob+28,%al


Of these differences, most are in fatal().   The one that matters
is the one that's actually called:

$ sdiff -w80 gencodes.1.s gencodes.2.s
        .file   "gencodes.c"                      .file   "gencodes.c"
        .version        "01.01"                   .version        "01.01"
gcc2_compiled.:                           gcc2_compiled.:

[ munch ] 

        incl insn_code_number                     incl insn_code_number
.L18:                                     .L18:
        jmp .L12                                  jmp .L12
        .align 4                                  .align 4
.L13:                                     .L13:
        pushl $.LC10                              pushl $.LC10
        call printf                               call printf
        addl $4,%esp                              addl $4,%esp
        pushl $.LC11                              pushl $.LC11
        call printf                               call printf
        addl $4,%esp                              addl $4,%esp
        pushl $.LC12                              pushl $.LC12
        call printf                               call printf
        addl $4,%esp                              addl $4,%esp
        pushl $__iob+24                |          pushl $__iob+16
        call fflush                               call fflush
        addl $4,%esp                              addl $4,%esp
        movb __iob+36,%al              |          movb __iob+28,%al
        andb $32,%al                              andb $32,%al
        testb %al,%al                             testb %al,%al
        je .L20                                   je .L20
        movl $33,%eax                             movl $33,%eax
        jmp .L21                                  jmp .L21
        .align 4                                  .align 4


All this results in the code doing an fflush() of the wrong file descriptor.


How can I help track this down?   

RJL

Re: #pragma push breaks bootstrap?

[Robert Lipe]

> > Hmmm, I take it back - it does look like your headers use #pragma
> > pack push(...).  The effect of the pragma is to change the packing of
> 
> It honestly didn't occur to me that it was packing the iob structure.
> This certainly explains things.

I think you're spot-on, Nick.

$ cat /tmp/p.c
#define BLAH \
int  x; \
unsigned char *_x1;\
unsigned char *_x2;\
unsigned char __x3;\
unsigned char __x4;\
unsigned char __x[2];


struct one { BLAH };
#pragma pack(4)
struct two { BLAH };
#pragma pack()

#include <stdio.h>
main()
{
        printf("%x %x\n", sizeof(struct one), sizeof(struct two));
        printf("%x\n", sizeof(__iob[0]));
}

In this example, "stage1/" holds a "bad" compiler and "stage2/" holds
one that works.  As a sanity check, I show the results from the system
compiler, too.


(robertl) rjlhome:/play/negcs/gcc
$ stage1/xgcc -Bstage1/ /tmp/p.c && ./a.out
10 18
18
(robertl) rjlhome:/play/negcs/gcc
$ stage2/xgcc -Bstage2/ /tmp/p.c && ./a.out
10 10
10
(robertl) rjlhome:/play/negcs/gcc
$ /bin/cc /tmp/p.c && ./a.out
10 10
10


So this certainly seems to explain things.   Note that this target is one
of the relatively few that defines  'HANDLE_SYSV_PRAGMA' so this means that
HANDLE_PRAGMA_PACK will be turned on by c-pragma.h.

Should the two forms of PRAGMA_PACK play nicely with each other?

RJL

Re: #pragma push breaks bootstrap?

[Nick Clifton]

Hi Robert,

: I walked through the patches of the last 24 hours and one of the most
: benign looking of the lot seems to be thing that triggers it.  If I
: revert i386/i386.h to -D981001 so that HANDLE_PRAGMA_PACK_PUSH_POP isn't
: set and redo the bootstrap, it works.  If I walk that lone file forward
: to the -D981002 tag and redo the bootstrap, it does the wrong thing.  The
: whole thing just reeks of a memory overwrite somewhere.

I agree, although I am stumped as to where the bug might lie.

I eyeballed my code, and I do not see how it could be affecting you.
(I am assuming that none of your system header files actually use
#pragma pack(..).  If they do, then maybe this might be the cause).

: The difference between the two versions is in the .s files.   When
: PRAGMA_PUSH and friends are turned on, egcs emits wrong offsets into
: the __iob array that is used by things like the 'fflush(stdout)' that
: appears in the gencodes source.
: 
: $ diff gencodes.1.s gencodes.2.s
: 125c125
: <       pushl $__iob+48
: ---
: >       pushl $__iob+32

Hmmm, I take it back - it does look like your headers use #pragma
pack push(...).  The effect of the pragma is to change the packing of
fields inside structures.  The point of this patch was to support the
way Microsoft's Visual C compiler implememnts the pragma, and so allow
Cygwin and pals to use MS system header files.

My guess is that <stdio.h> or one of its descendants is defining
#pragma pack, and that either a) the HANDLE_PRAGMA_PACK_PUSH_POP code
is not handling the pragma correctly, or b) there is difference in the
behaviour of #pragma pack between OpenServer and Win32.

: How can I help track this down?   

Can you find out if:

	a. #pragma pack is being used by your system header files
and	b. If so, is there any documentation on how the pragma is
	supposed to behave.

Cheers
	Nick