Hi, Alexandre,
> On Jun 16, 2023, at 3:26 AM, Alexandre Oliva <ol...@adacore.com> wrote:
>
> Hello, Qing,
>
> On Oct 27, 2022, Qing Zhao <qing.z...@oracle.com> wrote:
> <https://gcc.gnu.org/pipermail/gcc-patches/2022-October/604480.html>
>
>> On Oct 26, 2022, at 5:29 PM, Alexandre Oliva <ol...@adacore.com> wrote:
>>> I'm sure there are other scenarios in which keeping at least the
>>> possibility of 'all' is useful.
>> Okay.
>
>
>> i.e, instead introducing a new MODE “LEAFY_MODE” and a new user
>> sub-option, for LEAF functions, only
>> Clear its’ used registers even for “ALL”.
>
>> However, since there is need to clear the un-used registers for leaf
>> functions. It looks like it is needed to provide
>> This new sub-option to users.
>
>> Is this clear this time?
>
> Yeah, I guess I understand what you mean. But since there are cases in
> which clearing all (call-clobbered) registers in a leaf function is
> useful, I suppose it makes sense to offer both possibilities.
agreed.
>
> If there was a default operation mode for -fzero-call-used-regs, I guess
> it would make sense to consider leafy the default, rather than all, but
> since there isn't, and it always has to be specified explicitly, that's
> not something to be considered.
>
> So the available choices are:
>
> 1. introduce 'leafy' as a separate mode, leaving 'all' alone
>
> 2. change the behavior of 'all' to that of the proposed 'leafy', and either
>
> 2.a) add another mode that retains the currently-useful behavior of 'all',
> or
>
> 2.b) make the current behavior of 'all' no longer available
>
> Personally, I find 1. the least disruptive to existing users of
> -fzero-call-used-regs. If we were introducing the option now, maybe 2.a
> would be more sensible, but at this point, changing the meaning of 'all'
> seems to be a disservice to security-sensitive users.
>
> Those who would prefer the leaner operation on leaf functions can then
> switch to 'leafy' mode, but that's better than finding carefully-crafted
> code relying on the current behavior of 'all' for security suddenly
> changes from under them, isn't it?
Yes, I agree.
>
>
> That said, I'm willing to implement the alternate change, if changing
> the expected behavior is preferred over offering a different choice, if
> needed to get closure on this feature.
>
> For now, I'm just pinging the refreshed and retested patch.
As I mentioned in the previous round of review, I think that the documentation
might need to add more details on what’s the LEAFY mode,
The purpose of it, and how to use it, provide more details to the end-users.
> Ok to install?
>
>
> Add leafy mode for zero-call-used-regs
>
> Introduce 'leafy' to auto-select between 'used' and 'all' for leaf and
> nonleaf functions, respectively.
>
>
> for gcc/ChangeLog
>
> * doc/extend.texi (zero-call-used-regs): Document leafy and
> variants thereof.
> * flag-types.h (zero_regs_flags): Add LEAFY_MODE, as well as
> LEAFY and variants.
> * function.cc (gen_call_ued_regs_seq): Set only_used for leaf
> functions in leafy mode.
> * opts.cc (zero_call_used_regs_opts): Add leafy and variants.
>
> for gcc/testsuite/ChangeLog
>
> * c-c++-common/zero-scratch-regs-leafy-1.c: New.
> * c-c++-common/zero-scratch-regs-leafy-2.c: New.
> * gcc.target/i386/zero-scratch-regs-leafy-1.c: New.
> * gcc.target/i386/zero-scratch-regs-leafy-2.c: New.
> ---
> gcc/doc/extend.texi | 22 ++++++++++++++++++--
> gcc/flag-types.h | 5 +++++
> gcc/function.cc | 3 +++
> gcc/opts.cc | 4 ++++
> .../c-c++-common/zero-scratch-regs-leafy-1.c | 15 ++++++++++++++
> .../c-c++-common/zero-scratch-regs-leafy-2.c | 21 +++++++++++++++++++
> .../gcc.target/i386/zero-scratch-regs-leafy-1.c | 12 +++++++++++
> .../gcc.target/i386/zero-scratch-regs-leafy-2.c | 16 +++++++++++++++
> 8 files changed, 96 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-1.c
> create mode 100644 gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-2.c
> create mode 100644 gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-1.c
> create mode 100644 gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-2.c
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 7b5592502734e..f8b0bb53ef5d4 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
I think in the documentation of zero_call_used_regs,
After the description of the 3 basic values: “skip”, “used”, and “all”.
The description of the new value “leafy” need to be added first.
In addition to the below doc change.
The others LGTM.
Thanks.
Qing
> @@ -4412,10 +4412,28 @@ zeros all call-used registers that pass arguments.
> @item all-gpr-arg
> zeros all call-used general purpose registers that pass
> arguments.
> +
> +@item leafy
> +Same as @samp{used} in a leaf function, and same as @samp{all} in a
> +nonleaf function.
> +
> +@item leafy-gpr
> +Same as @samp{used-gpr} in a leaf function, and same as @samp{all-gpr}
> +in a nonleaf function.
> +
> +@item leafy-arg
> +Same as @samp{used-arg} in a leaf function, and same as @samp{all-arg}
> +in a nonleaf function.
> +
> +@item leafy-gpr-arg
> +Same as @samp{used-gpr-arg} in a leaf function, and same as
> +@samp{all-gpr-arg} in a nonleaf function.
> +
> @end table
>
> -Of this list, @samp{used-arg}, @samp{used-gpr-arg}, @samp{all-arg},
> -and @samp{all-gpr-arg} are mainly used for ROP mitigation.
> +Of this list, @samp{used-arg}, @samp{used-gpr-arg}, @samp{leafy-arg},
> +@samp{leafy-gpr-arg}, @samp{all-arg}, and @samp{all-gpr-arg} are mainly
> +used for ROP mitigation.
>
> The default for the attribute is controlled by @option{-fzero-call-used-regs}.
> @end table
> diff --git a/gcc/flag-types.h b/gcc/flag-types.h
> index f83d165fbfef1..6a2e1beb997ef 100644
> --- a/gcc/flag-types.h
> +++ b/gcc/flag-types.h
> @@ -348,6 +348,7 @@ namespace zero_regs_flags {
> const unsigned int ONLY_GPR = 1UL << 2;
> const unsigned int ONLY_ARG = 1UL << 3;
> const unsigned int ENABLED = 1UL << 4;
> + const unsigned int LEAFY_MODE = 1UL << 5;
> const unsigned int USED_GPR_ARG = ENABLED | ONLY_USED | ONLY_GPR | ONLY_ARG;
> const unsigned int USED_GPR = ENABLED | ONLY_USED | ONLY_GPR;
> const unsigned int USED_ARG = ENABLED | ONLY_USED | ONLY_ARG;
> @@ -356,6 +357,10 @@ namespace zero_regs_flags {
> const unsigned int ALL_GPR = ENABLED | ONLY_GPR;
> const unsigned int ALL_ARG = ENABLED | ONLY_ARG;
> const unsigned int ALL = ENABLED;
> + const unsigned int LEAFY_GPR_ARG = ENABLED | LEAFY_MODE | ONLY_GPR |
> ONLY_ARG;
> + const unsigned int LEAFY_GPR = ENABLED | LEAFY_MODE | ONLY_GPR;
> + const unsigned int LEAFY_ARG = ENABLED | LEAFY_MODE | ONLY_ARG;
> + const unsigned int LEAFY = ENABLED | LEAFY_MODE;
> }
>
> /* Settings of flag_incremental_link. */
> diff --git a/gcc/function.cc b/gcc/function.cc
> index 82102ed78d7e6..7b03f9e744199 100644
> --- a/gcc/function.cc
> +++ b/gcc/function.cc
> @@ -5868,6 +5868,9 @@ gen_call_used_regs_seq (rtx_insn *ret, unsigned int
> zero_regs_type)
> only_used = zero_regs_type & ONLY_USED;
> only_arg = zero_regs_type & ONLY_ARG;
>
> + if ((zero_regs_type & LEAFY_MODE) && leaf_function_p ())
> + only_used = true;
> +
> /* For each of the hard registers, we should zero it if:
> 1. it is a call-used register;
> and 2. it is not a fixed register;
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 86b94d62b588c..93c78be8b0d9a 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -2114,6 +2114,10 @@ const struct zero_call_used_regs_opts_s
> zero_call_used_regs_opts[] =
> ZERO_CALL_USED_REGS_OPT (all-gpr, zero_regs_flags::ALL_GPR),
> ZERO_CALL_USED_REGS_OPT (all-arg, zero_regs_flags::ALL_ARG),
> ZERO_CALL_USED_REGS_OPT (all, zero_regs_flags::ALL),
> + ZERO_CALL_USED_REGS_OPT (leafy-gpr-arg, zero_regs_flags::LEAFY_GPR_ARG),
> + ZERO_CALL_USED_REGS_OPT (leafy-gpr, zero_regs_flags::LEAFY_GPR),
> + ZERO_CALL_USED_REGS_OPT (leafy-arg, zero_regs_flags::LEAFY_ARG),
> + ZERO_CALL_USED_REGS_OPT (leafy, zero_regs_flags::LEAFY),
> #undef ZERO_CALL_USED_REGS_OPT
> {NULL, 0U}
> };
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-1.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-1.c
> new file mode 100644
> index 0000000000000..c1a0c31ba1c37
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-1.c
> @@ -0,0 +1,15 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fzero-call-used-regs=leafy" } */
> +
> +volatile int result = 0;
> +int
> +__attribute__((noipa))
> +foo (int x)
> +{
> + return x;
> +}
> +int main()
> +{
> + result = foo (2);
> + return 0;
> +}
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-2.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-2.c
> new file mode 100644
> index 0000000000000..d450620c1fcfe
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-leafy-2.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#include <assert.h>
> +int result = 0;
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("leafy")))
> +foo1 (int x)
> +{
> + return (x + 1);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("leafy")))
> +foo2 (int x)
> +{
> + return foo1 (x + 2);
> +}
> diff --git a/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-1.c
> b/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-1.c
> new file mode 100644
> index 0000000000000..2277710c771b7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-1.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fzero-call-used-regs=leafy -fno-stack-protector
> -fno-PIC" } */
> +
> +void
> +foo (void)
> +{
> +}
> +
> +/* { dg-final { scan-assembler-not "vzeroall" } } */
> +/* { dg-final { scan-assembler-not "%xmm" } } */
> +/* { dg-final { scan-assembler-not "xorl\[ \t\]+%" } } */
> +/* { dg-final { scan-assembler-not "movl\[ \t\]+%" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-2.c
> b/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-2.c
> new file mode 100644
> index 0000000000000..24b85c3dbb766
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/zero-scratch-regs-leafy-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fzero-call-used-regs=leafy-gpr -fno-stack-protector
> -fno-PIC" } */
> +
> +extern int bar (int);
> +
> +void
> +foo (void)
> +{
> + int x = bar (0);
> + if (x)
> + bar (1);
> +}
> +
> +/* { dg-final { scan-assembler "xorl\[ \t\]+%eax, %eax" } } */
> +/* { dg-final { scan-assembler "xorl\[ \t\]+%edx, %edx" } } */
> +/* { dg-final { scan-assembler "xorl\[ \t\]+%ecx, %ecx" } } */
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts. Ask me about <https://stallmansupport.org>
