Re: [PATCH] PR880088 Enable -Wtrampolines for no-exec-stack targets with -Wall.

[Mark Wielaard <mark at klomp dot org>]

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