This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: MachTen Egcs Patches



  In message <199807021827.LAA09959@mail.tenon.com>you write:
  > Hello,
  > 
  > We have wanted to get our gcc patches into the standard release for a
  > long time.  We were hoping that with the forthcoming freeze of egcs 1.1
  > that we might have chance to be included.
First note, I do not see a copyright assignment on file for you; you
will need to fill one out and probably an employer disclaimer before
we can do anything with these patches.  See:

http://www.cygnus.com/egcs/contribute.html.

Also note, this applies to any other folks that have contributed to
this code.

This is a rough first pass over the code.  I'm sure as we iterate on
the changes we'll notice more things that need to be addressed.


  > environment.  Since this system must reside equally well in real memory 
  > and virtual memory environments we have had to make modifications to the
  > alloca, __builtin_alloca and __builtin_va_alist parts of gcc.  
Can you please explain this?  Those functions really don't care about
whether your're running with or without virtual memory.  The va_alist
change in particular sounds totally bogus.


  > gcc/config/m68k/machten68k.h
  > 
  > /* Definitions of target machine for GNU compiler.  Macintosh 68000 version
  > .
  >    Copyright (C) 1987, 1988 Free Software Foundation, Inc.
You'll need to add 1998 to the copyright list -- similarly for the
other files you added.


  > /* Use MachTen include file search path.  */
  > /* Even for a cross compiler with MachTen as target! */
  > #define INCLUDE_DEFAULTS				\
  >   {							\
  >     { GPLUSPLUS_INCLUDE_DIR, 1, 1},			\
  >     { MACHINE_INCLUDE_DIR, 0, 1},			\
  >     { LOCAL_INCLUDE_DIR, 0, 1},				\
  >     { STANDARD_INCLUDE_DIR, 0, 1},			\
  >     { 0, 0, 0}						\
  >   }
This sounds wrong.  You shouldn't be modifying the INCLUDE_DEFAULTS
when building a cross compiler on MachTen, only when building a
MachTen target compiler.

  > /* Define __HAVE_68881 in preprocessor only if -m68881 is specified.
  >    This will control the use of inline 68881 insns in certain macros.
  >    Also inform the program which CPU this is for.  */
  > 
  > #ifndef CPP_SPEC
  > #define CPP_SPEC "\
  > %{!m68040-lc:%{!mc68040-lc:%{m680*: -Dmc680%*}%{mc680*: -Dmc680%*}}}\
  > %{m68040-lc: -Dmc68LC040}%{mc68040-lc: -Dmc68LC040}\
  > %{!m680*:%{!mc680*: -Dmc68020}}\
  > %{m68881:%{!msoft-float: -D__HAVE_68881__}}"
  > #endif
This may need updating -- some of this stuff has changed relatively
recently.


  > #ifndef ASM_SPEC
  > #define ASM_SPEC "\
  > %{!m68040-lc:%{!mc68040-lc:%{m680*: -mc680%*}%{mc680*: -mc680%*}}}\
  > %{m68040-lc: -mc68040}%{mc68040-lc: -mc68040}\
  > %{!m680*:%{!mc680*: -mc68020}} %{mno-68*} %{pic}"
  > #endif
Similarly.

  > /* Names to predefine in the preprocessor for this target machine.  */
  > /* __MACHTEN__ is needed inside cpp for direct calls to it; gcc will
  >    compose it from -DMACHTEN; TENON is a silent holdover.  */
  > /* TENON is *deprecated*, use __MACHTEN__ instead */
  > #ifndef CPP_PREDEFINES
  > #define CPP_PREDEFINES "-Dunix -D__MACHTEN__ -D__MACHTEN_68K__ -DTENON\
  >  -Asystem(unix) -Asystem(bsd) -Asystem(machten)\
  >  -Acpu(m68000) -Acpu(mc68000) -Acpu(m68k)\
  >  -Amachine(m68000) -Amachine(mc68000) -Amachine(m68k) -Amachine(mac)"
  > #endif
If at all possible, we should avoid -Dunix and -DTENON since they
pollute the ansi namespace.

  > #ifndef	CC1_SPEC
  > #define CC1_SPEC "-fno-builtin-alloca -fno-defer-pop"
  > #endif
Why are you turning off alloca & defer pop?  That seems wrong.

  > #ifndef	CC1PLUS_SPEC
  > #define CC1PLUS_SPEC "-fno-builtin-alloca -fno-defer-pop -felide-constructo rs"
  > #endif
