Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
Hi Mark, On Thu, Nov 29, 2018 at 12:57:03PM +0100, Mark Wielaard wrote: > On Wed, 2018-11-28 at 15:00 -0600, Segher Boessenkool wrote: > > On Tue, Nov 27, 2018 at 08:54:07PM +0100, Mark Wielaard wrote: > > > On Tue, 2018-11-27 at 12:37 -0600, Segher Boessenkool wrote: > > > > > diff --git a/gcc/config/rs6000/sysv4.h > > > > > b/gcc/config/rs6000/sysv4.h > > > > > index 0c67634..9330acf 100644 > > > > > --- a/gcc/config/rs6000/sysv4.h > > > > > +++ b/gcc/config/rs6000/sysv4.h > > > > > @@ -972,6 +972,11 @@ ncrtn.o%s" > > > > > /* Generate entries in .fixup for relocatable addresses. */ > > > > > #define RELOCATABLE_NEEDS_FIXUP 1 > > > > > > > > > > +#if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) > > > > > + #define TARGET_HAS_DEFAULT_NOEXEC_STACK (TARGET_32BIT \ > > > > > +|| DEFAULT_ABI == > > > > > ABI_ELFv2) > > > > > +#endif > > > > > > > > I don't think this belongs in sysv4.h . > > > > > > I might have gotten lost in the tree of defines/macros. > > > > > > There are two sysv4.h files gcc/config/rs6000/sysv4.h and > > > gcc/config/powerpcspe/sysv4.h > > > > Forget about powerpcspe please, I am talking about rs6000 only. > > I don't know the differences between the config backends, but it looks > like at least some of the configs were just copy/pasted which might > explain why they both define things the same (in sysv4.h). And they Yes, things are copied. My point is, my review is for rs6000/ only :-) > > You want linux.h and freebsd.h, maybe the "64" versions of those separately. > > Or put this in rs6000.h. sysv4.h is a random header for this, it doesn't > > belong there. > > The reason I added it to sysv4.h is because it matches where the > TARGET_ASM_FILE_END hook is setup. It might make sense to have Yeah, but it does not make sense to put it there, since you are handling other ABIs as well. > So if I understand correctly you would like to have: > > rs6000/linux.h and rs6000/freebsd.h: > #define TARGET_NEEDS_EXEC_STACK_FLAG_FOR_TRAMPOLINES 1 > > rs6000/linux64.h and rs6000/freebsd64.h: > #define TARGET_NEEDS_EXEC_STACK_FLAG_FOR_TRAMPOLINES (DEFAULT_ABI == > ABI_ELFv2) Something like that yes. Or in rs6000.h, maybe. Some location that makes sense :-) Segher
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
Hi Segher, On Wed, 2018-11-28 at 15:00 -0600, Segher Boessenkool wrote: > On Tue, Nov 27, 2018 at 08:54:07PM +0100, Mark Wielaard wrote: > > On Tue, 2018-11-27 at 12:37 -0600, Segher Boessenkool wrote: > > > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for > > > > those > > > > targets > > > > that have a non-executable default stack based on when they > > > > call > > > > file_end_indicate_exec_stack. > > > > > > As Paul says, that name isn't so good. > > > > > > TARGET_NEEDS_MAKING_THE_STACK_EXECUTABLE_FOR_TRAMPOLINES, or > > > similar? > > > > Would the slightly shorter > > TARGET_NEEDS_EXEC_STACK_MARKER_FOR_TRAMPOLINES be descriptive > > enough? > > "MARKER", is that some official name for it? If no, just "FLAG"? > Fine with me, sure. FLAG it is then! It is even a little shorter :) > > > > diff --git a/gcc/config/rs6000/sysv4.h > > > > b/gcc/config/rs6000/sysv4.h > > > > index 0c67634..9330acf 100644 > > > > --- a/gcc/config/rs6000/sysv4.h > > > > +++ b/gcc/config/rs6000/sysv4.h > > > > @@ -972,6 +972,11 @@ ncrtn.o%s" > > > > /* Generate entries in .fixup for relocatable addresses. */ > > > > #define RELOCATABLE_NEEDS_FIXUP 1 > > > > > > > > +#if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) > > > > + #define TARGET_HAS_DEFAULT_NOEXEC_STACK (TARGET_32BIT \ > > > > + || DEFAULT_ABI == > > > > ABI_ELFv2) > > > > +#endif > > > > > > I don't think this belongs in sysv4.h . > > > > I might have gotten lost in the tree of defines/macros. > > > > There are two sysv4.h files gcc/config/rs6000/sysv4.h and > > gcc/config/powerpcspe/sysv4.h > > Forget about powerpcspe please, I am talking about rs6000 only. I don't know the differences between the config backends, but it looks like at least some of the configs were just copy/pasted which might explain why they both define things the same (in sysv4.h). And they both use the same TARGET_ASM_FILE_END hook (set to rs6000_elf_file_end although that function also seems copy/pasted between powerpcspe.c and rs6000.c. Could you explain why I should forget about powerpcspe and/or why where and how to setup the new target marco would be different between the two config backends? > You want linux.h and freebsd.h, maybe the "64" versions of those separately. > Or put this in rs6000.h. sysv4.h is a random header for this, it doesn't > belong there. The reason I added it to sysv4.h is because it matches where the TARGET_ASM_FILE_END hook is setup. It might make sense to have specialized TARGET_ASM_FILE_END hooks too for [linux,freebsd)(64)].h. But that is probably a different discussion. So if I understand correctly you would like to have: rs6000/linux.h and rs6000/freebsd.h: #define TARGET_NEEDS_EXEC_STACK_FLAG_FOR_TRAMPOLINES 1 rs6000/linux64.h and rs6000/freebsd64.h: #define TARGET_NEEDS_EXEC_STACK_FLAG_FOR_TRAMPOLINES (DEFAULT_ABI == ABI_ELFv2) If so, shouldn't the same be done for powerpcspe? Sorry, you told me to forget about it, but I just cannot :) Thanks, Mark
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
On Tue, Nov 27, 2018 at 08:54:07PM +0100, Mark Wielaard wrote: > On Tue, 2018-11-27 at 12:37 -0600, Segher Boessenkool wrote: > > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those > > > targets > > > that have a non-executable default stack based on when they call > > > file_end_indicate_exec_stack. > > > > As Paul says, that name isn't so good. > > > > TARGET_NEEDS_MAKING_THE_STACK_EXECUTABLE_FOR_TRAMPOLINES, or similar? > > Would the slightly shorter > TARGET_NEEDS_EXEC_STACK_MARKER_FOR_TRAMPOLINES be descriptive enough? "MARKER", is that some official name for it? If no, just "FLAG"? Fine with me, sure. > > > diff --git a/gcc/config/rs6000/sysv4.h b/gcc/config/rs6000/sysv4.h > > > index 0c67634..9330acf 100644 > > > --- a/gcc/config/rs6000/sysv4.h > > > +++ b/gcc/config/rs6000/sysv4.h > > > @@ -972,6 +972,11 @@ ncrtn.o%s" > > > /* Generate entries in .fixup for relocatable addresses. */ > > > #define RELOCATABLE_NEEDS_FIXUP 1 > > > > > > +#if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) > > > + #define TARGET_HAS_DEFAULT_NOEXEC_STACK (TARGET_32BIT \ > > > +|| DEFAULT_ABI == ABI_ELFv2) > > > +#endif > > > > I don't think this belongs in sysv4.h . > > I might have gotten lost in the tree of defines/macros. > > There are two sysv4.h files gcc/config/rs6000/sysv4.h and > gcc/config/powerpcspe/sysv4.h Forget about powerpcspe please, I am talking about rs6000 only. You want linux.h and freebsd.h, maybe the "64" versions of those separately. Or put this in rs6000.h. sysv4.h is a random header for this, it doesn't belong there. Segher
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
Hi, On Tue, 2018-11-27 at 12:37 -0600, Segher Boessenkool wrote: > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those > > targets > > that have a non-executable default stack based on when they call > > file_end_indicate_exec_stack. > > As Paul says, that name isn't so good. > > TARGET_NEEDS_MAKING_THE_STACK_EXECUTABLE_FOR_TRAMPOLINES, or similar? Would the slightly shorter TARGET_NEEDS_EXEC_STACK_MARKER_FOR_TRAMPOLINES be descriptive enough? > > diff --git a/gcc/config/rs6000/sysv4.h b/gcc/config/rs6000/sysv4.h > > index 0c67634..9330acf 100644 > > --- a/gcc/config/rs6000/sysv4.h > > +++ b/gcc/config/rs6000/sysv4.h > > @@ -972,6 +972,11 @@ ncrtn.o%s" > > /* Generate entries in .fixup for relocatable addresses. */ > > #define RELOCATABLE_NEEDS_FIXUP 1 > > > > +#if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) > > + #define TARGET_HAS_DEFAULT_NOEXEC_STACK (TARGET_32BIT \ > > + || DEFAULT_ABI == ABI_ELFv2) > > +#endif > > I don't think this belongs in sysv4.h . I might have gotten lost in the tree of defines/macros. There are two sysv4.h files gcc/config/rs6000/sysv4.h and gcc/config/powerpcspe/sysv4.h Both define the TARGET_ASM_FILE_END hook to rs6000_elf_file_end. Which are defined in config/rs6000/rs6000.c and config/powerpcspe/powerpcspe.c. Both have: #if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) if (TARGET_32BIT || DEFAULT_ABI == ABI_ELFv2) file_end_indicate_exec_stack (); #endif And file_end_indicate_exec_stack () is what you call to flip the stack to be executable (if an executable stack marker is needed for generating the trampoline). That is why both sysv4.h files have the new target macro defined the same way. But maybe only one of them really needs it? Thanks, Mark
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
Hi Mark, On Mon, Nov 26, 2018 at 10:13:29AM +0100, Mark Wielaard wrote: > With -Wtrampolines a warning is produced whenever gcc generates executable > code on the stack at runtime to support taking a nested function address > that is used to call the nested function indirectly when it needs to access > any variables in its lexical scope. > > As a result the stack has to be marked as executable even for targets which > have a non-executable stack as default. > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those targets > that have a non-executable default stack based on when they call > file_end_indicate_exec_stack. As Paul says, that name isn't so good. TARGET_NEEDS_MAKING_THE_STACK_EXECUTABLE_FOR_TRAMPOLINES, or similar? > diff --git a/gcc/config/rs6000/sysv4.h b/gcc/config/rs6000/sysv4.h > index 0c67634..9330acf 100644 > --- a/gcc/config/rs6000/sysv4.h > +++ b/gcc/config/rs6000/sysv4.h > @@ -972,6 +972,11 @@ ncrtn.o%s" > /* Generate entries in .fixup for relocatable addresses. */ > #define RELOCATABLE_NEEDS_FIXUP 1 > > +#if defined (POWERPC_LINUX) || defined (POWERPC_FREEBSD) > + #define TARGET_HAS_DEFAULT_NOEXEC_STACK (TARGET_32BIT \ > +|| DEFAULT_ABI == ABI_ELFv2) > +#endif I don't think this belongs in sysv4.h . Segher
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
On Mon, 2018-11-26 at 18:29 +, Joseph Myers wrote: > On Mon, 26 Nov 2018, Mark Wielaard wrote: > > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those > > targets > > that have a non-executable default stack based on when they call > > file_end_indicate_exec_stack. > > Some targets (e.g. ia64) may default to no-exec stacks without > needing > that hook (e.g. if they use function descriptors, so trampolines have > no > need for an executable stack). Yes, but for such targets it doesn't really matter whether or not they define this new target macro. The -Wtrampolines warning only warns when creating an executable runtime trampoline on the stack. It won't for non-executable trampolines. So yes, then this target macro doesn't have to be defined even if the stack is non-executable by default. Defining the TARGET_HAS_DEFAULT_NOEXEC_STACK macro for such a target will then make it so that -Wall enables -Wtrampolines by default, but -Wtrampolines still won't warn because the trampolines aren't executable code on the stack. But I believe that is the correct behavior. The warning should only happen if gcc would otherwise silently make the stack executable for a target that has a default non- executable stack (and it won't for target that uses function descriptors). I thought the documentation updates for the warning made that more clear, but maybe they can be improved even more? A case could be made that taking a nested function pointer that escapes the lexical scope from which it takes some context variable always is a dangerous thing to do and that it should even warn if supporting that doesn't involve placing an executable runtime trampoline on the stack. Which is actually the case I made in the original bug. But I don't believe I got consensus for that. Thanks, Mark
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
On Mon, 26 Nov 2018, Mark Wielaard wrote: > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those targets > that have a non-executable default stack based on when they call > file_end_indicate_exec_stack. Some targets (e.g. ia64) may default to no-exec stacks without needing that hook (e.g. if they use function descriptors, so trampolines have no need for an executable stack). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
On Mon, 2018-11-26 at 08:19 -0500, Paul Koning wrote: > > On Nov 26, 2018, at 4:13 AM, Mark Wielaard wrote: > > > > With -Wtrampolines a warning is produced whenever gcc generates > > executable > > code on the stack at runtime to support taking a nested function > > address > > that is used to call the nested function indirectly when it needs > > to access > > any variables in its lexical scope. > > > > As a result the stack has to be marked as executable even for > > targets which > > have a non-executable stack as default. > > > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those > > targets > > that have a non-executable default stack based on when they call > > file_end_indicate_exec_stack. > > The word "default" suggests this is a constant attribute of the > platform. Can it be variable? No. The new target macro is just for those targets that have no executable stack by default. It basically tracks those targets that have a static rule for calling file_end_indicate_exec_stack () from their TARGET_ASM_FILE_END hook. See also the new documentation for the target macro and the existing documentation for the TARGET_ASM_FILE_END hook. I would also be fine with having -Wall enable -Wtrampolines by default. But in the bug report it was argued that it should only for targets with a default non-executable stack where gcc would otherwise silently make the whole stack executable. Cheers, Mark
Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
> On Nov 26, 2018, at 4:13 AM, Mark Wielaard wrote: > > With -Wtrampolines a warning is produced whenever gcc generates executable > code on the stack at runtime to support taking a nested function address > that is used to call the nested function indirectly when it needs to access > any variables in its lexical scope. > > As a result the stack has to be marked as executable even for targets which > have a non-executable stack as default. > > Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those targets > that have a non-executable default stack based on when they call > file_end_indicate_exec_stack. The word "default" suggests this is a constant attribute of the platform. Can it be variable? Whether the stack is executable or not may depend on -m switches. paul
[PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.
With -Wtrampolines a warning is produced whenever gcc generates executable code on the stack at runtime to support taking a nested function address that is used to call the nested function indirectly when it needs to access any variables in its lexical scope. As a result the stack has to be marked as executable even for targets which have a non-executable stack as default. Define a new target macro TARGET_HAS_DEFAULT_NOEXEC_STACK for those targets that have a non-executable default stack based on when they call file_end_indicate_exec_stack. Add -Wtrampolines to -Wall for those targets and update the documentation to better explain when -Wtrampolines will warn. gcc/ChangeLog PR c/880088 * common.opt (Wtrampolines): Improve documentation. * doc/invoke.texi (-Wtrampolines): Likewise. * doc/tm.texi.in (TARGET_HAS_DEFAULT_NOEXEC_STACK): Document macro. * doc/tm.texi: Regenerate. * defaults.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to zero if not defined yet. * config/aarch64/aarch64-freebsd.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to 1. * config/aarch64/aarch64-linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/alpha/linux-elf.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/arc/elf.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/arc/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/arm/arm.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to NEED_INDICATE_EXEC_STACK. * config/cris/cris.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to TARGET_LINUX. * config/i386/dragonfly.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/i386/freebsd.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/i386/gnu-user-common.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/i386/linux-common.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/m32r/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/m68k/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/microblaze/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/nds32/nds32.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to TARGET_LINUX_ABI. * config/nios2/nios2.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/pa/pa.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to NEED_INDICATE_EXEC_STACK. * config/powerpcspe/sysv4.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to TARGET_32BIT || DEFAULT_ABI == ABI_ELFv2 for POWERPC_LINUX || POWERPC_FREEBSD. * config/rs6000/sysv4.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/riscv/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to 1. * config/s390/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/s390/s390.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/sh/linux.h (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/sparc/sparc.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Define to NEED_INDICATE_EXEC_STACK. * config/tilegx/tilegx.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. * config/tilepro/tilepro.c (TARGET_HAS_DEFAULT_NOEXEC_STACK): Likewise. gcc/c-family/ChangeLog PR c/880088 * c.opt (Wtrampolines): Enable by -Wall when TARGET_HAS_DEFAULT_NOEXEC_STACK. --- gcc/ChangeLog| 51 gcc/c-family/ChangeLog | 6 + gcc/c-family/c.opt | 3 +++ gcc/common.opt | 2 +- gcc/config/aarch64/aarch64-freebsd.h | 1 + gcc/config/aarch64/aarch64-linux.h | 1 + gcc/config/alpha/linux-elf.h | 1 + gcc/config/arc/elf.h | 3 +++ gcc/config/arc/linux.h | 3 +++ gcc/config/arm/arm.c | 3 +++ gcc/config/cris/cris.c | 2 ++ gcc/config/i386/dragonfly.h | 1 + gcc/config/i386/freebsd.h| 1 + gcc/config/i386/gnu-user-common.h| 2 ++ gcc/config/i386/linux-common.h | 3 +++ gcc/config/m32r/linux.h | 1 + gcc/config/m68k/linux.h | 1 + gcc/config/microblaze/linux.h| 1 + gcc/config/nds32/nds32.c | 3 +++ gcc/config/nios2/nios2.c | 3 +++ gcc/config/pa/pa.c | 3 +++ gcc/config/powerpcspe/sysv4.h| 5 gcc/config/riscv/linux.h | 1 + gcc/config/rs6000/sysv4.h| 5 gcc/config/s390/linux.h | 1 + gcc/config/s390/s390.c | 3 +++ gcc/config/sh/linux.h| 1 + gcc/config/sparc/sparc.c | 3 +++ gcc/config/tilegx/tilegx.c | 3 +++ gcc/config/tilepro/tilepro.c | 3 +++ gcc/defaults.h | 6 + gcc/doc/invoke.texi | 20 +- gcc/doc/tm.texi | 11 gcc/doc/tm.texi.in | 11 34 files changed, 160 insertions(+), 8 deletions(-) diff --git
