Re: [Patch] New -fstack-check implementation (2/n)

[Ian Lance Taylor <iant@google.com>]

Eric Botcazou <ebotcazou@adacore.com> writes:

+@defmac STACK_CHECK_PROBE_IOR
+An integer which is nonzero if GCC should perform the stack probe
+as an inclusive OR instruction.  The default is zero.
 @end defmac

I know this follows the exisitng pattern of STACK_CHECK_PROBE_LOAD, but
I think it would be better to use an insn.  Why not use introduce a
check_stack_probe insn?  Or, since your patch is eliminating the
check_stack insn, why not reuse that?


> +@defmac STACK_CHECK_MOVING_SP
> +An integer which is nonzero if GCC should move the stack pointer during
> +stack checking.  This can be necessary on systems where the stack pointer
> +contains the bottom address of the memory area accessible to the executing
> +thread at any point in time.  In this situation an alternate signal stack
> +is required in order to be able to recover from a stack overflow.
> +The default value of this macro is zero.
> +@end defmac

I find this documentation to be somewhat cryptic.  If I understand
correctly, when this macro is defined, gcc will adjust the stack pointer
page by page when doing probes.  Please say that in the doc.  It's not
clear to me why the Linux kernel requires this--can you expand on that?
And why a target macro rather than a target hook for something new?


> -  /* Enable GNAT stack checking method if needed */
> -  if (!Stack_Check_Probes_On_Target)
> -    set_stack_check_libfunc (gen_rtx_SYMBOL_REF (Pmode, "_gnat_stack_check"));
> +  /* Enable appropriate stack checking method.  */
> +  if (Stack_Check_Probes_On_Target)
> +    ;
> +  else if (Stack_Check_Limits_On_Target)
> +    stack_check_symbol = gen_rtx_SYMBOL_REF (Pmode, "__gnat_stack_limit");
> +  else
> +    stack_check_libfunc = gen_rtx_SYMBOL_REF (Pmode, "_gnat_stack_check");

Letting the frontend set a global variable in the middle end seems to me
to be an ugly interface.  Why did you change away from the
set_stack_check_libfunc function?

Much more seriously, I don't see how this can work when using LTO.  In
LTO, by the time we expand stack checking, the frontend is gone.  This
needs to work differently.


>  void
>  probe_stack_range (HOST_WIDE_INT first, rtx size)
>  {
> +#ifdef SPARC_STACK_BIAS
> +  /* The probe offsets are counted negatively whereas the stack bias is
> +     counted positively.  */
> +  first -= SPARC_STACK_BIAS;
> +#endif

This does not look good in target independent code.  This needs to
become an officially named and documented target hook.


> +#if defined(HAVE_conditional_trap)
> +      emit_insn (gen_cond_trap (LTU, avail, req, const0_rtx));
> +#elif defined(HAVE_trap)

It's not enough to check the #ifdef; you also have to check available
with if ().


> +/* Define this to be nonzero to use an inclusive OR.  */
> +#define STACK_CHECK_PROBE_IOR 1

Don't write this style of comment in a tm.h file.  That's what the docs
are for.  Instead, write a comment saying why this should be defined to
1 here.


> +/* Define this to be nonzero if static stack checking is supported.  */
> +#define STACK_CHECK_STATIC_BUILTIN 1
> +
> +/* Define this to be nonzero if the stack pointer needs to be moved.  */
> +#define STACK_CHECK_MOVING_SP 1

Same here.


Ian