Re: [PATCH] Let gcc bootstrap again on OpenBSD

[Zack Weinberg <zack at codesourcery dot com>]

Marc Espie <espie@nerim.net> writes:

> Taking care of your comment, thanks Neil !
>
> 4.0 does indeed take care of LP64 and ELF by itself.
>
> Full bootstrap on 4.0-20040102 (no java yet).  Ada doesn't bootstrap on
> 4.0-20040109 (unrelated).
>
> May I commit  this one ?

I'll approve this for mainline provided that you correct the
copyright-boilerplate issues pointed out by Nathanael.  (I think it
would be most appropriate to write "1999, 2005" for copyright year.)

Having said that, there are some ways this patch could be made
better.  You may either make my suggested changes before committing
this patch, or commit it as is and then do a second patch (which is
preapproved) to implement the suggestions.

Also I'd like to point out that overriding INCLUDE_DEFAULTS and
STANDARD_STARTFILE_PREFIX (and any other macro whose normal definition
is affected by configure options) is usually wrong.  You seem to be
taking steps back toward the normal definitions with this; could you
talk a little about the reasons why the normal definitions don't work
for you?  (This concern does not affect my approval of your patches.)

We don't have an official maintainer for OpenBSD targets right now.
Would you be interested in taking that role?

>   /* Under OpenBSD, the normal location of the various *crt*.o files is the
>      /usr/lib directory.  */
> ! #undef STANDARD_STARTFILE_PREFIX
> ! #define STANDARD_STARTFILE_PREFIX	"/usr/local/lib/"

Comment does not match code; please fix whichever is wrong.  

> +       builtin_assert ("system=unix");		\
> +       builtin_assert ("system=bsd");		\
> +       builtin_assert ("system=OpenBSD");	\

Preprocessor assertions are deprecated.  Please consider removing your
builtin assertions.

> + /* Stack is explicitly denied execution rights on OpenBSD platforms.  */
> + #define ENABLE_EXECUTE_STACK						\
> + extern void __enable_execute_stack (void *);				\
> + void									\
> + __enable_execute_stack (void *addr)					\
> + {									\
> +   long size = getpagesize ();						\
> +   long mask = ~(size-1);						\
> +   char *page = (char *) (((long) addr) & mask); 			\
> +   char *end  = (char *) ((((long) (addr + TRAMPOLINE_SIZE)) & mask) + size); \
> + 								      \
> +   if (mprotect (page, end - page, PROT_READ | PROT_WRITE | PROT_EXEC) < 0) \
> +     perror ("mprotect of trampoline code");				\
> + }
> + 
> + #include <sys/types.h>
> + #include <sys/mman.h>

Including target-system headers in this file will cause trouble if
anyone ever tries to build a cross-compiler to OpenBSD from a system
that doesn't have sys/mman.h or sys/types.h.  It will also cause
trouble when building libgcc before a complete C library exists (yes,
people have legitimate reasons to do that).  Unfortunately there is no
good solution to this problem.  Best current practice (e.g. sol2.h,
netbsd.h) is to write your own prototypes for getpagesize and
mprotect, inside the definition of ENABLE_EXECUTE_STACK, and then use
integer literals for all constants that would normally come from
system headers, with comments indicating which symbolic constants they
correspond to.

In 4.1 hopefully we will get libgcc split out from gcc, which will
help.

> + /* The following macros were originally stolen from i386v4.h.
> +    These have to be defined to get PIC code correct.  */
> + 
> + /* Assembler format: dispatch tables.  */
> + 
> + /* Assembler format: sections.  */
> + 
> + /* Stack & calling: aggregate returns.  */
...
> + /* Assembler format: alignment output.  */
> + 
> + /* Note that we pick up ASM_OUTPUT_MAX_SKIP_ALIGN from i386/gas.h */
> + 
> + /* Note that we pick up ASM_OUTPUT_MI_THUNK from unix.h.  */

Please remove these vacuous comments.

zw