On 06/03/2011 05:45 PM, Rainer Orth wrote:
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.
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
