[build] Move ENABLE_EXECUTE_STACK to toplevel libgcc

[Paolo Bonzini]

On 06/03/2011 05:45 PM, Rainer Orth wrote:
> Paolo Bonzini<bonzini@gnu.org>  writes:
>
>>> >>  Seems like a plan, but I didn't go in this direction since I couldn't
>>> >>  test anything like this.
>> >
>> >  As long as you test the general configury on 1-2 platforms, it's not any
>> >  less tested than what you have now.
> Unfortunately, that's not true: my original patch just moved the
> existing ENABLE_EXECUTE_STACK definitions to new headers in libgcc and
> used them as is.  The revised one below consolidates them into two new
> files, only one of them has been tested so far.

Yes, I wasn't expecting to go to this length.  But thanks for doing it.

>>> >>  One other caveat: I don't know if I like grouping the configure support
>>> >>  for the enable-execute-stack.c variants together or would rather keep
>>> >>  all configuration for a single platform (or OS) together.  Probably a
>>> >>  matter of taste.
>> >
>> >  In case it wasn't clear, I was proposing the latter.
> I suppose you mean the former, i.e. grouping them together and not
> setting enable_execute_stack in the existing host cases?

I meant doing it in config.host, because I was expecting you to keep all 
implementations in separate *.c files, even for all the mprotect 
variants, but I agree your patch is better.

> Will do when I submit the final patch.  Right now, I've got a plan and a
> draft where I'm looking for comments.

Makes sense.

> * Except for Darwin, the code uses TRAMPOLINE_SIZE.  This only exists in
>    the backend headers.  While it could be duplicated somewhere in the
>    libgcc configury, I'd rather propose that gcc define
>    __TRAMPOLINE_SIZE__ (in gcc/c-family/c-cppbuiltin.c (c_cpp_builtins)
>    to avoid this.  On PowerPC Darwin, one cannot use TRAMPOLINE_SIZE
>    definition right now since the macro calls the rs6000_trampoline_size
>    function in rs6000/rs6000.c.  This would be solved nicely by
>    __TRAMPOLINE_SIZE__.

Good idea.

> * FreeBSD and Solaris use a check_enabling function to check if
>    __enable_execute_stack needs to run at all.  Initially, I had
>    enable-execute-stack-{freebsd, netbsd, sol2}.c, all including a common
>    enable-execute-stack-mprotect.c to avoid code duplication.  I now
>    consider that ugly and hard to read, and merged everything into a
>    common enable-execute-stack-mprotect.c below.  Right now, the file
>    uses __FreeBSD__, __NetBSD__, and __sun__&&  __svr4__ to select the
>    right code, but this could be autoconf'ed if desired.  There are a
>    couple of FIXME comments in the code.

Please avoid putting the ((constructor)) in common code.  You can put

static int i = 1;

in the #else case, and leave

static int i;

in common code.

> * NetBSD is extremely careful to avoid namespace pollution and uses
>    private __sysctl (declared privately) to avoid using getpagesize.
>    Either this is an issue, and we might do something similar on all
>    platforms, or it's not and we should avoid special-casing NetBSD
>    here.

Perhaps it has to do with getpagesize not being available for old 
versions of NetBSD.  No idea, but I'd leave it as is.

> * FreeBSD uses the unmodified address passed to __enable_execute_stack
>    to call mprocted, while all others round both address and size to a
>    pagesize boundary.  I cannot imagine that FreeBSD supports
>    byte-granularity mprotect, so this seems an oversight.

Agreed.

> * Windows32 calls abort when VirtualProtect fails.  With the exception
>    of Darwin and NetBSD, all others call perror, which seems to be
>    frowned upon.  We should be consistent here.

I think it should either abort() or do nothing, consistently across all 
platforms.

> * gcc/config/i386/mingw32.h has
>
> #ifdef IN_LIBGCC2
> #include<windows.h>
> #endif
>
>    I strongly suspect that this is only to declare VirtualQuery and
>    VirtualProtect and can go if ENABLE_EXECUTE_STACK is removed?

Yes, it should go.

> * Finally, __enable_execute_stack is only used on those NetBSD targets
> that actually define HAVE_ENABLE_EXECUTE_STACK, while OpenBSD uses it
> everywhere.  config.host could be simplyfied if NetBSD behaved the
> same.

I would add it to all NetBSDs, and perhaps all FreeBSDs too?  Do you 
know why SPARC is special?

Paolo