Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On Fri, Jun 30, 2017 at 11:21:48AM +0200, Martin Liška wrote:
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/asan/function-argument-3.C
> @@ -0,0 +1,27 @@
> +// { dg-do run }
> +// { dg-shouldfail "asan" }
> +
> +typedef int v4si __attribute__ ((vector_size (16)));
> +
> +static __attribute__ ((noinline)) int
> +goo (v4si *a)
> +{
> + return (*(volatile v4si *) (a + 1))[2];
> +}
> +
> +__attribute__ ((noinline)) int
> +foo (v4si arg)
> +{
> + return goo ();
> +}
This test fails on i686-linux when -msse2 is not on by default,
fixed thusly, committed as obvious:
2017-07-04 Jakub Jelinek
* g++.dg/asan/function-argument-3.C: Add -Wno-psabi to additional
options.
--- gcc/testsuite/g++.dg/asan/function-argument-3.C.jj 2017-07-03
19:03:25.0 +0200
+++ gcc/testsuite/g++.dg/asan/function-argument-3.C 2017-07-04
10:42:18.558716171 +0200
@@ -1,5 +1,6 @@
// { dg-do run }
// { dg-shouldfail "asan" }
+// { dg-additional-options "-Wno-psabi" }
typedef int v4si __attribute__ ((vector_size (16)));
Jakub
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On Fri, Jun 30, 2017 at 11:21:48AM +0200, Martin Liška wrote:
> @@ -858,6 +864,132 @@ sanitize_asan_mark_poison (void)
> }
> }
>
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> + that is it's DECL_VALUE_EXPR. */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> + if (TREE_CODE (*op) == PARM_DECL && DECL_HAS_VALUE_EXPR_P (*op))
> +{
> + *op = DECL_VALUE_EXPR (*op);
> + *walk_subtrees = 0;
> +}
If you are going to rely just on DECL_HAS_VALUE_EXPR_P here
> + for (tree arg = DECL_ARGUMENTS (current_function_decl);
> + arg; arg = DECL_CHAIN (arg))
> +{
I think you should gcc_assert here that !DECL_HAS_VALUE_EXPR_P (arg) here.
If that ever fails, another possibility would be to temporarily clear those
flags and remember it and then set it again after the walk_*. The question
would be what to do with arguments that already have DECL_VALUE_EXPR, are
addressable and you want to rewrite them.
> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> + {
> + TREE_ADDRESSABLE (arg) = 0;
> + /* The parameter is no longer addressable. */
> + tree type = TREE_TYPE (arg);
> + has_any_addressable_param = true;
Otherwise LGTM.
Jakub
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
0x943e01 execute
../../gcc/cfgexpand.c:6248
>
>> + /* Replace all usages of PARM_DECLs with the newly
>> + created variable VAR. */
>> + basic_block bb;
>> + FOR_EACH_BB_FN (bb, fun)
>> +{
>> + gimple_stmt_iterator gsi;
>> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next ())
>> +{
>> + gimple *stmt = gsi_stmt (gsi);
>> + gimple_stmt_iterator it = gsi_for_stmt (stmt);
>> + walk_gimple_stmt (, NULL, rewrite_usage_of_param, NULL);
>> +}
>> + for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next ())
>> +{
>> + gphi *phi = dyn_cast (gsi_stmt (gsi));
>> + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
>> +{
>> + hash_set visited_nodes;
>> + walk_tree (gimple_phi_arg_def_ptr (phi, i),
>> + rewrite_usage_of_param, NULL, _nodes);
>> +}
>
> Doesn't walk_gimple_stmt on the PHI handle this?
No, I see following in asan bootstrap:
^[2^[1^[2^[2../../gcc/c/c-decl.c: In function ‘tree_node* grokfield(location_t,
c_declarator*, c_declspecs*, tree, tree_node**)’:
../../gcc/c/c-decl.c:7525:1: error: address taken, but ADDRESSABLE bit not set
grokfield (location_t loc,
^
PHI argument
for PHI node
iftmp.2774_27 = PHI <(16), 0B(61), (54)>
during GIMPLE pass: sanopt
../../gcc/c/c-decl.c:7525:1: internal compiler error: verify_ssa failed
0x115356db verify_ssa(bool, bool)
../../gcc/tree-ssa.c:1186
0x10f850ff execute_function_todo
../../gcc/passes.c:1996
0x10f8376b do_per_function
../../gcc/passes.c:1655
0x10f853a7 execute_todo
../../gcc/passes.c:2043
Sending new version that I'm going to test.
Martin
>
> Jakub
>
>From bd7580c259c0552490d63527782d00926c9c5473 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
gcc/testsuite/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* g++.dg/asan/function-argument-1.C: New test.
* g++.dg/asan/function-argument-2.C: New test.
* g++.dg/asan/function-argument-3.C: New test.
gcc/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* sanopt.c (rewrite_usage_of_param): New function.
(sanitize_rewrite_addressable_params): Likewise.
(pass_sanopt::execute): Call rewrite_usage_of_param.
---
gcc/sanopt.c| 135
gcc/testsuite/g++.dg/asan/function-argument-1.C | 30 ++
gcc/testsuite/g++.dg/asan/function-argument-2.C | 24 +
gcc/testsuite/g++.dg/asan/function-argument-3.C | 27 +
4 files changed, 216 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C
diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..acb095b 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,12 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-ssa.h"
#include "tree-phinodes.h"
#include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
+#include "tree-dfa.h"
+#include "tree-ssa.h"
/* This is used to carry information about basic blocks. It is
attached to the AUX field of the standard CFG block. */
@@ -858,6 +864,132 @@ sanitize_asan_mark_poison (void)
}
}
+/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
+ that is it's DECL_VALUE_EXPR. */
+
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
+{
+ if (TREE_CODE (*op) == PARM_DECL && DECL_HAS_VALUE_EXPR_P (*op))
+{
+ *op = DECL_VALUE_EXPR (*op);
+ *walk_subtrees = 0;
+}
+
+ return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+ a new automatic variable is introduced. Right after function entry
+ a parameter is assigned to the variable. */
+
+static void
+sanitize_rewrite_addressable_params (function *fun)
+{
+ gimple *g;
+ gimple_seq stmts = NULL;
+ bool has_any_addressable_param = false;
+ auto_vec clear_value_expr_list;
+
+ for (tree arg = DECL_ARGUMENTS (current_function_decl);
+ arg; arg = DECL_CHAIN (arg))
+{
+ if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
+ {
+ TREE_ADDRESSABLE (arg) = 0;
+ /* The parameter is no longer addressable. */
+ tree type = TREE_TYPE (arg);
+ has_any_addressable_param = true;
+
+ /* Create a new automatic variable. */
+ tree var = build_decl (DECL_SOURCE_LOC
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On Tue, Jun 20, 2017 at 03:06:56PM +0200, Martin Liška wrote:
> +/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
> + that is it's DECL_VALUE_EXPR. */
> +
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
> +{
> + if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)
DECL_VALUE_EXPR testing is costly (it is a hash table lookup).
Therefore you should test DECL_HAS_VALUE_EXPR_P (*op) after checking
== PARM_DECL. And DECL_HAS_VALUE_EXPR_P should apply non-NULL
DECL_VALUE_EXPR.
That said, I wonder if we don't create DECL_VALUE_EXPR for PARM_DECLs in
other parts of the compiler, whether it wouldn't be safer to also test here
after == PARM_DECL and DECL_HAS_VALUE_EXPR_P check whether *op is in
addressable_params hash table.
> +{
> + *op = DECL_VALUE_EXPR (*op);
> + *walk_subtrees = 0;
> +}
> +
> + return NULL;
> +}
> +
> +/* For a given function FUN, rewrite all addressable parameters so that
> + a new automatic variable is introduced. Right after function entry
> + a parameter is assigned to the variable. */
> +
> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> + gimple *g;
> + gimple_seq stmts = NULL;
> + auto_vec addressable_params;
You don't really use the addressable_params vector anywhere, right?
Except for:
> +
> + for (tree arg = DECL_ARGUMENTS (current_function_decl);
> + arg; arg = DECL_CHAIN (arg))
> +{
> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> + {
> + TREE_ADDRESSABLE (arg) = 0;
> + /* The parameter is no longer addressable. */
> + tree type = TREE_TYPE (arg);
> + addressable_params.safe_push (arg);
pushing stuff into it and later
> + if (addressable_params.is_empty ())
> +return;
If you only need that, a bool flag if any params have been changed is
enough. But see above whether it wouldn't be safer to use a hash table
to verify it. Plus, I think it would be desirable to clear
DECL_HAS_VALUE_EXPR_P and SET_DECL_VALUE_EXPR to NULL afterwards
if (target_for_debug_bind (arg)) - whch can be done either the with vec
or with a hash table traversal, for that we don't care about the ordering.
> +
> + /* Create a new automatic variable. */
> + tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> + VAR_DECL, DECL_NAME (arg), type);
> + TREE_ADDRESSABLE (var) = 1;
> + DECL_ARTIFICIAL (var) = 1;
> + DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
This is 0 already from build_decl, IMHO no need to set it.
> + gimple_add_tmp_var (var);
> +
> + if (dump_file)
> + fprintf (dump_file,
> + "Rewriting parameter whose address is taken: %s\n",
> + IDENTIFIER_POINTER (DECL_NAME (arg)));
> +
> + SET_DECL_VALUE_EXPR (arg, var);
But obviously you miss setting DECL_HAS_VALUE_EXPR_P here.
> + /* Assign value of parameter to newly created variable. */
> + if ((TREE_CODE (type) == COMPLEX_TYPE
> +|| TREE_CODE (type) == VECTOR_TYPE))
> + {
> + /* We need to create a SSA name that will be used for the
> + assignment. */
Why don't you just set DECL_GIMPLE_REG_P (arg) = 1; for
COMPLEX_TYPE/VECTOR_TYPE? The arg is going to be only used to copy it into
the new var. And then just use get_or_create_ssa_default_def,
regardless of whether if is complex/vector or other.
> + /* Replace all usages of PARM_DECLs with the newly
> + created variable VAR. */
> + basic_block bb;
> + FOR_EACH_BB_FN (bb, fun)
> +{
> + gimple_stmt_iterator gsi;
> + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next ())
> + {
> + gimple *stmt = gsi_stmt (gsi);
> + gimple_stmt_iterator it = gsi_for_stmt (stmt);
> + walk_gimple_stmt (, NULL, rewrite_usage_of_param, NULL);
> + }
> + for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next ())
> + {
> + gphi *phi = dyn_cast (gsi_stmt (gsi));
> + for (unsigned i = 0; i < gimple_phi_num_args (phi); ++i)
> + {
> + hash_set visited_nodes;
> + walk_tree (gimple_phi_arg_def_ptr (phi, i),
> + rewrite_usage_of_param, NULL, _nodes);
> + }
Doesn't walk_gimple_stmt on the PHI handle this?
Jakub
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
PING^1 On 06/20/2017 03:06 PM, Martin Liška wrote: > On 06/20/2017 11:32 AM, Jakub Jelinek wrote: >> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote: Then something needs to be done for debugging too. If it is without VTA, then probably just having DECL_VALUE_EXPR is good enough, otherwise (VTA) you probably don't want that (or can reset it at that point), but instead emit after the initialization stmt a debug stmt that the variable value now lives in a different var. Though ideally we want the debugger to be able to also change the value of the var, that might be harder. With DECL_VALUE_EXPR on the other side the debug info will be incorrect in the prologue until it is assigned to the slot. >>> >>> Here I'm not sure about how to distinguish whether to build or not to build >>> the debug statement. According to flag_var_tracking? >> >> More like if (target_for_debug_bind (arg)) >> And if that is false, just make sure DECL_VALUE_EXPR is set to var. >> >>> You mean something like: >>> g = gimple_build_debug_bind (arg, var, g); >>> ? >> >> Well, there is no stmt, so the last argument would be just NULL. >> I don't understand the distinction. If you turn the original parm for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code (but I think it would be better to use the default SSA_NAME of the PARM_DECL if it is a gimple reg type, rather than use the PARM_DECL itself and wait for update_ssa). >>> >>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails >>> for me >>> as one needs to have a temporary SSA name, otherwise: >>> >>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: >>> error: invalid rhs for gimple memory store >>> foo (v4si arg) >>> ^~~ >>> arg >>> >>> arg >>> >>> # .MEM_4 = VDEF <.MEM_1(D)> >>> arg = arg; >>> during GIMPLE pass: sanopt >>> >>> If I see correctly the function in my test-case does not have default def >>> SSA name for the parameter. >>> Thus I guess I need to create a SSA name? >> >> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and >> use the default def, you shouldn't run into this. >> >> Jakub >> > > Good I fixed that in v2, that passes regression tests. > Ale objections should be resolved in the version. > > Ready for trunk? > Martin >
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On 06/20/2017 11:32 AM, Jakub Jelinek wrote:
> On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote:
>>> Then something needs to be done for debugging too. If it is without VTA,
>>> then probably just having DECL_VALUE_EXPR is good enough, otherwise
>>> (VTA) you probably don't want that (or can reset it at that point), but
>>> instead emit after the initialization stmt a debug stmt that the variable
>>> value now lives in a different var. Though ideally we want the debugger
>>> to be able to also change the value of the var, that might be harder.
>>> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
>>> the prologue until it is assigned to the slot.
>>
>> Here I'm not sure about how to distinguish whether to build or not to build
>> the debug statement. According to flag_var_tracking?
>
> More like if (target_for_debug_bind (arg))
> And if that is false, just make sure DECL_VALUE_EXPR is set to var.
>
>> You mean something like:
>> g = gimple_build_debug_bind (arg, var, g);
>> ?
>
> Well, there is no stmt, so the last argument would be just NULL.
>
>>> I don't understand the distinction. If you turn the original parm
>>> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
>>> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
>>> if it is a gimple reg type, rather than use the PARM_DECL itself
>>> and wait for update_ssa).
>>
>> Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails
>> for me
>> as one needs to have a temporary SSA name, otherwise:
>>
>> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1:
>> error: invalid rhs for gimple memory store
>> foo (v4si arg)
>> ^~~
>> arg
>>
>> arg
>>
>> # .MEM_4 = VDEF <.MEM_1(D)>
>> arg = arg;
>> during GIMPLE pass: sanopt
>>
>> If I see correctly the function in my test-case does not have default def
>> SSA name for the parameter.
>> Thus I guess I need to create a SSA name?
>
> I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and
> use the default def, you shouldn't run into this.
>
> Jakub
>
Good I fixed that in v2, that passes regression tests.
Ale objections should be resolved in the version.
Ready for trunk?
Martin
>From ed5da705250c3015e964de8d23d1aa3d0056012a Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
gcc/testsuite/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* g++.dg/asan/function-argument-1.C: New test.
* g++.dg/asan/function-argument-2.C: New test.
* g++.dg/asan/function-argument-3.C: New test.
gcc/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* sanopt.c (rewrite_usage_of_param): New function.
(sanitize_rewrite_addressable_params): Likewise.
(pass_sanopt::execute): Call rewrite_usage_of_param.
---
gcc/sanopt.c| 132
gcc/testsuite/g++.dg/asan/function-argument-1.C | 30 ++
gcc/testsuite/g++.dg/asan/function-argument-2.C | 24 +
gcc/testsuite/g++.dg/asan/function-argument-3.C | 27 +
4 files changed, 213 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C
diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..077811b5b93 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,12 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-ssa.h"
#include "tree-phinodes.h"
#include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
+#include "tree-dfa.h"
+#include "tree-ssa.h"
/* This is used to carry information about basic blocks. It is
attached to the AUX field of the standard CFG block. */
@@ -858,6 +864,129 @@ sanitize_asan_mark_poison (void)
}
}
+/* Rewrite all usages of tree OP which is a PARM_DECL with a VAR_DECL
+ that is it's DECL_VALUE_EXPR. */
+
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *)
+{
+ if (TREE_CODE (*op) == PARM_DECL && DECL_VALUE_EXPR (*op) != NULL_TREE)
+{
+ *op = DECL_VALUE_EXPR (*op);
+ *walk_subtrees = 0;
+}
+
+ return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+ a new automatic variable is introdu
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On Tue, Jun 20, 2017 at 11:23:36AM +0200, Martin Liška wrote: > > Then something needs to be done for debugging too. If it is without VTA, > > then probably just having DECL_VALUE_EXPR is good enough, otherwise > > (VTA) you probably don't want that (or can reset it at that point), but > > instead emit after the initialization stmt a debug stmt that the variable > > value now lives in a different var. Though ideally we want the debugger > > to be able to also change the value of the var, that might be harder. > > With DECL_VALUE_EXPR on the other side the debug info will be incorrect in > > the prologue until it is assigned to the slot. > > Here I'm not sure about how to distinguish whether to build or not to build > the debug statement. According to flag_var_tracking? More like if (target_for_debug_bind (arg)) And if that is false, just make sure DECL_VALUE_EXPR is set to var. > You mean something like: > g = gimple_build_debug_bind (arg, var, g); > ? Well, there is no stmt, so the last argument would be just NULL. > > I don't understand the distinction. If you turn the original parm > > for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code > > (but I think it would be better to use the default SSA_NAME of the PARM_DECL > > if it is a gimple reg type, rather than use the PARM_DECL itself > > and wait for update_ssa). > > Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for > me > as one needs to have a temporary SSA name, otherwise: > > /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1: > error: invalid rhs for gimple memory store > foo (v4si arg) > ^~~ > arg > > arg > > # .MEM_4 = VDEF <.MEM_1(D)> > arg = arg; > during GIMPLE pass: sanopt > > If I see correctly the function in my test-case does not have default def SSA > name for the parameter. > Thus I guess I need to create a SSA name? I'd expect if you have DECL_GIMPLE_REG_P set on the PARM_DECL and use the default def, you shouldn't run into this. Jakub
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On 06/19/2017 04:13 PM, Jakub Jelinek wrote:
> On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote:
>> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
>> }
>> }
>>
>
> Missing function comment.
>
>> +static tree
>> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
>> +{
>> + struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
>> + std::pair *replacement = (std::pair *)wi->info;
>
> Missing space after )
>
>> +
>> + if (*op == replacement->first)
>> +{
>> + *op = replacement->second;
>> + *walk_subtrees = 0;
>> +}
>> +
>> + return NULL;
>> +}
>
>> +static void
>> +sanitize_rewrite_addressable_params (function *fun)
>> +{
>> + basic_block entry_bb = NULL;
>> +
>> + for (tree arg = DECL_ARGUMENTS (current_function_decl);
>> + arg; arg = DECL_CHAIN (arg))
>> +{
>> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
>> +{
>> + /* The parameter is no longer addressable. */
>> + tree type = TREE_TYPE (arg);
>> + TREE_ADDRESSABLE (arg) = 0;
>> +
>> + /* Create a new automatic variable. */
>> + tree var = build_decl (DECL_SOURCE_LOCATION (arg),
>> + VAR_DECL, DECL_NAME (arg), type);
>> + TREE_ADDRESSABLE (var) = 1;
>> + DECL_ARTIFICIAL (var) = 1;
>> + DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
>
> I think it is highly inefficient to walk the whole IL for every addressable
> argument. Can't you first find out what PARM_DECLs you need to change,
> stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs
> perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever),
> then if you create at least one, walk whole IL and replace all the
> PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE
> flag for all of them and emit the initialization sequence?
Yes, this is doable. I've done that.
> Then something needs to be done for debugging too. If it is without VTA,
> then probably just having DECL_VALUE_EXPR is good enough, otherwise
> (VTA) you probably don't want that (or can reset it at that point), but
> instead emit after the initialization stmt a debug stmt that the variable
> value now lives in a different var. Though ideally we want the debugger
> to be able to also change the value of the var, that might be harder.
> With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
> the prologue until it is assigned to the slot.
Here I'm not sure about how to distinguish whether to build or not to build
the debug statement. According to flag_var_tracking?
You mean something like:
g = gimple_build_debug_bind (arg, var, g);
?
>
>> +
>> + gimple_add_tmp_var (var);
>> +
>> + if (dump_file)
>> +fprintf (dump_file,
>> + "Rewritting parameter whos address is taken: %s\n",
>> + IDENTIFIER_POINTER (DECL_NAME (arg)));
>
> s/tting/ting/, s/whos/whose/
>> +
>> + gimple_seq stmts = NULL;
>> +
>> + /* Assign value of parameter to newly created variable. */
>> + if ((TREE_CODE (type) == COMPLEX_TYPE
>> + || TREE_CODE (type) == VECTOR_TYPE))
>> +{
>> + /* We need to create a SSA name that will be used for the
>> + assignment. */
>> + tree tmp = make_ssa_name (type);
>> + gimple *g = gimple_build_assign (tmp, arg);
>> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> + gimple_seq_add_stmt (, g);
>> + g = gimple_build_assign (var, tmp);
>> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> + gimple_seq_add_stmt (, g);
>> +}
>> + else
>> +{
>> + gimple *g = gimple_build_assign (var, arg);
>> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
>> + gimple_seq_add_stmt (, g);
>> +}
>
> I don't understand the distinction. If you turn the original parm
> for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
> (but I think it would be better to use the default SSA_NAME of the PARM_DECL
> if it is a gimple reg type, rather than use the PARM_DECL itself
> and wait for update_ssa).
Yes, the test-case /gcc/testsuite/g++.dg/asan/function-argument-3.C fails for me
as one needs to have a temporary SSA name, otherwise:
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/function-argument-3.C:13:1:
error: invalid rhs for gimple memory store
foo (v4si arg)
^~~
arg
arg
# .MEM_4 = VDEF <.MEM_1(D)>
arg = arg;
during GIMPLE pass: sanopt
If I see correctly the function in my test-case does not have default def SSA
name for the parameter.
Thus I guess I need to create a SSA name?
Thanks,
Martin
>
> Jakub
>
Re: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
On Mon, Jun 19, 2017 at 03:50:42PM +0200, Martin Liška wrote:
> @@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
> }
> }
>
Missing function comment.
> +static tree
> +rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
> +{
> + struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
> + std::pair *replacement = (std::pair *)wi->info;
Missing space after )
> +
> + if (*op == replacement->first)
> +{
> + *op = replacement->second;
> + *walk_subtrees = 0;
> +}
> +
> + return NULL;
> +}
> +static void
> +sanitize_rewrite_addressable_params (function *fun)
> +{
> + basic_block entry_bb = NULL;
> +
> + for (tree arg = DECL_ARGUMENTS (current_function_decl);
> + arg; arg = DECL_CHAIN (arg))
> +{
> + if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
> + {
> + /* The parameter is no longer addressable. */
> + tree type = TREE_TYPE (arg);
> + TREE_ADDRESSABLE (arg) = 0;
> +
> + /* Create a new automatic variable. */
> + tree var = build_decl (DECL_SOURCE_LOCATION (arg),
> + VAR_DECL, DECL_NAME (arg), type);
> + TREE_ADDRESSABLE (var) = 1;
> + DECL_ARTIFICIAL (var) = 1;
> + DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
I think it is highly inefficient to walk the whole IL for every addressable
argument. Can't you first find out what PARM_DECLs you need to change,
stick the corresponding VAR_DECL somewhere (dunno, e.g. a vector with pairs
perhaps sorted by DECL_UID, or stick it into DECL_VALUE_EXPR or whatever),
then if you create at least one, walk whole IL and replace all the
PARM_DECLs you want to replace, then finally clear the TREE_ADDRESSABLE
flag for all of them and emit the initialization sequence?
Then something needs to be done for debugging too. If it is without VTA,
then probably just having DECL_VALUE_EXPR is good enough, otherwise
(VTA) you probably don't want that (or can reset it at that point), but
instead emit after the initialization stmt a debug stmt that the variable
value now lives in a different var. Though ideally we want the debugger
to be able to also change the value of the var, that might be harder.
With DECL_VALUE_EXPR on the other side the debug info will be incorrect in
the prologue until it is assigned to the slot.
> +
> + gimple_add_tmp_var (var);
> +
> + if (dump_file)
> + fprintf (dump_file,
> + "Rewritting parameter whos address is taken: %s\n",
> + IDENTIFIER_POINTER (DECL_NAME (arg)));
s/tting/ting/, s/whos/whose/
> +
> + gimple_seq stmts = NULL;
> +
> + /* Assign value of parameter to newly created variable. */
> + if ((TREE_CODE (type) == COMPLEX_TYPE
> +|| TREE_CODE (type) == VECTOR_TYPE))
> + {
> + /* We need to create a SSA name that will be used for the
> + assignment. */
> + tree tmp = make_ssa_name (type);
> + gimple *g = gimple_build_assign (tmp, arg);
> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> + gimple_seq_add_stmt (, g);
> + g = gimple_build_assign (var, tmp);
> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> + gimple_seq_add_stmt (, g);
> + }
> + else
> + {
> + gimple *g = gimple_build_assign (var, arg);
> + gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
> + gimple_seq_add_stmt (, g);
> + }
I don't understand the distinction. If you turn the original parm
for complex/vector DECL_GIMPLE_REG_P, you should need the exact same code
(but I think it would be better to use the default SSA_NAME of the PARM_DECL
if it is a gimple reg type, rather than use the PARM_DECL itself
and wait for update_ssa).
Jakub
[PATCH] ASAN: handle addressable params (PR sanitize/81040).
Hi.
Following patch addresses issue where we have a function argument which address
is taken and -fsanitize=address does not wrap up the argument with red zone.
It's done in sanopt pass, where I create a new automatic variable which is used
in the function instead of the original argument.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
And I can bootstrap-asan on the same machine.
Ready to be installed?
Martin
>From f8a48a3f361d9914dd45c1896e8c5ba607a62b06 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 14 Jun 2017 11:40:01 +0200
Subject: [PATCH] ASAN: handle addressable params (PR sanitize/81040).
gcc/testsuite/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* g++.dg/asan/function-argument-1.C: New test.
* g++.dg/asan/function-argument-2.C: New test.
* g++.dg/asan/function-argument-3.C: New test.
gcc/ChangeLog:
2017-06-19 Martin Liska <mli...@suse.cz>
PR sanitize/81040
* sanopt.c (rewrite_usage_of_param): New function.
(sanitize_rewrite_addressable_params): Likewise.
(pass_sanopt::execute): Call rewrite_usage_of_param.
---
gcc/sanopt.c| 118
gcc/testsuite/g++.dg/asan/function-argument-1.C | 30 ++
gcc/testsuite/g++.dg/asan/function-argument-2.C | 24 +
gcc/testsuite/g++.dg/asan/function-argument-3.C | 27 ++
4 files changed, 199 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-1.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-2.C
create mode 100644 gcc/testsuite/g++.dg/asan/function-argument-3.C
diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 16bdba76042..10464841972 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -37,6 +37,10 @@ along with GCC; see the file COPYING3. If not see
#include "gimple-ssa.h"
#include "tree-phinodes.h"
#include "ssa-iterators.h"
+#include "gimplify.h"
+#include "gimple-iterator.h"
+#include "gimple-walk.h"
+#include "cfghooks.h"
/* This is used to carry information about basic blocks. It is
attached to the AUX field of the standard CFG block. */
@@ -858,6 +862,117 @@ sanitize_asan_mark_poison (void)
}
}
+static tree
+rewrite_usage_of_param (tree *op, int *walk_subtrees, void *data)
+{
+ struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
+ std::pair<tree, tree> *replacement = (std::pair<tree, tree> *)wi->info;
+
+ if (*op == replacement->first)
+{
+ *op = replacement->second;
+ *walk_subtrees = 0;
+}
+
+ return NULL;
+}
+
+/* For a given function FUN, rewrite all addressable parameters so that
+ a new automatic variable is introduced. Right after function entry
+ a parameter is assigned to the variable. */
+
+static void
+sanitize_rewrite_addressable_params (function *fun)
+{
+ basic_block entry_bb = NULL;
+
+ for (tree arg = DECL_ARGUMENTS (current_function_decl);
+ arg; arg = DECL_CHAIN (arg))
+{
+ if (TREE_ADDRESSABLE (arg) && !TREE_ADDRESSABLE (TREE_TYPE (arg)))
+ {
+ /* The parameter is no longer addressable. */
+ tree type = TREE_TYPE (arg);
+ TREE_ADDRESSABLE (arg) = 0;
+
+ /* Create a new automatic variable. */
+ tree var = build_decl (DECL_SOURCE_LOCATION (arg),
+ VAR_DECL, DECL_NAME (arg), type);
+ TREE_ADDRESSABLE (var) = 1;
+ DECL_ARTIFICIAL (var) = 1;
+ DECL_SEEN_IN_BIND_EXPR_P (var) = 0;
+
+ gimple_add_tmp_var (var);
+
+ if (dump_file)
+ fprintf (dump_file,
+ "Rewritting parameter whos address is taken: %s\n",
+ IDENTIFIER_POINTER (DECL_NAME (arg)));
+
+ gimple_seq stmts = NULL;
+
+ /* Assign value of parameter to newly created variable. */
+ if ((TREE_CODE (type) == COMPLEX_TYPE
+ || TREE_CODE (type) == VECTOR_TYPE))
+ {
+ /* We need to create a SSA name that will be used for the
+ assignment. */
+ tree tmp = make_ssa_name (type);
+ gimple *g = gimple_build_assign (tmp, arg);
+ gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+ gimple_seq_add_stmt (, g);
+ g = gimple_build_assign (var, tmp);
+ gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+ gimple_seq_add_stmt (, g);
+ }
+ else
+ {
+ gimple *g = gimple_build_assign (var, arg);
+ gimple_set_location (g, DECL_SOURCE_LOCATION (arg));
+ gimple_seq_add_stmt (, g);
+ }
+
+ /* Replace all usages of PARM_DECL with the newly
+ created variable VAR. */
+ basic_block bb;
+ gimple_stmt_iterator gsi;
+ FOR_EACH_BB_FN (bb, fun)
+ {
+ std::pair<tree, tree> replacement (arg, var);
+ struct walk_stmt_info wi;
+ memset (, 0, sizeof (wi));
+ wi.info = (void *)
+
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next ())
+ {
+ gimple *stmt = gsi_stmt (gsi);
+ gimple_stmt_iterator it = gsi_for_stmt (
