On Tue, Jan 17, 2017 at 05:16:44PM +0100, Martin Liška wrote:
> > If it did, we would ICE because ASAN_POISON_USE would survive this way until
> > expansion. A quick fix for the ICE (if it can ever happen) would be easy,
> > in sanopt remove ASAN_POISON_USE calls which have argument that is not lhs
> > of ASAN_POISON (all other ASAN_POISON_USE calls will be handled by my
> > incremental patch). Of course that would also mean in that case we'd report
> > a read rather than write. But if it can't happen or is very unlikely to
> > happen, then it is a non-issue.
>
> Thank you Jakub for working on that.
>
> The patch is fine, I added DCE support and a test-case. Please see attached
> patch.
> asan.exp regression tests look fine and I've been building linux kernel with
> KASAN
> enabled. I'll also do asan-boostrap.
>
> I would like to commit the patch soon, should I squash both patches together,
> or would it
> be preferred to separate basic optimization and support for stores?
Your choice, either is fine. If the two patches pass bootstrap/regtest
(ideally also asan-bootstrap), they are ok for trunk. Just one nit:
> --- a/gcc/tree-ssa-dce.c
> +++ b/gcc/tree-ssa-dce.c
> @@ -1384,6 +1384,10 @@ eliminate_unnecessary_stmts (void)
> case IFN_MUL_OVERFLOW:
> maybe_optimize_arith_overflow (&gsi, MULT_EXPR);
> break;
> + case IFN_ASAN_POISON:
> + if (!gimple_has_lhs (stmt))
> + remove_dead_stmt (&gsi, bb);
> + break;
> default:
> break;
> }
This doesn't seem to be the best spot for it. At least when looking at
say:
int
foo (int x)
{
int *ptr = 0;
if (x < 127)
return 5;
{
int a;
ptr = &a;
*ptr = 12345;
}
if (x == 34)
return *ptr;
return 7;
}
where the ASAN_POISON is initially used and only after evrp becomes dead,
then cddce1 calls eliminate_unnecessary_stmts and removes the lhs of the
ASAN_POISON only (and not the whole stmt, unlike how e.g. GOMP_SIMD_LANE is
handled), and only next dce pass tons of passes later removes the
ASAN_POISON call.
So IMHO you need one of these (untested) patches. The former assumes that
the DCE pass is the only one that can drop the lhs of ASAN_POISON. If that
is not the case, then perhaps the second patch is better, by removing the
stmt regardless if we've removed the lhs in the current dce pass or in
whatever earlier pass. I think it shouldn't break IFN_*_OVERFLOW, because
maybe_optimize_arith_overflow starts with
tree lhs = gimple_call_lhs (stmt);
if (lhs == NULL_TREE || ...)
return;
Jakub
--- gcc/tree-ssa-dce.c.jj 2017-01-01 12:45:38.380670110 +0100
+++ gcc/tree-ssa-dce.c 2017-01-17 17:35:43.650902141 +0100
@@ -1367,10 +1367,18 @@ eliminate_unnecessary_stmts (void)
update_stmt (stmt);
release_ssa_name (name);
- /* GOMP_SIMD_LANE without lhs is not needed. */
- if (gimple_call_internal_p (stmt)
- && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE)
- remove_dead_stmt (&gsi, bb);
+ /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not
+ needed. */
+ if (gimple_call_internal_p (stmt))
+ switch (gimple_call_internal_fn (stmt))
+ {
+ case IFN_GOMP_SIMD_LANE:
+ case IFN_ASAN_POISON:
+ remove_dead_stmt (&gsi, bb);
+ break;
+ default:
+ break;
+ }
}
else if (gimple_call_internal_p (stmt))
switch (gimple_call_internal_fn (stmt))
--- gcc/tree-ssa-dce.c.jj 2017-01-01 12:45:38.380670110 +0100
+++ gcc/tree-ssa-dce.c 2017-01-17 17:37:38.639427099 +0100
@@ -1366,13 +1366,8 @@ eliminate_unnecessary_stmts (void)
maybe_clean_or_replace_eh_stmt (stmt, stmt);
update_stmt (stmt);
release_ssa_name (name);
-
- /* GOMP_SIMD_LANE without lhs is not needed. */
- if (gimple_call_internal_p (stmt)
- && gimple_call_internal_fn (stmt) == IFN_GOMP_SIMD_LANE)
- remove_dead_stmt (&gsi, bb);
}
- else if (gimple_call_internal_p (stmt))
+ if (gimple_call_internal_p (stmt))
switch (gimple_call_internal_fn (stmt))
{
case IFN_ADD_OVERFLOW:
@@ -1384,6 +1379,13 @@ eliminate_unnecessary_stmts (void)
case IFN_MUL_OVERFLOW:
maybe_optimize_arith_overflow (&gsi, MULT_EXPR);
break;
+ /* GOMP_SIMD_LANE or ASAN_POISON without lhs is not
+ needed. */
+ case IFN_GOMP_SIMD_LANE:
+ case IFN_ASAN_POISON:
+ if (gimple_call_lhs (stmt) == NULL_TREE)
+ remove_dead_stmt (&gsi, bb);
+ break;
default:
break;
}
