Re: [PATCH] Add security_sensitive attribute to clean function stack and regs.
On 03/22/2016 03:15 PM, Marcos Díaz wrote:
the attached patch adds a new attribute 'security_sensitive' for functions.
The idea was discussed in PR middle-end/69976.
The first question would be: I see several submissions from folks
@tallertechnologies.com. Do you and your employer have copyright
assignments on file with the FSF?
The patch violates gcc coding guidelines in many ways. I'll point out
some issues, but in general you are expected to familiarize yourself
with the requirements first. Please review the entire patch yourself for
such issues.
+bool ix86_sec_sensitive_attr_p(void)
Break line after return types.
{
- return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI;
+ return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI
+ && !ix86_sec_sensitive_attr_p();
Wrap multi-line expressions in parentheses to get correct formatting
from the editor.
+ return (regno == BX_REG)
+|| (regno == BP_REG)
+|| (regno == SP_REG)
+|| (regno == R12_REG)
+|| (regno == R13_REG)
+|| (regno == R14_REG)
+|| (regno == R15_REG);
Same here but also remove unnecessary parentheses (in many places in
this patch).
+}
+
+/* Returns true iff regno is an integer register.*/
+static inline bool is_integer_reg(unsigned int regno)
Space before open parens.
+ rtx ret = ix86_function_value
+ (TREE_TYPE (DECL_RESULT (cfun->decl)), cfun->decl, true);
Probably better to split this line after one of the commas.
+ // Is parallel
Use C style comments consistently.
+/* Adds to str in pos position the neame of the register regno.*/
Always two spaces after a period, including at the end of a comment.
In general it would be ideal if this functionality was not x86 specific.
I could imagine that thread_prologue_and_epilogue_insns could do this in
a machine-independent fashion.
Bernd
Re: [PATCH] Add security_sensitive attribute to clean function stack and regs.
On Mon, Mar 28, 2016 at 3:05 PM, Marcos Díaz wrote: > (Ping and changelog fix) > > On Tue, Mar 22, 2016 at 11:15 AM, Marcos Díaz > wrote: >> Hi, >>the attached patch adds a new attribute 'security_sensitive' for >> functions. >> The idea was discussed in PR middle-end/69976. >> This attribute makes gcc to emit clean up code at the function's epilogue. >> This clean-up code cleans the stack used by this function and that isn't >> needed anymore. It also cleans used registers. It only works in x86_64. >> Please, review the discussion here: >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69976 >> since we had some doubts with the implementation. >> >> We also added some test-cases and ran all tests in x86_64. >> We think this isn't a bug-fix but a new feature. >> >> Changelog >> 2016-03-21 Marcos Diaz >>Andres Tiraboschi >> >> PR tree-optimization/69820 > This line should've been PR middle-end/69976 >> * config/i386/i386-protos.h: Add ix86_clear_regs_emit and >> ix86_sec_sensitive_attr_p >> * config/i386/i386.c: (ix86_sec_sensitive_attr_p): New function >> (ix86_using_red_zone): now take into account if the function has the new >> attribute. >> (is_preserved_reg): New function. >> (is_integer_reg): New function. >> (is_used_as_ret): New function. >> (reg_to_string): New function. >> (clear_reg_emit): New function. >> (ix86_clear_regs_emit): New function. >> (ix86_expand_epilogue): Added code to emit clean up code only when >> security_sensitive attribute is set. >> (ix86_handle_security_sensitive_attribute): New function. >> (ix86_attribute_table): Added new attribute. >> * config/i386/i386.md: (UNSPECV_CLRSTACK): New unspecv. >> (UNSPECV_CLRREGS): New unspecv. >> (return): Conditionally emit cleaning regs code. >> (simple_return): Likewise >> (clear_regs): New insn. >> (clear_stack): New insn. >> * doc/extend.texi: Added description for new security_sensitive attribute. > > Changelog > 2016-03-21 Marcos Diaz >Andres Tiraboschi > > PR middle-end/69976 > * config/i386/i386-protos.h: Add ix86_clear_regs_emit and > ix86_sec_sensitive_attr_p > * config/i386/i386.c: (ix86_sec_sensitive_attr_p): New function > (ix86_using_red_zone): now take into account if the function has the new > attribute. > (is_preserved_reg): New function. > (is_integer_reg): New function. > (is_used_as_ret): New function. > (reg_to_string): New function. > (clear_reg_emit): New function. > (ix86_clear_regs_emit): New function. > (ix86_expand_epilogue): Added code to emit clean up code only when > security_sensitive attribute is set. > (ix86_handle_security_sensitive_attribute): New function. > (ix86_attribute_table): Added new attribute. > * config/i386/i386.md: (UNSPECV_CLRSTACK): New unspecv. > (UNSPECV_CLRREGS): New unspecv. > (return): Conditionally emit cleaning regs code. > (simple_return): Likewise > (clear_regs): New insn. > (clear_stack): New insn. > * doc/extend.texi: Added description for new security_sensitive attribute. ping^2
Re: [PATCH] Add security_sensitive attribute to clean function stack and regs.
(Ping and changelog fix) On Tue, Mar 22, 2016 at 11:15 AM, Marcos Díaz wrote: > Hi, >the attached patch adds a new attribute 'security_sensitive' for functions. > The idea was discussed in PR middle-end/69976. > This attribute makes gcc to emit clean up code at the function's epilogue. > This clean-up code cleans the stack used by this function and that isn't > needed anymore. It also cleans used registers. It only works in x86_64. > Please, review the discussion here: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69976 > since we had some doubts with the implementation. > > We also added some test-cases and ran all tests in x86_64. > We think this isn't a bug-fix but a new feature. > > Changelog > 2016-03-21 Marcos Diaz >Andres Tiraboschi > > PR tree-optimization/69820 This line should've been PR middle-end/69976 > * config/i386/i386-protos.h: Add ix86_clear_regs_emit and > ix86_sec_sensitive_attr_p > * config/i386/i386.c: (ix86_sec_sensitive_attr_p): New function > (ix86_using_red_zone): now take into account if the function has the new > attribute. > (is_preserved_reg): New function. > (is_integer_reg): New function. > (is_used_as_ret): New function. > (reg_to_string): New function. > (clear_reg_emit): New function. > (ix86_clear_regs_emit): New function. > (ix86_expand_epilogue): Added code to emit clean up code only when > security_sensitive attribute is set. > (ix86_handle_security_sensitive_attribute): New function. > (ix86_attribute_table): Added new attribute. > * config/i386/i386.md: (UNSPECV_CLRSTACK): New unspecv. > (UNSPECV_CLRREGS): New unspecv. > (return): Conditionally emit cleaning regs code. > (simple_return): Likewise > (clear_regs): New insn. > (clear_stack): New insn. > * doc/extend.texi: Added description for new security_sensitive attribute. Changelog 2016-03-21 Marcos Diaz Andres Tiraboschi PR middle-end/69976 * config/i386/i386-protos.h: Add ix86_clear_regs_emit and ix86_sec_sensitive_attr_p * config/i386/i386.c: (ix86_sec_sensitive_attr_p): New function (ix86_using_red_zone): now take into account if the function has the new attribute. (is_preserved_reg): New function. (is_integer_reg): New function. (is_used_as_ret): New function. (reg_to_string): New function. (clear_reg_emit): New function. (ix86_clear_regs_emit): New function. (ix86_expand_epilogue): Added code to emit clean up code only when security_sensitive attribute is set. (ix86_handle_security_sensitive_attribute): New function. (ix86_attribute_table): Added new attribute. * config/i386/i386.md: (UNSPECV_CLRSTACK): New unspecv. (UNSPECV_CLRREGS): New unspecv. (return): Conditionally emit cleaning regs code. (simple_return): Likewise (clear_regs): New insn. (clear_stack): New insn. * doc/extend.texi: Added description for new security_sensitive attribute.
[PATCH] Add security_sensitive attribute to clean function stack and regs.
Hi,
the attached patch adds a new attribute 'security_sensitive' for functions.
The idea was discussed in PR middle-end/69976.
This attribute makes gcc to emit clean up code at the function's epilogue.
This clean-up code cleans the stack used by this function and that isn't
needed anymore. It also cleans used registers. It only works in x86_64.
Please, review the discussion here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69976
since we had some doubts with the implementation.
We also added some test-cases and ran all tests in x86_64.
We think this isn't a bug-fix but a new feature.
Changelog
2016-03-21 Marcos Diaz
Andres Tiraboschi
PR tree-optimization/69820
* config/i386/i386-protos.h: Add ix86_clear_regs_emit and
ix86_sec_sensitive_attr_p
* config/i386/i386.c: (ix86_sec_sensitive_attr_p): New function
(ix86_using_red_zone): now take into account if the function has the new
attribute.
(is_preserved_reg): New function.
(is_integer_reg): New function.
(is_used_as_ret): New function.
(reg_to_string): New function.
(clear_reg_emit): New function.
(ix86_clear_regs_emit): New function.
(ix86_expand_epilogue): Added code to emit clean up code only when
security_sensitive attribute is set.
(ix86_handle_security_sensitive_attribute): New function.
(ix86_attribute_table): Added new attribute.
* config/i386/i386.md: (UNSPECV_CLRSTACK): New unspecv.
(UNSPECV_CLRREGS): New unspecv.
(return): Conditionally emit cleaning regs code.
(simple_return): Likewise
(clear_regs): New insn.
(clear_stack): New insn.
* doc/extend.texi: Added description for new security_sensitive attribute.
diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h
index e4652f3..b69aa59 100644
--- a/gcc/config/i386/i386-protos.h
+++ b/gcc/config/i386/i386-protos.h
@@ -32,6 +32,8 @@ extern HOST_WIDE_INT ix86_initial_elimination_offset (int,
int);
extern void ix86_expand_prologue (void);
extern void ix86_maybe_emit_epilogue_vzeroupper (void);
extern void ix86_expand_epilogue (int);
+extern void ix86_clear_regs_emit (rtx*);
+extern bool ix86_sec_sensitive_attr_p(void);
extern void ix86_expand_split_stack_prologue (void);
extern void ix86_output_addr_vec_elt (FILE *, int);
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 3d8dbc4..7c58d6d 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -3690,12 +3690,19 @@ make_pass_stv (gcc::context *ctxt)
return new pass_stv (ctxt);
}
+bool ix86_sec_sensitive_attr_p(void)
+{
+ return lookup_attribute ("security_sensitive", DECL_ATTRIBUTES (cfun->decl));
+}
+
+
/* Return true if a red-zone is in use. */
static inline bool
ix86_using_red_zone (void)
{
- return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI;
+ return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI
+ && !ix86_sec_sensitive_attr_p();
}
/* Return a string that documents the current -m options. The caller is
@@ -13325,6 +13332,144 @@ ix86_emit_restore_sse_regs_using_mov (HOST_WIDE_INT
cfa_offset,
}
}
+/* Returns true iff regno is a register that must be preserved in a function.*/
+static inline bool
+is_preserved_reg(const unsigned int regno)
+{
+ return (regno == BX_REG)
+|| (regno == BP_REG)
+|| (regno == SP_REG)
+|| (regno == R12_REG)
+|| (regno == R13_REG)
+|| (regno == R14_REG)
+|| (regno == R15_REG);
+}
+
+/* Returns true iff regno is an integer register.*/
+static inline bool is_integer_reg(unsigned int regno)
+{
+ return (regno <= 7u) ||
+ ((FIRST_REX_INT_REG <= regno) && (regno <= LAST_REX_INT_REG));
+}
+
+/* Returns true iff regno is used to return in the current function.*/
+static inline bool
+is_used_as_ret(const unsigned int reg_number)
+{
+ bool is_ret = ix86_function_value_regno_p(reg_number);
+ if (is_ret)
+{
+ rtx ret = ix86_function_value
+ (TREE_TYPE (DECL_RESULT (cfun->decl)), cfun->decl, true);
+ if ((REG_P(ret))
+ && (is_integer_reg(REGNO(ret)))
+ && (is_integer_reg(reg_number))
+)
+ {
+ if ((GET_MODE(ret) == TImode) || (GET_MODE(ret) == CDImode))
+ is_ret = (reg_number == AX_REG) || (reg_number == DX_REG);
+ else
+ is_ret = (reg_number == AX_REG);
+ }
+ else if ((REG_P(ret)))
+ is_ret = REGNO(ret) == reg_number;
+ else
+ {
+ // Is parallel
+ const unsigned int len = XVECLEN(ret, 0);
+ unsigned int j = 0u;
+ bool found = false;
+ while (!found && j < len)
+ {
+ const rtx explist = XVECEXP(ret, 0, j);
+ const rtx ret_reg = XEXP(explist, 0);
+ found = REGNO(ret_reg) == reg_number;
+ ++j;
+ }
+ is_ret = found;
+ }
+
+}
+ return is_ret;
+}
+
+// Make this big enough to store any instruction
+#define MAX_CLEAR_STRING_SIZE 50
+
+/* Adds to str in pos position the neame of the register regno.*/
+static size_t
+reg_to_string(const unsigned in
