Re: [PATCH 7/7] tools/nolibc: simplify stackprotector compiler flags

[Thomas Weißschuh]

On 2023-05-23 22:12:38+0200, Willy Tarreau wrote:
> Hi Thomas, Zhangjin,
> 
> I merged and pushed this series on top of the previous series, it's in
> branch 20230523-nolibc-rv32+stkp3.
> 
> However, Thomas, I found an issue with this last patch that I partially
> reverted in a last patch I pushed as well so that we can discuss it:
> 
> On Sun, May 21, 2023 at 11:36:35AM +0200, Thomas Weißschuh wrote:
> >  
> > -CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \
> > -			$(call cc-option,-mstack-protector-guard=global) \
> > -			$(call cc-option,-fstack-protector-all)
> > -CFLAGS_STKP_i386 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_x86_64 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_x86 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_arm64 = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_arm = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_mips = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_riscv = $(CFLAGS_STACKPROTECTOR)
> > -CFLAGS_STKP_loongarch = $(CFLAGS_STACKPROTECTOR)
> > +CFLAGS_STACKPROTECTOR = $(call cc-option,-mstack-protector-guard=global -fstack-protector-all)
> >  CFLAGS_s390 = -m64
> >  CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
> >  		$(call cc-option,-fno-stack-protector) \
> > +		$(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all)) \
> >  		$(CFLAGS_STKP_$(ARCH)) $(CFLAGS_$(ARCH))
> 
> By doing so, we reintroduce the forced stack-protector mechanism
> that cannot be disabled anymore. The ability to disable it was the
> main point of the options above. In fact checking __SSP__* was a
> solution to save the user from having to set -DNOLIBC_STACKPROTECTOR
> to match their compiler's flags, but here in the nolibc-test we still
> want to be able to forcefully disable stkp for a run (at least to
> unbreak gcc-9, and possibly to confirm that non-stkp builds still
> continue to work when your local toolchain has it by default).

Wouldn't this work?

make CFLAGS_x86=-fno-stack-protector nolibc-test

Or we do 

CFLAGS_STACKPROTECTOR ?= $(call cc-option,-mstack-protector-guard=global $(call cc-option,-fstack-protector-all))
CFLAGS ?= ... $(CFLAGS_STACKPROTECTOR)

And then it is always:

make CFLAGS_STACKPROTECTOR= nolibc-test

An alternative would also be to use a GCC 9 compatible mechanism:

#if __has_attribute(no_stack_protector)
#define __no_stack_protector __attribute__((no_stack_protector))
#else
#define __no_stack_protector __attribute__((__optimize__("-fno-stack-protector")))
#endif

Or we combine CFLAGS_STACKPROTECTOR and __no_stack_protector.

> So I reverted that part and only dropped -DNOLIBC_STACKPROTECTOR.
> This way I could run the test on all archs with gcc-9 by forcing
> CFLAGS_STACKPROTECTOR= and verified it was still possible to
> disable it for a specific arch only by setting CFLAGS_STKP_$ARCH.
> 
> If you're fine with this we can squash this one into your cleanup
> patch.

To be honest I was happy to get rid of all these architecture specific
variables.

> Zhangjin I think this branch should work well enough for you to
> serve as a base for your upcoming series. We'll clean it up later
> once we all agree on the stat() replacement for syscall() and on
> the various remaining points including your time32 alternatives.
> 
> Thanks to you both,
> Willy
> 
> PS: we're still carrying a long CC list of individuals who are likely
>     not that much interested in our discussion, I propose that we trim
>     it on next exchanges and only keep us 3 and the lists, in order to
>     remove some of their e-mail pollution.

I trimmed the list a bit.

Thomas