Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Joseph S. Myers jos...@codesourcery.com writes: On Tue, 7 Jun 2011, Rainer Orth wrote: So my view is that you should define __LIBGCC_TRAMPOLINE_SIZE__, only if -fbuilding-libgcc. I can give it a try if I can figure out how to define -fbuilding-libgcc via the option handling machinery. I just want to avoid having to That should be no more than ; Define extra predefined macros for use in libgcc. fbuilding-libgcc C ObjC C++ ObjC++ Undocumented Var(flag_building_libgcc) in c.opt. Indeed, that did the trick. The following revised patch has been bootstrapped without regressions on i386-pc-solaris2.10. I've already got build, Darwin and Mingw approval and can commit the osf and solaris parts on my own. If the C parts are ok now, I suppose I still need approval either from the various *BSD maintainers (who haven't chimed in yet) of from a global reviewer? Thanks. Rainer 2011-05-29 Rainer Orth r...@cebitec.uni-bielefeld.de Joseph Myers jos...@codesourcery.com gcc: * config/alpha/netbsd.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/alpha/osf5.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/darwin.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/i386/mingw32.h (MINGW_ENABLE_EXECUTE_STACK): Remove. (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. [IN_LIBGCC2]: Don't include windows.h. * config/i386/netbsd-elf.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/i386/netbsd64.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/netbsd.h (NETBSD_ENABLE_EXECUTE_STACK): Remove. * config/openbsd.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/sol2.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/sparc/freebsd.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/sparc/netbsd-elf.h (ENABLE_EXECUTE_STACK): Remove. (HAVE_ENABLE_EXECUTE_STACK): Define. * config/alpha/alpha.c (alpha_trampoline_init): Test HAVE_ENABLE_EXECUTE_STACK. * config/i386/i386.c (ix86_trampoline_init): Likewise. * config/sparc/sparc.c (sparc32_initialize_trampoline): Likewise. (sparc64_initialize_trampoline): Likewise. * libgcc2.c [L_enable_execute_stack]: Remove. * system.h (ENABLE_EXECUTE_STACK): Poison. * doc/tm.texi.in (Trampolines, ENABLE_EXECUTE_STACK): Remove. * doc/tm.texi: Regenerate. * Makefile.in (LIBGCC2_CFLAGS): Add -fbuilding-libgcc. gcc/c-family: * c.opt (fbuilding-libgcc): New option. * c-cppbuiltin.c (c_cpp_builtins): Define __LIBGCC_TRAMPOLINE_SIZE__ if flag_building_libgcc. libgcc: * enable-execute-stack-empty.c: New file. * enable-execute-stack-mprotect.c: New file. * config/i386/enable-execute-stack-mingw32.c: New file. * config.host (enable_execute_stack): New variable. Select appropriate variants. * configure.ac: Link enable-execute-stack.c to $enable_execute_stack. * configure: Regenerate. * Makefile.in (LIB2ADD): Add enable-execute-stack.c. (lib2funcs): Remove _enable_execute_stack. diff --git a/gcc/Makefile.in b/gcc/Makefile.in --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -722,7 +722,7 @@ LIBGCC2_DEBUG_CFLAGS = -g LIBGCC2_CFLAGS = -O2 $(LIBGCC2_INCLUDES) $(GCC_CFLAGS) $(TARGET_LIBGCC2_CFLAGS) \ $(LIBGCC2_DEBUG_CFLAGS) $(GTHREAD_FLAGS) \ -DIN_LIBGCC2 -D__GCC_FLOAT_NOT_NEEDED \ --fno-stack-protector \ +-fbuilding-libgcc -fno-stack-protector \ $(INHIBIT_LIBC_CFLAGS) # Additional options to use when compiling libgcc2.a. diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c --- a/gcc/c-family/c-cppbuiltin.c +++ b/gcc/c-family/c-cppbuiltin.c @@ -1,5 +1,5 @@ /* Define builtin-in macros for the C family front ends. - Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010 + Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Free Software Foundation, Inc. This file is part of GCC. @@ -727,6 +727,12 @@ c_cpp_builtins (cpp_reader *pfile) builtin_define_fixed_point_constants (UTA, , uta_type_node); } + /* For libgcc-internal use only. */ + if (flag_building_libgcc) +/* For libgcc enable-execute-stack.c. */ +builtin_define_with_int_value (__LIBGCC_TRAMPOLINE_SIZE__, + TRAMPOLINE_SIZE); + /* For use in assembly language. */ builtin_define_with_value (__REGISTER_PREFIX__, REGISTER_PREFIX, 0); builtin_define_with_value
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On Thu, 9 Jun 2011, Rainer Orth wrote: Indeed, that did the trick. The following revised patch has been bootstrapped without regressions on i386-pc-solaris2.10. I've already got build, Darwin and Mingw approval and can commit the osf and solaris parts on my own. If the C parts are ok now, I suppose I still need approval either from the various *BSD maintainers (who haven't chimed in yet) of from a global reviewer? The C parts of this version are OK. -- Joseph S. Myers jos...@codesourcery.com
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On 06/09/2011 09:42 AM, Rainer Orth wrote: Indeed, that did the trick. The following revised patch has been bootstrapped without regressions on i386-pc-solaris2.10. I've already got build, Darwin and Mingw approval and can commit the osf and solaris parts on my own. If the C parts are ok now, I suppose I still need approval either from the various *BSD maintainers (who haven't chimed in yet) of from a global reviewer? Presumably the BSD folks will eventually complain if something breaks. Patch is OK. bernd
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On Tue, 31 May 2011, Paolo Bonzini wrote: I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK affects only three cpus. Currently, our documentation of libgcc configury and macros used is close to non-existant. That's probably for someone who has implemented this stuff. True, OTOH HAVE_ENABLE_EXECUTE_STACK is a target macro, and those are well documented. Just say that it has to be defined if libgcc provides a non-trivial implementation of __enable_execute_stack; it doesn't need to delve into how to hack libgcc. As I understand it, HAVE_ENABLE_EXECUTE_STACK is only used in code under gcc/config/. That is, it is not a target macro as usually understood but is logically private to a few back ends (and it would be a bug to introduce uses of it elsewhere, just like it was a bug to introduce uses of TARGET_64BIT outside gcc/config/ when that macro also is logically private). -- Joseph S. Myers jos...@codesourcery.com
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Joseph S. Myers jos...@codesourcery.com writes: On Tue, 31 May 2011, Paolo Bonzini wrote: I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK affects only three cpus. Currently, our documentation of libgcc configury and macros used is close to non-existant. That's probably for someone who has implemented this stuff. True, OTOH HAVE_ENABLE_EXECUTE_STACK is a target macro, and those are well documented. Just say that it has to be defined if libgcc provides a non-trivial implementation of __enable_execute_stack; it doesn't need to delve into how to hack libgcc. As I understand it, HAVE_ENABLE_EXECUTE_STACK is only used in code under gcc/config/. That is, it is not a target macro as usually understood but is logically private to a few back ends (and it would be a bug to introduce uses of it elsewhere, just like it was a bug to introduce uses of TARGET_64BIT outside gcc/config/ when that macro also is logically private). That was my reasoning when originally not including the documenation in my patch. So I'll just remove the HAVE_ENABLE_EXECUTE_STACK docs from my revised patch http://gcc.gnu.org/ml/gcc-patches/2011-06/msg00366.html Are you ok with that and the __TRAMPOLINE_SIZE__ definition in c-cppbuiltin.c (c_cpp_builtins)? Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Joseph S. Myers jos...@codesourcery.com writes: On Mon, 6 Jun 2011, Rainer Orth wrote: Paolo Bonzini bonz...@gnu.org writes: * 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. Included in the revised patch below. This part will need Joseph's approval. Is this definition ever going to be of use to user code, or even to system headers? It looks purely like an implementation detail rather than something meaningful for users. That would tend to suggest conditioning it on the proposed new -fbuilding-libgcc option that I suggested in http://gcc.gnu.org/ml/gcc-patches/2010-10/msg01206.html. (There are a *lot* of target macros that are in a similar position - see the list under used by the compiler itself and libgcc in http://gcc.gnu.org/wiki/Top-Level_Libgcc_Migration. Even if we could define and document public semantics for corresponding predefined macros, I don't think they would be of practical use to users of GCC, so for most of them I expect defining __LIBGCC_macro, only if -fbuilding-libgcc, is the best way of eliminating the dependence on host tm.h without making implementation details visible to users of GCC.) So my view is that you should define __LIBGCC_TRAMPOLINE_SIZE__, only if -fbuilding-libgcc. I can give it a try if I can figure out how to define -fbuilding-libgcc via the option handling machinery. I just want to avoid having to implement too many unrelated cleanups, otherwise it will take a year to get this single patch done. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On 06/07/2011 05:30 PM, Rainer Orth wrote: So my view is that you should define __LIBGCC_TRAMPOLINE_SIZE__, only if -fbuilding-libgcc. I can give it a try if I can figure out how to define -fbuilding-libgcc via the option handling machinery. I just want to avoid having to implement too many unrelated cleanups, otherwise it will take a year to get this single patch done. Agreed, what about just renaming the macro to __LIBGCC_TRAMPOLINE_SIZE__ for now? I don't think it's fair to put on Rainer the burden of implementing the new flag right now. If no one beats me to it, I'll look at it when coming back from holidays in two weeks. Paolo
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On Tue, 7 Jun 2011, Rainer Orth wrote: So my view is that you should define __LIBGCC_TRAMPOLINE_SIZE__, only if -fbuilding-libgcc. I can give it a try if I can figure out how to define -fbuilding-libgcc via the option handling machinery. I just want to avoid having to That should be no more than ; Define extra predefined macros for use in libgcc. fbuilding-libgcc C ObjC C++ ObjC++ Undocumented Var(flag_building_libgcc) in c.opt. -- Joseph S. Myers jos...@codesourcery.com
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Paolo Bonzini bonz...@gnu.org writes:
* 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.
Included in the revised patch below. This part will need Joseph's approval.
* 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.
Ok, done.
* 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.
I've now included the relevant comments from the old
ENABLE_EXECUTE_STACK definitions where it's claimed this is done for
namespace cleanliness.
* 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.
I'm going for abort then. Silently failing here is bad IMO since the
user will have to debug an issue in the runtime library. libgcc2.c is
already using abort in a couple of places, so there's a precedent.
* gcc/config/i386/mingw32.h has
#ifdef IN_LIBGCC2
#includewindows.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.
Done.
* 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?
I've no idea. Given that OpenBSD uses the same implementation across
all targets, I'm treating the two other BSDs the same now, unless the
maintainers objects.
I've removed a couple of FIXME comments I had in the last version:
* Instead of __FreeBSD__, one could use HAVE_SYSCTLBYNAME instead, but
that would need a new libgcc config.h header. In addition, we might
have to check for kern.stackprot to make sure the code really works.
* Similarly, instead of testing __sun__ __svr4__, one could check
_SC_STACK_PROT.
* Last, rather than checking __NetBSD__, one could go for HAVE___SYSCTL.
* __sysctl is currently declared manully. AFAICS there's no header for
that. At least the FreeBSD libc declares it itself in two places:
lib/libc/sys/stack_protector.c and lib/libc/gen/sysctl.c.
Given that this is closely tied to the various platforms, it seems
appropriate to continue to use the OS defines.
Here's the full patch, regtested without regressions on
i386-pc-solaris2.11.
Ok for mainline?
Rainer
2011-05-29 Rainer Orth r...@cebitec.uni-bielefeld.de
gcc:
* config/alpha/netbsd.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/alpha/osf5.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/darwin.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/i386/mingw32.h (MINGW_ENABLE_EXECUTE_STACK): Remove.
(ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
[IN_LIBGCC2]: Don't include windows.h.
* config/i386/netbsd-elf.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/i386/netbsd64.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/netbsd.h (NETBSD_ENABLE_EXECUTE_STACK): Remove.
* config/openbsd.h (ENABLE_EXECUTE_STACK): Remove.
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On 06/06/2011 11:17 AM, Rainer Orth wrote: * Instead of __FreeBSD__, one could use HAVE_SYSCTLBYNAME instead, but that would need a new libgcc config.h header. In addition, we might have to check for kern.stackprot to make sure the code really works. * Similarly, instead of testing __sun__ __svr4__, one could check _SC_STACK_PROT. * Last, rather than checking __NetBSD__, one could go for HAVE___SYSCTL. * __sysctl is currently declared manully. AFAICS there's no header for that. At least the FreeBSD libc declares it itself in two places: lib/libc/sys/stack_protector.c and lib/libc/gen/sysctl.c. Given that this is closely tied to the various platforms, it seems appropriate to continue to use the OS defines. Agreed, it would be a worse can of worms when bootstrapping a target (i.e. no target headers are available). Parts I can approve are good. Thanks! Paolo
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On Fri, 3 Jun 2011, Rainer Orth wrote: * 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. The man page of mprotect on FreeBSD 9 (the next release) states the following which seems supportive of your theory: NAME mprotect -- control the protection of pages : DESCRIPTION The mprotect() system call changes the specified pages to have protection prot. Not all implementations will guarantee protection on a page basis; the granularity of protection changes may be as large as an entire region. A region is the virtual address space defined by the start and end addresses of a struct vm_map_entry. Gerald
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Rainer Orth r...@cebitec.uni-bielefeld.de writes: * 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. Apparently freebsd's mprotect aligns the address by itself, so it will not make any difference. At least linux's mprotect will barf if the address is unaligned. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Paolo Bonzini bonz...@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.
The various __enable_execute_stack
implementations differ in minor ways that would have to be autoconf'ed.
Just select them by $host.
Ok, I've done that.
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?
Another question: should we keep the variants in libgcc/config (like
your config/enable-execute-stack-mprotect.c) or at the toplevel (like
enable-execute-stack-empty.c)? At the moment I see a mixture of files
at the libgcc toplevel and others in config.
I would put them under config/ unless they are platform-independent.
Ok, done.
I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK
affects only three cpus. Currently, our documentation of libgcc
configury and macros used is close to non-existant. That's probably for
someone who has implemented this stuff.
True, OTOH HAVE_ENABLE_EXECUTE_STACK is a target macro, and those are well
documented. Just say that it has to be defined if libgcc provides a
non-trivial implementation of __enable_execute_stack; it doesn't need to
delve into how to hack libgcc.
Will do when I submit the final patch. Right now, I've got a plan and a
draft where I'm looking for comments.
you can make the above work, that would be great as an example of how to
move stuff to the libgcc toplevel directory.
I'll give it a try, but it might take over the weekend.
Take your time, we're in stage1 now.
We had a public holiday yesterday :-)
So here we go: I'm including only the libgcc part of the patch, the rest
is unchanged.
The patch is only lightly tested so far, i.e. bootstrapped on
i386-pc-solaris2.8 and i386-pc-solaris2.11.
The existing variants (listed by the previous libgcc headers) are:
alpha/osf5-lib.h mprotect
addr page, end + TRAMPOLINE_SIZE
darwin-lib.h mprotect
addr page, end + TARGET_64BIT ? 48 : 40
rs6000_trampoline_size, cannot use
trampoline size which is a function call
freebsd-lib.h mprotect
addr, TRAMPOLINE_SIZE, why no page?
check_enabling via sysctlbyname
i386/mingw32-lib.hVirtualProtect
netbsd-lib.h mprotect
addr page, end + TRAMPOLINE_SIZE
pagesize via __sysctl
openbsd-lib.h mprotect
addr page, end + TRAMPOLINE_SIZE
sol2-lib.hmprotect
addr page, end + TRAMPOLINE_SIZE
check_enabling via sysconf
mingw32 is completely separate, while the others have much in common,
namely the use of mprotect to enable stack RWX permissions.
But there are also considerable differences:
* 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__.
* 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.
* NetBSD is extremely careful to avoid namespace pollution and uses
private __sysctl (declared privately) to avoid using getpagesize.
Either this is an issue, and
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On 06/03/2011 05:45 PM, Rainer Orth wrote:
Paolo Bonzinibonz...@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
#includewindows.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
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On 05/30/2011 05:59 PM, Rainer Orth wrote:
This is my hopefully last patch for toplevel libgcc moves: it moves
ENABLE_EXECUTE_STACK to $target-lib.h headers in libgcc/config. Since
gcc/config/sol2.h is only used on Solaris targets anymore and Solaris 8
is the minimal supported version, I've removed both a Solaris 2.6
workaround and includesys/mman.h directly. Same thing in osf5-lib.h.
Since the existance of the macro is used in alpha/alpha.c, i386/i386.c,
and sparc/sparc.c to enable calls to __enable_execute_stack, I had to
define HAVE_ENABLE_EXECUTE_STACK in the gcc/config headers to convey the
necessary information.
The new libgcc/config/$target-lib.h headers are added to libgcc_tm_file
in gcc/config.gcc. I'd rather add them to libgcc/config.host instead so
the information is kept local to libgcc.
Did you have any problems doing so?
Long term, it would be even better to do something like this in
libgcc/config.host:
enable_execute_stack=enable-execute-stack-empty.c
case $host in
...) enable_execute_stack=config/enable-execute-stack-mprotect.c ;;
...
esac
in libgcc/configure.ac:
AC_CONFIG_LINKS(enable-execute-stack.c:$enable_execute_stack)
in libgcc/Makefile.in:
LIB2ADD += enable-execute-stack.c
and drop this hunk altogether from gcc/libgcc2.c:
#ifdef L_enable_execute_stack
/* Attempt to turn on execute permission for the stack. */
#ifdef ENABLE_EXECUTE_STACK
ENABLE_EXECUTE_STACK
#else
void
__enable_execute_stack (void *addr __attribute__((__unused__)))
{}
#endif /* ENABLE_EXECUTE_STACK */
#endif /* L_enable_execute_stack */
Bootstrapped without regressions on i386-pc-solaris2.11 and
sparc-sun-solaris2.11.
I've Cc'ed the OS port maintainers of the other affected targets and
would appreciate testing/review. An OpenBSD maintainer isn't listed,
unfortunately. Also included are the CPU port maintainers for the
modified backends.
Ok for mainline after a week if no problems occur in testing on the
other targets?
It's a good start, but at least you need changes to the documentation;
if you can make the above work, that would be great as an example of how
to move stuff to the libgcc toplevel directory.
Paolo
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
Paolo Bonzini bonz...@gnu.org writes:
The new libgcc/config/$target-lib.h headers are added to libgcc_tm_file
in gcc/config.gcc. I'd rather add them to libgcc/config.host instead so
the information is kept local to libgcc.
Did you have any problems doing so?
There weren't any provisions in libgcc/configure.ac similar to
libgcc_tm_file in gcc/configure.ac, so I took the easy way out.
Long term, it would be even better to do something like this in
libgcc/config.host:
enable_execute_stack=enable-execute-stack-empty.c
case $host in
...) enable_execute_stack=config/enable-execute-stack-mprotect.c ;;
...
esac
in libgcc/configure.ac:
AC_CONFIG_LINKS(enable-execute-stack.c:$enable_execute_stack)
in libgcc/Makefile.in:
LIB2ADD += enable-execute-stack.c
and drop this hunk altogether from gcc/libgcc2.c:
#ifdef L_enable_execute_stack
/* Attempt to turn on execute permission for the stack. */
#ifdef ENABLE_EXECUTE_STACK
ENABLE_EXECUTE_STACK
#else
void
__enable_execute_stack (void *addr __attribute__((__unused__)))
{}
#endif /* ENABLE_EXECUTE_STACK */
#endif /* L_enable_execute_stack */
Seems like a plan, but I didn't go in this direction since I couldn't
test anything like this. The various __enable_execute_stack
implementations differ in minor ways that would have to be autoconf'ed.
This should be done by someone with access to the affected platforms.
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.
Another question: should we keep the variants in libgcc/config (like
your config/enable-execute-stack-mprotect.c) or at the toplevel (like
enable-execute-stack-empty.c)? At the moment I see a mixture of files
at the libgcc toplevel and others in config.
Bootstrapped without regressions on i386-pc-solaris2.11 and
sparc-sun-solaris2.11.
I've Cc'ed the OS port maintainers of the other affected targets and
would appreciate testing/review. An OpenBSD maintainer isn't listed,
unfortunately. Also included are the CPU port maintainers for the
modified backends.
Ok for mainline after a week if no problems occur in testing on the
other targets?
It's a good start, but at least you need changes to the documentation; if
I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK
affects only three cpus. Currently, our documentation of libgcc
configury and macros used is close to non-existant. That's probably for
someone who has implemented this stuff.
you can make the above work, that would be great as an example of how to
move stuff to the libgcc toplevel directory.
I'll give it a try, but it might take over the weekend.
Thanks.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
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. The various __enable_execute_stack implementations differ in minor ways that would have to be autoconf'ed. Just select them by $host. 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. Another question: should we keep the variants in libgcc/config (like your config/enable-execute-stack-mprotect.c) or at the toplevel (like enable-execute-stack-empty.c)? At the moment I see a mixture of files at the libgcc toplevel and others in config. I would put them under config/ unless they are platform-independent. I'd thought about it, but refrained since HAVE_ENABLE_EXECUTE_STACK affects only three cpus. Currently, our documentation of libgcc configury and macros used is close to non-existant. That's probably for someone who has implemented this stuff. True, OTOH HAVE_ENABLE_EXECUTE_STACK is a target macro, and those are well documented. Just say that it has to be defined if libgcc provides a non-trivial implementation of __enable_execute_stack; it doesn't need to delve into how to hack libgcc. you can make the above work, that would be great as an example of how to move stuff to the libgcc toplevel directory. I'll give it a try, but it might take over the weekend. Take your time, we're in stage1 now. Paolo
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
On May 30, 2011, at 8:59 AM, Rainer Orth wrote: This is my hopefully last patch for toplevel libgcc moves: it moves ENABLE_EXECUTE_STACK to $target-lib.h headers in libgcc/config. Ok for mainline after a week if no problems occur in testing on the other targets? Ok for the darwin bits.
Re: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
- Original Message -
From: Rainer Orth r...@cebitec.uni-bielefeld.de
To: gcc-patches@gcc.gnu.org
Cc: Joseph S. Myers jos...@codesourcery.com, Paolo Bonzini
bonz...@gnu.org, Ralf Wildenhues ralf.wildenh...@gmx.de, Mike Stump
mikest...@comcast.net, Loren J. Rittle ljrit...@acm.org, Kai Tietz
kti...@redhat.com, Dave Korn dave.korn.cyg...@gmail.com, Jason Thorpe
thor...@netbsd.org, Krister Walfridsson krister.walfrids...@gmail.com,
Uros Bizjak ubiz...@gmail.com, Richard Henderson r...@redhat.com, Eric
Botcazou ebotca...@adacore.com
Sent: Monday, May 30, 2011 5:59:10 PM
Subject: [build] Move ENABLE_EXECUTE_STACK to toplevel libgcc
This is my hopefully last patch for toplevel libgcc moves: it moves
ENABLE_EXECUTE_STACK to $target-lib.h headers in libgcc/config. Since
gcc/config/sol2.h is only used on Solaris targets anymore and Solaris 8
is the minimal supported version, I've removed both a Solaris 2.6
workaround and include sys/mman.h directly. Same thing in osf5-lib.h.
Since the existance of the macro is used in alpha/alpha.c, i386/i386.c,
and sparc/sparc.c to enable calls to __enable_execute_stack, I had to
define HAVE_ENABLE_EXECUTE_STACK in the gcc/config headers to convey the
necessary information.
The new libgcc/config/$target-lib.h headers are added to libgcc_tm_file
in gcc/config.gcc. I'd rather add them to libgcc/config.host instead so
the information is kept local to libgcc.
Bootstrapped without regressions on i386-pc-solaris2.11 and
sparc-sun-solaris2.11.
I've Cc'ed the OS port maintainers of the other affected targets and
would appreciate testing/review. An OpenBSD maintainer isn't listed,
unfortunately. Also included are the CPU port maintainers for the
modified backends.
Ok for mainline after a week if no problems occur in testing on the
other targets?
Thanks.
Rainer
2011-05-29 Rainer Orth r...@cebitec.uni-bielefeld.de
gcc:
* config/alpha/netbsd.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/alpha/osf5.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/darwin.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/i386/mingw32.h (MINGW_ENABLE_EXECUTE_STACK): Remove.
(ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/i386/netbsd-elf.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/i386/netbsd64.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/netbsd.h (NETBSD_ENABLE_EXECUTE_STACK): Remove.
* config/openbsd.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/sol2.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/sparc/freebsd.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/sparc/netbsd-elf.h (ENABLE_EXECUTE_STACK): Remove.
(HAVE_ENABLE_EXECUTE_STACK): Define.
* config/alpha/alpha.c (alpha_trampoline_init): Test
HAVE_ENABLE_EXECUTE_STACK.
* config/i386/i386.c (ix86_trampoline_init): Likewise.
* config/sparc/sparc.c (sparc32_initialize_trampoline): Likewise.
(sparc64_initialize_trampoline): Likewise.
* config.gcc (*-*-darwin*): Add darwin-lib.h to libgcc_tm_file.
(*-*-openbsd*): Add openbsd-lib.h to libgcc_tm_file.
(*-*-solaris2*): Add sol2-lib.h to libgcc_tm_file.
(alpha*-*-netbsd*): Add netbsd-lib.h to libgcc_tm_file.
(alpha*-dec-osf5.1): Add alpha/osf5-lib.h to libgcc_tm_file.
(i[34567]86-*-mingw*): Add i386/mingw32-lib.h to libgcc_tm_file.
(i[34567]86-*-netbsdelf*, x86_64-*-netbsd*): Add netbsd-lib.h to
libgcc_tm_file.
(sparc64-*-freebsd*): Add freebsd-lib.h to libgcc_tm_file.
(sparc-*-netbsdelf*, sparc64-*-netbsd*): Add netbsd-lib.h to
libgcc_tm_file.
* system.h (ENABLE_EXECUTE_STACK): Poison.
libgcc:
* config/alpha/osf5-lib.h: New file.
* config/darwin-lib.h: New file.
* config/freebsd-lib.h: New file.
* config/i386/mingw32-lib.h: New file.
* config/netbsd-lib.h: New file.
* config/openbsd-lib.h: New file.
* config/sol2-lib.h: New file.
diff --git a/gcc/config.gcc b/gcc/config.gcc
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -487,6 +487,7 @@ case ${target} in
esac
tm_file=${tm_file} ${cpu_type}/darwin.h
tm_p_file=${tm_p_file} darwin-protos.h
+ libgcc_tm_file=${libgcc_tm_file} darwin-lib.h
target_gtfiles=\$(srcdir)/config/darwin.c
extra_options=${extra_options} darwin.opt
c_target_objs=${c_target_objs} darwin-c.o
@@ -648,6 +649,7 @@ case ${target} in
esac
;;
*-*-openbsd*)
+ libgcc_tm_file=${libgcc_tm_file} openbsd-lib.h
tmake_file=t-libc-ok t-openbsd t-libgcc-pic
case ${enable_threads
