On 12/09/2016 12:56 PM, Jakub Jelinek wrote:
> On Fri, Dec 09, 2016 at 12:39:24PM +0100, Martin Liška wrote:
>> + if (is_gimple_call (stmt) && gimple_call_internal_p (stmt))
>> + {
>> + enum internal_fn ifn = gimple_call_internal_fn (stmt);
>> + switch (ifn)
>> + {
>> + case IFN_ASAN_MARK:
>> + {
>> + HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt,
>> 0));
>> + bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
>> + if (is_clobber)
>> + {
>> + bitmap_set_bit (with_poison, bb->index);
>> + finish = true;
>> + }
>> + break;
>> + }
>> + default:
>> + break;
>> + }
>
> This looks weird. Wouldn't it be nicer to just use
> if (gimple_call_internal_fn (stmt) == IFN_ASAN_MARK)
> {
> HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt,
> 0));
> bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
> if (is_clobber)
> {
> bitmap_set_bit (with_poison, bb->index);
> finish = true;
> }
> }
> (or don't use finish and just break; there)?
>> + enum internal_fn ifn = gimple_call_internal_fn (stmt);
>> + switch (ifn)
>> + {
>> + case IFN_ASAN_MARK:
>> + {
>
> Likewise.
>
> No function comment:
>
>> +static bool
>> +maybe_contains_asan_check (gimple *stmt)
>> +{
>> + if (is_gimple_call (stmt))
>> + {
>> + if (gimple_call_internal_p (stmt))
>> + {
>> + enum internal_fn ifn = gimple_call_internal_fn (stmt);
>> + switch (ifn)
>> + {
>> + case IFN_ASAN_CHECK:
>> + return true;
>> + default:
>> + return false;
>
> Are all internal functions really known to be ASAN_CHECK free?
I rewrite the function to utilize gimple_call_flags to have ECF_CONST. These
functions should not touch memory, thus should not contain ASAN_CHECK.
>
>> + }
>> + }
>> + else
>> + return true;
>
> What about builtins? Many will not be fine, but many should be known
> to be ASAN_CHECK free. Consider e.g. most math builtins (except sincos).
>
>> @@ -698,6 +928,9 @@ pass_sanopt::execute (function *fun)
>> bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
>> && asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
>>
>> + sanitize_asan_mark_unpoison ();
>> + sanitize_asan_mark_poison ();
>> +
>
> What I don't really like here is that you force several IL walks for all
> sanitizers, just in case. Look e.g. how asan_num_accesses is computed,
> couldn't you compute similarly has_asan_marks and guard those two functions
> on that?
>
> Jakub
>
All other remarks were resolved. Attaching new version and I'm going to test it.
Thanks,
Martin
>From 07fea05d31feb19474cb5e383764b30933ddd09a Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Fri, 25 Nov 2016 15:05:33 +0100
Subject: [PATCH] Add sanopt for ASAN_MARK poison and unpoison.
---
gcc/sanopt.c | 218 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 215 insertions(+), 3 deletions(-)
diff --git a/gcc/sanopt.c b/gcc/sanopt.c
index 77307d9..b98e8d7 100644
--- a/gcc/sanopt.c
+++ b/gcc/sanopt.c
@@ -160,8 +160,10 @@ struct sanopt_ctx
/* Number of IFN_ASAN_CHECK statements. */
int asan_num_accesses;
-};
+ /* True when the current functions constains an ASAN_MARK. */
+ bool contains_asan_mark;
+};
/* Return true if there might be any call to free/munmap operation
on any path in between DOM (which should be imm(BB)) and BB. */
@@ -582,6 +584,9 @@ sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
if (!remove)
ctx->asan_num_accesses++;
break;
+ case IFN_ASAN_MARK:
+ ctx->contains_asan_mark = true;
+ break;
default:
break;
}
@@ -620,10 +625,11 @@ sanopt_optimize_walker (basic_block bb, struct sanopt_ctx *ctx)
/* Try to remove redundant sanitizer checks in function FUN. */
static int
-sanopt_optimize (function *fun)
+sanopt_optimize (function *fun, bool *contains_asan_mark)
{
struct sanopt_ctx ctx;
ctx.asan_num_accesses = 0;
+ ctx.contains_asan_mark = false;
/* Set up block info for each basic block. */
alloc_aux_for_blocks (sizeof (sanopt_info));
@@ -638,6 +644,7 @@ sanopt_optimize (function *fun)
free_aux_for_blocks ();
+ *contains_asan_mark = ctx.contains_asan_mark;
return ctx.asan_num_accesses;
}
@@ -671,18 +678,215 @@ public:
}; // class pass_sanopt
+static void
+sanitize_asan_mark_unpoison (void)
+{
+ /* 1) Find all BBs that contain an ASAN_MARK poison call. */
+ auto_sbitmap with_poison (last_basic_block_for_fn (cfun) + 1);
+ bitmap_clear (with_poison);
+ basic_block bb;
+
+ FOR_EACH_BB_FN (bb, cfun)
+ {
+ if (bitmap_bit_p (with_poison, bb->index))
+ continue;
+
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+ {
+ gimple *stmt = gsi_stmt (gsi);
+ if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+ {
+ HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
+ bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+ if (is_clobber)
+ {
+ bitmap_set_bit (with_poison, bb->index);
+ break;
+ }
+ }
+ }
+ }
+
+ auto_sbitmap poisoned (last_basic_block_for_fn (cfun) + 1);
+ bitmap_clear (poisoned);
+ auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+ bitmap_copy (worklist, with_poison);
+
+ /* 2) Propagate the information to all reachable blocks. */
+ while (!bitmap_empty_p (worklist))
+ {
+ unsigned i = bitmap_first_set_bit (worklist);
+ bitmap_clear_bit (worklist, i);
+ basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+ gcc_assert (bb);
+
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->succs)
+ if (!bitmap_bit_p (poisoned, e->dest->index))
+ {
+ bitmap_set_bit (poisoned, e->dest->index);
+ bitmap_set_bit (worklist, e->dest->index);
+ }
+ }
+
+ /* 3) Iterate all BBs not included in POISONED BBs and remove unpoison
+ ASAN_MARK preceding an ASAN_MARK poison (which can still happen). */
+ FOR_EACH_BB_FN (bb, cfun)
+ {
+ if (bitmap_bit_p (poisoned, bb->index))
+ continue;
+
+ gimple_stmt_iterator gsi;
+ for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi);)
+ {
+ bool next = true;
+ gimple *stmt = gsi_stmt (gsi);
+ if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+ {
+ HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
+ bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+ if (is_clobber)
+ break;
+ else
+ {
+ if (dump_file)
+ fprintf (dump_file, "Removing ASAN_MARK unpoison\n");
+ unlink_stmt_vdef (stmt);
+ gsi_remove (&gsi, true);
+ next = false;
+ }
+ }
+
+ if (next)
+ gsi_next (&gsi);
+ }
+ }
+}
+
+/* Return true when STMT is either ASAN_CHECK call or a call of a function
+ that can contain an ASAN_CHECK. */
+
+static bool
+maybe_contains_asan_check (gimple *stmt)
+{
+ if (is_gimple_call (stmt))
+ {
+ if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+ return false;
+ else
+ return !(gimple_call_flags (stmt) & ECF_CONST);
+ }
+ else if (is_a<gasm *> (stmt))
+ return true;
+
+ return false;
+}
+
+static void
+sanitize_asan_mark_poison (void)
+{
+ /* 1) Find all BBs that possibly contain an ASAN_CHECK. */
+ auto_sbitmap with_check (last_basic_block_for_fn (cfun) + 1);
+ bitmap_clear (with_check);
+ basic_block bb;
+
+ FOR_EACH_BB_FN (bb, cfun)
+ {
+ if (bitmap_bit_p (with_check, bb->index))
+ continue;
+
+ gimple_stmt_iterator gsi;
+ bool finish = false;
+ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
+ {
+ gimple *stmt = gsi_stmt (gsi);
+ if (maybe_contains_asan_check (stmt))
+ {
+ bitmap_set_bit (with_check, bb->index);
+ finish = true;
+ }
+
+ if (finish)
+ break;
+ }
+ }
+
+ auto_sbitmap can_reach_check (last_basic_block_for_fn (cfun) + 1);
+ bitmap_clear (can_reach_check);
+ auto_sbitmap worklist (last_basic_block_for_fn (cfun) + 1);
+ bitmap_copy (worklist, with_check);
+
+ /* 2) Propagate the information to all definitions blocks. */
+ while (!bitmap_empty_p (worklist))
+ {
+ unsigned i = bitmap_first_set_bit (worklist);
+ bitmap_clear_bit (worklist, i);
+ basic_block bb = BASIC_BLOCK_FOR_FN (cfun, i);
+ gcc_assert (bb);
+
+ edge e;
+ edge_iterator ei;
+ FOR_EACH_EDGE (e, ei, bb->preds)
+ if (!bitmap_bit_p (can_reach_check, e->src->index))
+ {
+ bitmap_set_bit (can_reach_check, e->src->index);
+ bitmap_set_bit (worklist, e->src->index);
+ }
+ }
+
+ /* 3) Iterate all BBs not included in CAN_REACH_CHECK BBs and remove poison
+ ASAN_MARK not followed by a call to function having an ASAN_CHECK. */
+ FOR_EACH_BB_FN (bb, cfun)
+ {
+ if (bitmap_bit_p (can_reach_check, bb->index))
+ continue;
+
+ gimple_stmt_iterator gsi;
+ bool finish = false;
+ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi);)
+ {
+ bool prev = true;
+ gimple *stmt = gsi_stmt (gsi);
+ if (maybe_contains_asan_check (stmt))
+ finish = true;
+ else if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+ {
+ HOST_WIDE_INT flags = tree_to_shwi (gimple_call_arg (stmt, 0));
+ bool is_clobber = (flags & ASAN_MARK_CLOBBER) != 0;
+ if (is_clobber)
+ {
+ if (dump_file)
+ fprintf (dump_file, "Removing ASAN_MARK poison\n");
+ unlink_stmt_vdef (stmt);
+ gsi_remove (&gsi, true);
+ prev = false;
+ }
+ }
+
+ if (finish)
+ break;
+
+ if (prev)
+ gsi_prev (&gsi);
+ }
+ }
+}
+
unsigned int
pass_sanopt::execute (function *fun)
{
basic_block bb;
int asan_num_accesses = 0;
+ bool contains_asan_mark = false;
/* Try to remove redundant checks. */
if (optimize
&& (flag_sanitize
& (SANITIZE_NULL | SANITIZE_ALIGNMENT
| SANITIZE_ADDRESS | SANITIZE_VPTR)))
- asan_num_accesses = sanopt_optimize (fun);
+ asan_num_accesses = sanopt_optimize (fun, &contains_asan_mark);
else if (flag_sanitize & SANITIZE_ADDRESS)
{
gimple_stmt_iterator gsi;
@@ -692,12 +896,20 @@ pass_sanopt::execute (function *fun)
gimple *stmt = gsi_stmt (gsi);
if (gimple_call_internal_p (stmt, IFN_ASAN_CHECK))
++asan_num_accesses;
+ else if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+ contains_asan_mark = true;
}
}
bool use_calls = ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD < INT_MAX
&& asan_num_accesses >= ASAN_INSTRUMENTATION_WITH_CALL_THRESHOLD;
+ if (contains_asan_mark)
+ {
+ sanitize_asan_mark_unpoison ();
+ sanitize_asan_mark_poison ();
+ }
+
bool need_commit_edge_insert = false;
FOR_EACH_BB_FN (bb, fun)
{
--
2.10.2
