Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> +/* Handle a "zero_call_used_regs" attribute; arguments as in
> + struct attribute_spec.handler. */
> +
> +static tree
> +handle_zero_call_used_regs_attribute (tree *node, tree name, tree args,
> + int ARG_UNUSED (flags),
> + bool *no_add_attrs)
> +{
> + tree decl = *node;
> + tree id = TREE_VALUE (args);
> +
> + if (TREE_CODE (decl) != FUNCTION_DECL)
> + {
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "%qE attribute applies only to functions", name);
> + *no_add_attrs = true;
> + }
> +
> + if (TREE_CODE (id) != STRING_CST)
> + {
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "attribute %qE arguments not a string", name);
The existing message for this seems to be:
"%qE argument not a string"
(which seems a bit terse, but hey)
> + *no_add_attrs = true;
> + }
> +
> + bool found = false;
> + for (unsigned int i = 0; zero_call_used_regs_opts[i].name != NULL; ++i)
> + if (strcmp (TREE_STRING_POINTER (id),
> + zero_call_used_regs_opts[i].name) == 0)
> + {
> + found = true;
> + break;
> + }
> +
> + if (!found)
> + {
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "unrecognized zero_call_used_regs attribute: %qs",
> + TREE_STRING_POINTER (id));
The attribute name needs to be quoted, and it would be good if it
wasn't hard-coded into the string:
error_at (DECL_SOURCE_LOCATION (decl),
"unrecognized %qE attribute argument %qs", name,
TREE_STRING_POINTER (id));
> @@ -228,6 +228,10 @@ unsigned int flag_sanitize_coverage
> Variable
> bool dump_base_name_prefixed = false
>
> +; What subset of registers should be zeroed
Think it would be useful to add “ on function return.”.
> +Variable
> +unsigned int flag_zero_call_used_regs
> +
> ###
> Driver
>
> diff --git a/gcc/df.h b/gcc/df.h
> index 8b6ca8c..0f098d7 100644
> --- a/gcc/df.h
> +++ b/gcc/df.h
> @@ -1085,6 +1085,7 @@ extern void df_update_entry_exit_and_calls (void);
> extern bool df_hard_reg_used_p (unsigned int);
> extern unsigned int df_hard_reg_used_count (unsigned int);
> extern bool df_regs_ever_live_p (unsigned int);
> +extern bool df_epilogue_uses_p (unsigned int);
> extern void df_set_regs_ever_live (unsigned int, bool);
> extern void df_compute_regs_ever_live (bool);
> extern void df_scan_verify (void);
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index c9f7299..b011c17 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3992,6 +3992,96 @@ performing a link with relocatable output (i.e.@:
> @code{ld -r}) on them.
> A declaration to which @code{weakref} is attached and that is associated
> with a named @code{target} must be @code{static}.
>
> +@item zero_call_used_regs ("@var{choice}")
> +@cindex @code{zero_call_used_regs} function attribute
> +
> +The @code{zero_call_used_regs} attribute causes the compiler to zero
> +a subset of all call-used registers at function return according to
> +@var{choice}.
Suggest dropping “according to @var{choice}” here, since it's now
disconnected with the part that talks about what @var{choice} is.
> +This is used to increase the program security by either mitigating
s/the program security/program security/
> +Return-Oriented Programming (ROP) or preventing information leak
leakage
(FWIW, I'm not sure “mitigating ROP” is really correct usage, but I don't
have any better suggestions.)
> +through registers.
> +
> +A ``call-used'' register is a register whose contents can be changed by
> +a function call; therefore, a caller cannot assume that the register has
> +the same contents on return from the function as it had before calling
> +the function. Such registers are also called ``call-clobbered'',
> +``caller-saved'', or ``volatile''.
Reading it back, perhaps it would be better to put this whole paragraph
in a footnote immediately after the first use of “call-used registers”,
i.e.
…call-used registers@footnote{A ``call-used'' register…}…
It obviously breaks the flow when reading the raw .texi, but I think it
reads better in the final version.
> +In order to satisfy users with different security needs and control the
> +run-time overhead at the same time, GCC provides a flexible way to choose
> +the subset of the call-used registers to be zeroed.
Maybe s/GCC/the @var{choice} parameter/.
> +
> +The three basic values of @var{choice} are:
After which, I think this should be part of the previous paragraph.
> +@itemize @bullet
> +@item
> +@samp{skip} doesn't zero any call-used registers.
> +
> +@item
> +@samp{used} only zeros call-used registers that are used in the function.
> +A ``used'' register is one whose content has been set or referenced in
> +the function.
> +
> +@item
> +@samp{all} zeros all call-used registers.
> +@end itemize
> +
> +In addition to these three basic choices, it is possible to modify
> +@samp{used} or @samp{all} as follows:
> +
> +@itemize @bullet
> +@item
> +Adding @samp{-gpr} restricts the zeroing to general-purpose registers.
> +
> +@item
> +Adding @samp{-arg} restricts the zeroing to registers that can sometimes
> +be used to pass function arguments. This includes all argument registers
> +defined by the platform's calling conversion, regardless of whether the
> +function uses those registers for function arguments or not.
> +@end itemize
> +
> +The modifiers can be used individually or together. If they are used
> +together, they must appear in the order above.
> +
> +The full list of @var{choice}s is therefore:
> +
> +@itemize @bullet
> +@item
> +@samp{skip} doesn't zero any call-used register.
> +
> +@item
> +@samp{used} only zeros call-used registers that are used in the function.
> +
> +@item
> +@samp{all} zeros all call-used registers.
> +
> +@item
> +@samp{used-arg} only zeros used call-used registers that pass arguments.
> +
> +@item
> +@samp{used-gpr} only zeros used call-used general purpose registers.
> +
> +@item
> +@samp{used-gpr-arg} only zeros used call-used general purpose registers that
> +pass arguments.
> +
> +@item
> +@samp{all-gpr-arg} zeros all call-used general purpose registers that pass
> +arguments.
> +
> +@item
> +@samp{all-arg} zeros all call-used registers that pass arguments.
> +
> +@item
> +@samp{all-gpr} zeros all call-used general purpose registers.
> +@end itemize
By using a table, I meant:
@table @samp
@item skip
…
@item used
…
etc.
@end @table
Did you try that and think the output looked odd?
I think the order should be more consistent. E.g. “all” should be listed
with the other “all” options, and the ordering of foos for “used-foo”
and “all-foo” should be the same. So maybe:
- skip
- used
- used-arg
- used-gpr
- used-gpr-arg
- all
- all-arg
- all-gpr
- all-gpr-arg
> +Among this list, @samp{used-gpr-arg}, @samp{used-arg}, @samp{all-gpr-arg},
IMO s/Among/Of/ reads slightly better.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index c049932..c9e3128 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -550,7 +550,7 @@ Objective-C and Objective-C++ Dialects}.
> -funit-at-a-time -funroll-all-loops -funroll-loops @gol
> -funsafe-math-optimizations -funswitch-loops @gol
> -fipa-ra -fvariable-expansion-in-unroller -fvect-cost-model -fvpt @gol
> --fweb -fwhole-program -fwpa -fuse-linker-plugin @gol
> +-fweb -fwhole-program -fwpa -fuse-linker-plugin -fzero-call-used-regs @gol
> --param @var{name}=@var{value}
> -O -O0 -O1 -O2 -O3 -Os -Ofast -Og}
>
> @@ -12550,6 +12550,19 @@ int foo (void)
>
> Not all targets support this option.
>
> +@item -fzero-call-used-regs=@var{choice}
> +@opindex fzero-call-used-regs
> +Zero call-used registers at function return to increase the program
s/the program/program/
> +security by either mitigating Return-Oriented Programming (ROP) or
> +preventing information leak through registers.
s/leak/leakage/
> @@ -173,6 +173,9 @@ struct GTY(()) rtl_data {
> local stack. */
> unsigned int stack_alignment_estimated;
>
> + /* How to zero call-used regsiters for this routine. */
> + unsigned int zero_call_used_regs;
> +
Typo: regsiters. But I don't think we need to add this to crtl.
It's just data that's passed between pass_zero_call_used_regs::execute
and gen_call_used_regs_seq.
> /* How many NOP insns to place at each function entry by default. */
> unsigned short patch_area_size;
>
> @@ -285,6 +287,24 @@ enum sanitize_code {
> | SANITIZE_BOUNDS_STRICT
> };
>
> +/* Different settings for zeroing subset of registers. */
> +namespace zero_regs_code {
Should only be one space after “namespace”. Having “code” in the name
surprised me, think “flags” would be better.
> @@ -5815,6 +5817,101 @@ make_prologue_seq (void)
> return seq;
> }
>
> +/* Emit a sequence of insns to zero the call-used registers before RET. */
> +using namespace zero_regs_code;
Making the “using” file-wide is too much, since the file has a lot of code
unrelated to this feature. It should go in the function body instead.
> +
> +static void
> +gen_call_used_regs_seq (rtx_insn *ret)
> +{
> + bool gpr_only = true;
> + bool used_only = true;
> + bool arg_only = true;
> +
> + /* No need to zero call-used-regs in main (). */
> + if (MAIN_NAME_P (DECL_NAME (current_function_decl)))
> + return;
> +
> + /* No need to zero call-used-regs if __builtin_eh_return is called
> + since it isn't a normal function return. */
> + if (crtl->calls_eh_return)
> + return;
> +
> + /* If gpr_only is true, only zero call-used registers that are
> + general-purpose registers; if used_only is true, only zero
> + call-used registers that are used in the current function;
> + if arg_only is true, only zero call-used registers that pass
> + parameters defined by the flatform's calling conversion. */
> +
> + gpr_only = crtl->zero_call_used_regs & ONLY_GPR;
> + used_only = crtl->zero_call_used_regs & ONLY_USED;
> + arg_only = crtl->zero_call_used_regs & ONLY_ARG;
Guess it would be nice to be consistent about which side the “only”
goes on. FWIW, I don't mind which way: GPR_ONLY etc. would be
OK with me if you prefer that.
> +
> + /* 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;
> + and 3. it is not live at the return of the routine;
> + and 4. it is general registor if gpr_only is true;
> + and 5. it is used in the routine if used_only is true;
> + and 6. it is a register that passes parameter if arg_only is true. */
> +
> + /* First, prepare the data flow information. */
> + basic_block bb = BLOCK_FOR_INSN (ret);
> + auto_bitmap live_out;
> + bitmap_copy (live_out, df_get_live_out (bb));
> + df_simulate_initialize_backwards (bb, live_out);
> + df_simulate_one_insn_backwards (bb, ret, live_out);
> +
> + HARD_REG_SET selected_hardregs;
> + CLEAR_HARD_REG_SET (selected_hardregs);
> + for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> + {
> + if (!crtl->abi->clobbers_full_reg_p (regno))
> + continue;
> + if (fixed_regs[regno])
> + continue;
> + if (REGNO_REG_SET_P (live_out, regno))
> + continue;
> + if (gpr_only
> + && !TEST_HARD_REG_BIT (reg_class_contents[GENERAL_REGS], regno))
> + continue;
> + if (used_only && !df_regs_ever_live_p (regno))
> + continue;
> + if (arg_only && !FUNCTION_ARG_REGNO_P (regno))
> + continue;
> +
> + /* Now this is a register that we might want to zero. */
> + SET_HARD_REG_BIT (selected_hardregs, regno);
> + }
> +
> + if (hard_reg_set_empty_p (selected_hardregs))
> + return;
> +
> + /* Now that we have a hard register set that needs to be zeroed, pass it to
> + target to generate zeroing sequence. */
> + HARD_REG_SET zeroed_hardregs;
> + start_sequence ();
> + zeroed_hardregs = targetm.calls.zero_call_used_regs (selected_hardregs);
> + rtx_insn *seq = get_insns ();
> + end_sequence ();
> + if (seq)
> + {
> + /* Emit the memory blockage and register clobber asm volatile before
> + the whole sequence. */
> + start_sequence ();
> + expand_asm_reg_clobber_mem_blockage (zeroed_hardregs);
> + rtx_insn *seq_barrier = get_insns ();
> + end_sequence ();
> +
> + emit_insn_before (seq_barrier, ret);
> + emit_insn_before (seq, ret);
> +
> + /* Update the data flow information. */
> + crtl->must_be_zero_on_return |= zeroed_hardregs;
> + df_set_bb_dirty (EXIT_BLOCK_PTR_FOR_FN (cfun));
> + }
> +}
> +
> +
> /* Return a sequence to be used as the epilogue for the current function,
> or NULL. */
>
> @@ -6486,7 +6583,100 @@ make_pass_thread_prologue_and_epilogue (gcc::context
> *ctxt)
> {
> return new pass_thread_prologue_and_epilogue (ctxt);
> }
> -
>
> +
> +namespace {
> +
> +const pass_data pass_data_zero_call_used_regs =
> +{
> + RTL_PASS, /* type */
> + "zero_call_used_regs", /* name */
> + OPTGROUP_NONE, /* optinfo_flags */
> + TV_NONE, /* tv_id */
> + 0, /* properties_required */
> + 0, /* properties_provided */
> + 0, /* properties_destroyed */
> + 0, /* todo_flags_start */
> + 0, /* todo_flags_finish */
> +};
> +
> +class pass_zero_call_used_regs: public rtl_opt_pass
> +{
> +public:
> + pass_zero_call_used_regs (gcc::context *ctxt)
> + : rtl_opt_pass (pass_data_zero_call_used_regs, ctxt)
> + {}
> +
> + /* opt_pass methods: */
> + virtual unsigned int execute (function *);
> +
> +}; // class pass_zero_call_used_regs
> +
> +unsigned int
> +pass_zero_call_used_regs::execute (function *fun)
> +{
> + unsigned int zero_regs_type = UNSET;
> + unsigned int attr_zero_regs_type = UNSET;
> +
> + tree attr_zero_regs
> + = lookup_attribute ("zero_call_used_regs",
> + DECL_ATTRIBUTES (fun->decl));
The “= …” line should be indented by only two extra spaces. Although
in this case it fits on two lines anyway:
tree attr_zero_regs = lookup_attribute ("zero_call_used_regs",
DECL_ATTRIBUTES (fun->decl));
> + /* Get the type of zero_call_used_regs from function attribute. */
“from the function attribute” Might be worth adding “We have already
filtered out invalid attribute values.”, to explain why there's (rightly)
no failure path.
> + if (attr_zero_regs)
> + {
> + /* The TREE_VALUE of an attribute is a TREE_LIST whose TREE_VALUE
> + is the attribute argument's value. */
> + attr_zero_regs = TREE_VALUE (attr_zero_regs);
> + gcc_assert (TREE_CODE (attr_zero_regs) == TREE_LIST);
> + attr_zero_regs = TREE_VALUE (attr_zero_regs);
> + gcc_assert (TREE_CODE (attr_zero_regs) == STRING_CST);
> +
> + for (unsigned int i = 0; zero_call_used_regs_opts[i].name != NULL; ++i)
> + if (strcmp (TREE_STRING_POINTER (attr_zero_regs),
> + zero_call_used_regs_opts[i].name) == 0)
> + {
> + attr_zero_regs_type = zero_call_used_regs_opts[i].flag;
> + break;
> + }
> + }
> +
> + zero_regs_type = attr_zero_regs_type;
> +
> + if (!zero_regs_type)
> + zero_regs_type = flag_zero_call_used_regs;
Having two variables seems unnecessarily complicated. I think the
attribute code should assign directly to “zero_regs_type”.
> +
> + crtl->zero_call_used_regs = zero_regs_type;
> +
> + /* No need to zero call-used-regs when no user request is present. */
> + if (!(zero_regs_type & ENABLED))
> + return 0;
> +
> + edge_iterator ei;
> + edge e;
> +
> + /* This pass needs data flow information. */
> + df_analyze ();
> +
> + /* Iterate over the function's return instructions and insert any
> + register zeroing required by the -fzero-call-used-regs command-line
> + option or the "zero_call_used_regs" function attribute. */
> + FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds)
> + {
> + rtx_insn *insn = BB_END (e->src);
> + if (JUMP_P (insn) && ANY_RETURN_P (JUMP_LABEL (insn)))
> + gen_call_used_regs_seq (insn);
As noted above, we could just pass “zero_regs_type” here rather than
store it in crtl.
> @@ -987,6 +990,35 @@ default_function_value_regno_p (const unsigned int regno
> ATTRIBUTE_UNUSED)
> #endif
> }
>
> +/* The default hook for TARGET_ZERO_CALL_USED_REGS. */
> +
> +HARD_REG_SET
> +default_zero_call_used_regs (HARD_REG_SET need_zeroed_hardregs)
> +{
> + gcc_assert (!hard_reg_set_empty_p (need_zeroed_hardregs));
> +
> + for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; regno++)
> + if (TEST_HARD_REG_BIT (need_zeroed_hardregs, regno))
> + {
> + rtx_insn *last_insn = get_last_insn ();
> + machine_mode mode = GET_MODE (regno_reg_rtx[regno]);
> + rtx zero = CONST0_RTX (mode);
> + rtx_insn *insn = emit_move_insn (regno_reg_rtx[regno], zero);
> + if (!valid_insn_p (insn))
> + {
> + static bool issued_error;
> + if (!issued_error)
> + {
> + issued_error = true;
> + sorry ("%qs not supported on this target",
> + "fzero-call-used_regs");
"-fzero-call-used_regs"
> + }
> + delete_insns_since (last_insn);
> + }
> + }
> + return need_zeroed_hardregs;
> +}
> +
> rtx
> default_internal_arg_pointer (void)
> {
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 44ab926..e0a925f 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -160,6 +160,7 @@ extern unsigned int default_function_arg_round_boundary
> (machine_mode,
> const_tree);
> extern bool hook_bool_const_rtx_commutative_p (const_rtx, int);
> extern rtx default_function_value (const_tree, const_tree, bool);
> +extern HARD_REG_SET default_zero_call_used_regs (HARD_REG_SET);
> extern rtx default_libcall_value (machine_mode, const_rtx);
> extern bool default_function_value_regno_p (const unsigned int);
> extern rtx default_internal_arg_pointer (void);
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-1.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-1.c
> new file mode 100644
> index 0000000..2463353
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-1.c
> @@ -0,0 +1,15 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fzero-call-used-regs=skip" } */
> +
> +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-10.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c
> new file mode 100644
> index 0000000..bdaf8e7
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-10.c
> @@ -0,0 +1,92 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +#include <assert.h>
> +int result = 0;
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("skip")))
> +foo1 (int x)
> +{
> + return (x + 1);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("used-gpr-arg")))
> +foo2 (int x)
> +{
> + return (x + 2);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("used-gpr")))
> +foo3 (int x)
> +{
> + return (x + 3);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("used-arg")))
> +foo4 (int x)
> +{
> + return (x + 4);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("used")))
> +foo5 (int x)
> +{
> + return (x + 5);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("all-gpr-arg")))
> +foo6 (int x)
> +{
> + return (x + 6);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("all-gpr")))
> +foo7 (int x)
> +{
> + return (x + 7);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("all-arg")))
> +foo8 (int x)
> +{
> + return (x + 8);
> +}
> +
> +int
> +__attribute__((noipa))
> +__attribute__ ((zero_call_used_regs("all")))
> +foo9 (int x)
> +{
> + return (x + 9);
> +}
> +
> +int main()
> +{
> + result = foo1 (1);
> + result += foo2 (1);
> + result += foo3 (1);
> + result += foo4 (1);
> + result += foo5 (1);
> + result += foo6 (1);
> + result += foo7 (1);
> + result += foo8 (1);
> + result += foo9 (1);
> + assert (result == 54);
> + return 0;
> +}
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> new file mode 100644
> index 0000000..6756f57
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-11.c
> @@ -0,0 +1,92 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fzero-call-used-regs=all" } */
> +
> +#include <assert.h>
> +int result = 0;
> …
I think this should just #include "zero-scratch-regs-10.c".
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-2.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-2.c
> new file mode 100644
> index 0000000..73c3794
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fzero-call-used-regs=used-gpr-arg" } */
> +
> +volatile int result = 0;
> +int
> +__attribute__((noipa))
> +foo (int x)
> +{
> + return x;
> +}
> +int main()
> +{
> + result = foo (2);
> + return 0;
> +}
Similarly these can just #include "zero-scratch-regs-1.c". This makes
it easier to update the tests in a consistent way in future (if necessary).
> diff --git a/gcc/testsuite/c-c++-common/zero-scratch-regs-attr-usages.c
> b/gcc/testsuite/c-c++-common/zero-scratch-regs-attr-usages.c
> new file mode 100644
> index 0000000..c60e946
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/zero-scratch-regs-attr-usages.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile} */
> +/* { dg-options "-O2" } */
> +
> +int result __attribute__ ((zero_call_used_regs("all"))); /* { dg-error
> "attribute applies only to functions" } */
> +int
> +__attribute__ ((zero_call_used_regs("gpr-arg-all")))
> +foo1 (int x) /* { dg-error "unrecognized zero_call_used_regs attribute" } */
> +{
> + return (x + 1);
> +}
Could you also add a test for the string constant check? E.g. with
__attribute__ ((zero_call_used_regs(1))).
Thanks,
Richard