Similarly.

  > 
  > #ifndef LINK_SPEC
  > #define LINK_SPEC "-x"
  > #endif
What does this do?

  > # Strip installed executables.. please!
  > INSTALL_PROGRAM = $(INSTALL) -s
This is wrong.  It should be driven by users, not by compiler writers.

  > # Common prefix for installation directories.
  > prefix = /usr
Ugh.  This is wrong too.  prefix is a user option and if the user
does not specify --prefix, then it should default to /usr/local.

  > # One might argue that GCC_O_CPPFLAGS and CCCP_O_CPPFLAGS belong in
  > # x-machten68k, however, their presence here is convenient for making both
  > # a native m68k compiler and a cross compiler running on PowerMacs.
  > GCC_O_CPPFLAGS=\
  > 	-DSTANDARD_STARTFILE_PREFIX=\"/usr/mac68k/lib/\" \
  > 	-DSTANDARD_EXEC_PREFIX=\"/usr/mac68k/bin/\" \
  > 	-DDEFAULT_TARGET_VERSION=\"$(version)\" \
  > 	-DDEFAULT_TARGET_MACHINE=\"$(target)\" \
  > 	-DTOOLDIR_BASE_PREFIX=\"/usr/local/\"
  > 
  > CCCP_O_CPPFLAGS=\
  > 	-DGCC_INCLUDE_DIR=\"\" \
  > 	-DGPLUSPLUS_INCLUDE_DIR=\"/usr/include/g++\" \
  > 	-DLOCAL_INCLUDE_DIR=\"/usr/local/include\" \
  > 	-DCROSS_INCLUDE_DIR=\"\" \
  > 	-DTOOL_INCLUDE_DIR=\"\" \
  > 	-DMACHINE_INCLUDE_DIR=\"/usr/mac68k/include\"
See the comment above this code.  this is wrong.

  > 
  > # Even tho gcc is the compiler on MachTen, refer to it by the historical na
  > me
  > # so that USE_ALLOCA and USE_HOST_ALLOCA are properly set (as alloca.o)!
  > CC = cc
Remove this.  It is not needed or wanted anymore.

  > ALL= native xgcc specs lang.start.encap lang.rest.encap
  > LANGUAGES = c c++
  > STMP_FIXPROTO=
Kill ALL & LANGUAGES.  These should not be set in an x-* file.

  > X_CFLAGS= -fno-builtin-alloca -fno-defer-pop
  > 
  > # Extra compile/load switches specific to MachTen
  > ALLOCA= alloca.o
  > ALLOCA_FLAGS= -DSTACK_DIRECTION=-1
  > 
  > XGCC_LDFLAGS= -stack 32768 ${LDFLAGS}
  > CCCP_LDFLAGS= -stack 49152 ${LDFLAGS}
  > CC1_LDFLAGS= -stack 49152 ${LDFLAGS}
  > CC1OBJ_LDFLAGS= -stack 49152 ${LDFLAGS}
  > CC1PLUS_LDFLAGS= -stack 49152 ${LDFLAGS}
  > F771_LDFLAGS= -stack 49152 ${LDFLAGS}
Is there a better way to do this?

  > /* Note presence of selected function in host library */
  > #define HAVE_PUTENV		/* putenv() */
  > #define HAVE_STRERROR		/* strerror() */
  > #define HAVE_VPRINTF		/* vprintf() */
Most of this stuff should not be necessary anymore due to autoconf.


I worry about the alloca code you added to the rs6000 directory.  I think
you need to explain the need to override alloca in more detail.


  > /* No structure field wants to be aligned rounder than this.  */
  > #undef BIGGEST_FIELD_ALIGNMENT
  > //
  > // wanted to make this the default.  simply to many things started
  > // breaking - therefore had to return to default
  > #define   BIGGEST_FIELD_ALIGNMENT 16
Do not put C++ style comments in gcc code.  Use C comments instead.


Comments should be complete sentences ending with a period followed by
two spaces.  Many of them need updating to this convention.


  > USER_H =
  > INSTALL_ASSERT_H =
This is wrong.  It will cause problems with C++.  You should find a way
to work with the header files gcc installs.

  > CFLAGS = -malignppc -O3 -pipe -fstrength-reduce -fomit-frame-pointer -finli
  > ne-functions -finhibit-size-directive -DREAL_MEMORY_TARGET
  > CXXFLAGS = -malignppc -O3 -pipe -fstrength-reduce -fomit-frame-pointer -fin
  > line-functions -finhibit-size-directive -DREAL_MEMORY_TARGET
