Re: [PATCH][ARM] Fix PR target/69403: Bug in thumb2_ior_scc_strict_it pattern
Hi Kyrill, the patched gcc generates correct asm for me for the test case. (I'll then build the whole system to see if other it-block related bugs are gone too.) One short question, the newly generated RTL for x = x |(a) will be orr %0, %1, #1; it ; mov%D2\\t%0, %1 (b) The cond in (a) should be the reverse of cond in(b), right? Thanks for your quick fix. Han On Thu, Jan 21, 2016 at 9:51 AM, Kyrill Tkachov wrote: > Hi all, > > In this wrong-code PR the pattern for performing > x = x | for -mrestrict-it is buggy and ends up writing 1 to the > result register rather than orring it. > > The offending pattern is *thumb2_ior_scc_strict_it. > My proposed solution is to rewrite it as a splitter, remove the > alternative for the case where operands[1] and 0 are the same reg > that emits the bogus: > it ; mov%0, #1; it ; orr %0, %1 > > to emit the RTL equivalent to: > orr %0, %1, #1; it ; mov%D2\\t%0, %1 > while marking operand 0 as an earlyclobber operand so that it doesn't > get assigned the same register as operand 1. > > This way we avoid the wrong-code, make the sequence better (by eliminating > the move of #1 into a register > and relaxing the constraints from 'l' to 'r' since only the register move > has to be conditional). > and still stay within the rules for arm_restrict_it. > > Bootstrapped and tested on arm-none-linux-gnueabihf configured > --with-arch=armv8-a and --with-mode=thumb. > > Ok for trunk, GCC 5 and 4.9? > > Han, can you please try this out to see if it solves the problem on your end > as well? > > Thanks, > Kyrill > > 2016-01-21 Kyrylo Tkachov > > PR target/69403 > * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to > define_insn_and_split. Ensure operands[1] and operands[0] do not > get assigned the same register. > > 2016-01-21 Kyrylo Tkachov > > PR target/69403 > * gcc.c-torture/execute/pr69403.c: New test. -- Han Shen | Software Engineer | shen...@google.com | +1-650-440-3330
[google gcc-4_9]: Backport trunk:r232727 fix for PR/69403.
Backport trunk:r232727 fix for PR/69403 - wrong
thumb2_ior_scc_strict_it insn pattern.
Note this only affect armv7-a tuned for armv8 arch, tested / booted
affected ChromeOS book.
Ok for google/gcc-4_9 branch?
--
Han Shen
Index: gcc/ChangeLog
===
--- gcc/ChangeLog (revision 232940)
+++ gcc/ChangeLog (working copy)
@@ -1,3 +1,10 @@
+2016-01-22 Kyrylo Tkachov
+
+ PR target/69403
+ * config/arm/thumb2.md (*thumb2_ior_scc_strict_it): Convert to
+ define_insn_and_split. Ensure operands[1] and operands[0] do not
+ get assigned the same register.
+
2015-03-26 Bill Schmidt
Backport of r214242, r214254, and bug fix patches from mainline
Index: gcc/config/arm/thumb2.md
===
--- gcc/config/arm/thumb2.md (revision 232940)
+++ gcc/config/arm/thumb2.md (working copy)
@@ -642,15 +642,27 @@
(set_attr "type" "multiple")]
)
-(define_insn "*thumb2_ior_scc_strict_it"
- [(set (match_operand:SI 0 "s_register_operand" "=l,l")
+(define_insn_and_split "*thumb2_ior_scc_strict_it"
+ [(set (match_operand:SI 0 "s_register_operand" "=&r")
(ior:SI (match_operator:SI 2 "arm_comparison_operator"
[(match_operand 3 "cc_register" "") (const_int 0)])
- (match_operand:SI 1 "s_register_operand" "0,?l")))]
+ (match_operand:SI 1 "s_register_operand" "r")))]
"TARGET_THUMB2 && arm_restrict_it"
- "@
- it\\t%d2\;mov%d2\\t%0, #1\;it\\t%d2\;orr%d2\\t%0, %1
- mov\\t%0, #1\;orr\\t%0, %1\;it\\t%D2\;mov%D2\\t%0, %1"
+ "#" ; orr\\t%0, %1, #1\;it\\t%D2\;mov%D2\\t%0, %1
+ "&& reload_completed"
+ [(set (match_dup 0) (ior:SI (match_dup 1) (const_int 1)))
+ (cond_exec (match_dup 4)
+ (set (match_dup 0) (match_dup 1)))]
+ {
+machine_mode mode = GET_MODE (operands[3]);
+rtx_code rc = GET_CODE (operands[2]);
+
+if (mode == CCFPmode || mode == CCFPEmode)
+ rc = reverse_condition_maybe_unordered (rc);
+else
+ rc = reverse_condition (rc);
+operands[4] = gen_rtx_fmt_ee (rc, VOIDmode, operands[3], const0_rtx);
+ }
[(set_attr "conds" "use")
(set_attr "length" "8")
(set_attr "type" "multiple")]
Index: gcc/testsuite/ChangeLog
===
--- gcc/testsuite/ChangeLog (revision 232940)
+++ gcc/testsuite/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2016-01-22 Kyrylo Tkachov
+
+ PR target/69403
+ * gcc.c-torture/execute/pr69403.c: New test.
+
2015-03-26 Bill Schmidt
Backport r214254 and related tests from mainline
Index: .
===
--- . (revision 232940)
+++ . (working copy)
Property changes on: .
___
Modified: svn:mergeinfo
Merged /trunk:r232727
Re: [PATCH] Add a new option "-fstack-protector-strong"
smashing attack needs a frame
address to perform the attack. The frame address of function F can be
from one of the following:
- (A) an address taking operator (&) on any local variables of F
- (B) any address computation based on (A)
- (C) any address casting operation from either (A) or (B)
- (D) the name of a local array of F
- (E) the address from calling “alloca”
Function F is said to be vulnerable if its frame address is
exposed via (A) ~ (E).
What about struct-returning functions? Internally, an address is passed to
the called function. Would they trigger this? What about the this pointer
in C++ code?
Yes for 'this' pointer. 'this' pointer of a local class instance is
regarded as a reference to stack address, thus it is protected. For
example -
int
foo1 ()
{
A a;
a.method (); // a.method() exposes 'this', so this function is
protected.
return a.state;
}
No for 'struct-returning' functions. But I regard this not an issue ---
at the programming level, there is no way to get one's hand on the
address of a returned structure ---
struct Node foo();
struct Node *p = &foo(); // compiler error - lvalue required as
unary '&' operand.
If write this way -
struct Node p = foo();
struct Node *q = &p;
The protection would be triggered.
--
Florian Weimer / Red Hat Product Security Team
ChangeLog and patch below --
gcc/ChangeLog
2013-04-16 Han Shen
* cfgexpand.c (record_or_union_type_has_array_p): Helper function
to check if a record or union contains an array field.
(expand_used_vars): Add logic handling '-fstack-protector-strong'.
* common.opt (fstack-protector-all): New option.
* doc/cpp.texi (__SSP_STRONG__): New builtin "__SSP_STRONG__".
* doc/invoke.texi (Optimization Options): Document
"-fstack-protector-strong".
* gcc.c (LINK_SSP_SPEC): Add 'fstack-protector-strong'.
gcc/c-family/ChangeLog
2013-04-16 Han Shen
* c-cppbuiltin.c (c_cpp_builtins): Added "__SSP_STRONG__=3".
gcc/testsuite/ChangeLog
2013-04-16 Han Shen
Test cases for '-fstack-protector-strong'.
* gcc.dg/fstack-protector-strong.c: New.
* g++.dg/fstack-protector-strong.C: New.
diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index 3e210d9..0059626 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -888,6 +888,8 @@ c_cpp_builtins (cpp_reader *pfile)
/* Make the choice of the stack protector runtime visible to source
code.
The macro names and values here were chosen for compatibility with an
earlier implementation, i.e. ProPolice. */
+ if (flag_stack_protect == 3)
+cpp_define (pfile, "__SSP_STRONG__=3");
if (flag_stack_protect == 2)
cpp_define (pfile, "__SSP_ALL__=2");
else if (flag_stack_protect == 1)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index a651d8c..b370cdb 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1291,6 +1291,10 @@ clear_tree_used (tree block)
clear_tree_used (t);
}
+#define SPCT_FLAG_ALL 2
+#define SPCT_FLAG_DEFAULT 1
+#define SPCT_FLAG_STRONG 3
+
/* Examine TYPE and determine a bit mask of the following features. */
#define SPCT_HAS_LARGE_CHAR_ARRAY 1
@@ -1360,7 +1364,8 @@ stack_protect_decl_phase (tree decl)
if (bits & SPCT_HAS_SMALL_CHAR_ARRAY)
has_short_buffer = true;
- if (flag_stack_protect == 2)
+ if (flag_stack_protect == SPCT_FLAG_ALL ||
+ flag_stack_protect == SPCT_FLAG_STRONG)
{
if ((bits & (SPCT_HAS_SMALL_CHAR_ARRAY | SPCT_HAS_LARGE_CHAR_ARRAY))
&& !(bits & SPCT_HAS_AGGREGATE))
@@ -1514,6 +1519,27 @@ estimated_stack_frame_size (struct cgraph_node *node)
return size;
}
+/* Helper routine to check if a record or union contains an array field. */
+
+static int
+record_or_union_type_has_array_p (const_tree tree_type)
+{
+ tree fields = TYPE_FIELDS (tree_type);
+ tree f;
+
+ for (f = fields; f; f = DECL_CHAIN (f))
+if (TREE_CODE (f) == FIELD_DECL)
+ {
+ tree field_type = TREE_TYPE (f);
+ if (RECORD_OR_UNION_TYPE_P (field_type)
+ && record_or_union_type_has_array_p (field_type))
+ return 1;
+ if (TREE_CODE (field_type) == ARRAY_TYPE)
+ return 1;
+ }
+ return 0;
+}
+
/* Expand all variables used in the function. */
static rtx
@@ -1525,6 +1551,7 @@ expand_used_vars (void)
struct pointer_map_t *ssa_name_decls;
unsigned i;
unsigned len;
+ bool gen_stack_protect_signal = false;
/* Compute the phase of the stack frame for this function. */
{
@@ -1576,6 +1603,24 @@ expand_used_vars (void)
}
pointer_map_destroy (ssa_name_decls);
+ if (flag_stack_protect == SPCT_FLAG_STRONG)
+FOR_EACH_LOCAL_DEC
[trunk] RFS: translate built-in include paths for sysroot (issue5394041)
2011-11-15 Han Shen
* gcc/Makefile.in:
* gcc/configure:
* gcc/cppdefault.c:
diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index ae4f4da..0a05783 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -615,6 +615,7 @@ gcc_tooldir = @gcc_tooldir@
build_tooldir = $(exec_prefix)/$(target_noncanonical)
# Directory in which the compiler finds target-independent g++ includes.
gcc_gxx_include_dir = @gcc_gxx_include_dir@
+gcc_gxx_include_dir_add_sysroot = @gcc_gxx_include_dir_add_sysroot@
# Directory to search for site-specific includes.
local_includedir = $(local_prefix)/include
includedir = $(prefix)/include
@@ -3979,6 +3980,7 @@ PREPROCESSOR_DEFINES = \
-DGCC_INCLUDE_DIR=\"$(libsubdir)/include\" \
-DFIXED_INCLUDE_DIR=\"$(libsubdir)/include-fixed\" \
-DGPLUSPLUS_INCLUDE_DIR=\"$(gcc_gxx_include_dir)\" \
+ -DGPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT=$(gcc_gxx_include_dir_add_sysroot) \
-DGPLUSPLUS_TOOL_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/$(target_noncanonical)\" \
-DGPLUSPLUS_BACKWARD_INCLUDE_DIR=\"$(gcc_gxx_include_dir)/backward\" \
-DLOCAL_INCLUDE_DIR=\"$(local_includedir)\" \
diff --git a/gcc/configure b/gcc/configure
index 99334ce..364d8c2 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -638,6 +638,7 @@ host_xm_include_list
host_xm_file_list
host_exeext
gcc_gxx_include_dir
+gcc_gxx_include_dir_add_sysroot
gcc_config_arguments
float_h_file
extra_programs
@@ -3291,12 +3292,20 @@ gcc_gxx_include_dir=
# Specify the g++ header file directory
# Check whether --with-gxx-include-dir was given.
+gcc_gxx_include_dir_add_sysroot=0
if test "${with_gxx_include_dir+set}" = set; then :
withval=$with_gxx_include_dir; case "${withval}" in
yes) as_fn_error "bad value ${withval} given for g++ include directory"
"$LINENO" 5 ;;
no);;
*) gcc_gxx_include_dir=$with_gxx_include_dir ;;
esac
+ if test "${with_sysroot+set}" = set; then :
+gcc_gxx_without_sysroot=`expr "${gcc_gxx_include_dir}" :
"${with_sysroot}"'\(.*\)'`
+if test "${gcc_gxx_without_sysroot}"; then :
+ gcc_gxx_include_dir="${gcc_gxx_without_sysroot}"
+ gcc_gxx_include_dir_add_sysroot=1
+fi
+ fi
fi
diff --git a/gcc/cppdefault.c b/gcc/cppdefault.c
index 099899a..e8341d5 100644
--- a/gcc/cppdefault.c
+++ b/gcc/cppdefault.c
@@ -44,15 +44,15 @@ const struct default_include cpp_include_defaults[]
= {
#ifdef GPLUSPLUS_INCLUDE_DIR
/* Pick up GNU C++ generic include files. */
-{ GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
+{ GPLUSPLUS_INCLUDE_DIR, "G++", 1, 1, GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 0
},
#endif
#ifdef GPLUSPLUS_TOOL_INCLUDE_DIR
/* Pick up GNU C++ target-dependent include files. */
-{ GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1, 0, 1 },
+{ GPLUSPLUS_TOOL_INCLUDE_DIR, "G++", 1, 1,
GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 1 },
#endif
#ifdef GPLUSPLUS_BACKWARD_INCLUDE_DIR
/* Pick up GNU C++ backward and deprecated include files. */
-{ GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1, 0, 0 },
+{ GPLUSPLUS_BACKWARD_INCLUDE_DIR, "G++", 1, 1,
GPLUSPLUS_INCLUDE_DIR_ADD_SYSROOT, 0 },
#endif
#ifdef GCC_INCLUDE_DIR
/* This is the dir for gcc's private headers. */
--
This patch is available for review at http://codereview.appspot.com/5394041
[4.7][google] Adding a new option -fstack-protector-strong. (issue5461043)
Hi, this patch provides a new stack protection option -
"fstack-protector-strong".
Background - some times stack-protector is too-simple while stack-protector-all
over-kills, for example, to build one of our core systems, we forcibly add
"-fstack-protector-all" to all compile commands, which brings big performance
penalty (due to extra stack guard/check insns on function prologue and
epilogue) on both atom and arm. To use "-fstack-protector" is just regarded as
not secure enough (only "protects" <2% functions) by the system secure team. So
I'd like to add the option "-fstack-protector-strong", that hits the balance
between "-fstack-protector" and "-fstack-protector-all".
Benefit - gain big performance while sacrificing little security (for scenarios
using -fstack-protector-all)
Status - implemented internally, to be up-streamed or merged to google branch
only.
Detail -
https://docs.google.com/a/google.com/document/d/1xXBH6rRZue4f296vGt9YQcuLVQHeE516stHwt8M9xyU/edit?hl=en_US
Tested - manually, built chrome browser using the modified compiler with
"-fstack-protector-strong".
=
gcc/ChangeLog:
* cfgexpand.c (expand_used_vars): Add logic handling
stack-protector-strong.
(is_local_address_taken): Internal function that returns true when
gimple contains
an address taken on function local variables.
(record_or_union_type_has_array): New, tests if a record or union type
contains an array.
* common.opt (fstack-protector-all): New option.
gcc/testsuite/ChangeLog
* gcc.dg/fstack-protector-strong.c: New.
==
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 8684721..1d9df87 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1507,6 +1507,50 @@ estimated_stack_frame_size (struct cgraph_node *node)
return size;
}
+static int is_local_address_taken(tree t) {
+ if (t && TREE_CODE(t) == ADDR_EXPR) {
+int i;
+tree local_var;
+tree v = TREE_OPERAND(t, 0);
+switch (TREE_CODE(v)) {
+case MEM_REF:
+ for (i = 0; i < TREE_OPERAND_LENGTH(v); ++i)
+if (is_local_address_taken(TREE_OPERAND(v, i)))
+ return 1;
+ return 0;
+case COMPONENT_REF:
+ while (v && TREE_CODE(v) == COMPONENT_REF)
+v = TREE_OPERAND(v, 0);
+ break;
+case VAR_DECL:
+default:
+ ;
+}
+if (v && TREE_CODE(v) == VAR_DECL && !is_global_var(v)) {
+ FOR_EACH_VEC_ELT(tree, cfun->local_decls, i, local_var) {
+if (local_var == v)
+ return 1;
+ }
+}
+ }
+ return 0;
+}
+
+static int record_or_union_type_has_array(tree tree_type) {
+ tree fields = TYPE_FIELDS(tree_type);
+ tree f;
+ for (f = fields; f; f = DECL_CHAIN (f)) {
+if (TREE_CODE(f) == FIELD_DECL) {
+ tree field_type = TREE_TYPE(f);
+ if (RECORD_OR_UNION_TYPE_P(field_type))
+return record_or_union_type_has_array(field_type);
+ if (TREE_CODE(field_type) == ARRAY_TYPE)
+return 1;
+}
+ }
+ return 0;
+}
+
/* Expand all variables used in the function. */
static void
@@ -1516,6 +1560,8 @@ expand_used_vars (void)
VEC(tree,heap) *maybe_local_decls = NULL;
unsigned i;
unsigned len;
+ int gen_stack_protect_signal = 0;
+ basic_block bb;
/* Compute the phase of the stack frame for this function. */
{
@@ -1548,6 +1594,63 @@ expand_used_vars (void)
}
}
+ /* Examine each basic block for address taking of local variables. */
+ FOR_EACH_BB(bb) {
+gimple_stmt_iterator si;
+/* Scanning phis. */
+for (si = gsi_start_phis(bb); !gsi_end_p(si); gsi_next(&si)) {
+ gimple phi_stmt = gsi_stmt(si);
+ unsigned int i;
+ for (i = 0; i < gimple_phi_num_args(phi_stmt); ++i)
+if (is_local_address_taken(gimple_phi_arg(phi_stmt, i)->def))
+ ++gen_stack_protect_signal;
+}
+/* Scanning assignments and calls. */
+for (si = gsi_start_bb(bb); !gen_stack_protect_signal && !gsi_end_p(si);
+ gsi_next(&si)) {
+ gimple stmt = gsi_stmt (si);
+ if (is_gimple_assign(stmt)) {
+switch(gimple_assign_rhs_class(stmt)) {
+case GIMPLE_TERNARY_RHS:
+ if (is_local_address_taken(gimple_assign_rhs3(stmt))) {
+++gen_stack_protect_signal;
+break;
+ }
+ /* Otherwise, fall through. */
+case GIMPLE_BINARY_RHS:
+ if (is_local_address_taken(gimple_assign_rhs2(stmt))) {
+++gen_stack_protect_signal;
+break;
+ }
+case GIMPLE_SINGLE_RHS:
+case GIMPLE_UNARY_RHS:
+ if (is_local_address_taken(gimple_assign_rhs1(stmt)))
+++gen_stack_protect_signal;
+ break;
+case GIMPLE_INVALID_RHS:
+ break;
+}
+ }
+
+ if (!gen_stack_protect_signal && is_gimple_call(stmt)) {
+int ii, num_arg = gimple_call_num_args(stmt);
+for (ii = 0; !gen_stack_protect_signal && ii < num_arg; ++ii)
+