Remove this -- you shouldn't be passing most/all of these flags when
building the compiler.

  > # Common prefix for installation directories.
  > prefix = /usr
See earlier comments abbout prefix.

  > # One might argue that GCC_O_CPPFLAGS and CCCP_O_CPPFLAGS belong in
  > # x-machtenppc, however, their presence here is convenient for making both
  > # a native PPC compiler and a cross compiler running on m68k-Macs.
  > 
  > 
  > GCC_O_CPPFLAGS =\
  > 	-DSTANDARD_STARTFILE_PREFIX=\"$(prefix)/lib/gcc-lib/\"\
  > 	-DSTANDARD_EXEC_PREFIX=\"$(prefix)/lib/gcc-lib/\"\
  > 	-DDEFAULT_TARGET_VERSION=\"$(version)\"\
  > 	-DDEFAULT_TARGET_MACHINE=\"$(target)\"\
  > 	-DTOOLDIR_BASE_PREFIX=\"/usr/lib/gcc-lib/\"
  > 
  > CCCP_O_CPPFLAGS =\
  > 	-DGCC_INCLUDE_DIR=\"$(libsubdir)/include\"\
  > 	-DGPLUSPLUS_INCLUDE_DIR=\"/usr/include/g++\"\
  > 	-DLOCAL_INCLUDE_DIR=\"/usr/local/include\"\
  > 	-DCROSS_INCLUDE_DIR=\"$(libsubdir)/sys-include\"\
  > 	-DTOOL_INCLUDE_DIR=\"$(tooldir)/include\"\
  > 	-DMACHINE_INCLUDE_DIR=\"/usr/macppc/include\"
Same comment as for the m68k files.


  > # Directory in which the compiler finds g++ includes.
  > gxx_include_dir = /usr/include/g++
You shouldn't be setting this variable either.

  > ALL= native xgcc specs lang.start.encap lang.rest.encap
  > LANGUAGES = c
  > STMP_FIXPROTO=
Setting ALL and LANGUAGES is wrong.


  > /* It would be better to get this (actually BSD4_4) from <sys/param.h>,
  >    however, this is all we really need.  */
  > #define bsd4_4	1
This should not be necessary anymore.

  > /* Note presence of selected function in host library */
  > //#define HAVE_PUTENV		/* putenv() */
  > //#define HAVE_STRERROR		/* strerror() */
  > //#define HAVE_VPRINTF		/* vprintf() */
Delete all this stuff.

  > + #ifdef REAL_MEMORY_TARGET
All these ifdefs need to be rethought or explained better -- why can't
you use the builtin alloca stuff?

  > + #ifdef __MACHTEN__
  > +       else if( type == ps_align ) {
  > +           if( state == ps_right )
  > +             maximum_field_alignment = align * 8;
  > +           else
  > +             warning( "malformed `#pragma align`" );
  > +       } else if( type == ps_options ) {
  > +           /* a great deal of the source already supports this form of pra
  > gma */
  > +           /* simply cheaper to modify the compiler than to fix everything
  >  */
  > +           if( state == ps_value ) {
  > +                 if( strcmp( value,"native" ) == 0  ||
  > +                     strcmp( value,"power" ) == 0 )
  > +                         maximum_field_alignment = 32;
  > +                 else
  > +                 if( strcmp( value,"mac68k" ) == 0  ||
  > +                     strcmp( value,"m68k" ) == 0 )
  > +                         maximum_field_alignment = 16;
  > +                 else
  > +                 if( strcmp( value,"reset" ) == 0 )
  > +                         maximum_field_alignment = TARGET_ALIGN_M68K ? 16 
  > : 32;
  > +                 else
  > +                         maximum_field_alignment = 32;
  > +           } else
  > +             warning( "malformed `#pragma options align = <reset|mac68k|na
  > tive|power>`" );
  > +       }
  > + #endif
ifdefs on __MACHTEN__ are not appropriate in the generic parts of the
compiler.  You need to find a better (more generic) way to solve these
problems.

Also note you have many formatting problems.  I would recommend you
read the GNU coding standards and update your code to conform.


  > +                # On MachTen, the headers are already ok.
  > +                fixincludes=Makefile.in
Don't be so sure :-)  We recommend running fixincludes even if you
think your headers are OK.



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]