On Wed, Nov 05, 2014 at 10:37:46PM +0100, Marek Polacek wrote:
> 2014-11-05 Marek Polacek <pola...@redhat.com>
>
> * sanopt.c (sanopt_optimize_walker): Limit removal of the checks.
> testsuite/
> * c-c++-common/ubsan/align-2.c: Add dg-output.
> * c-c++-common/ubsan/align-4.c: Likewise.
> * c-c++-common/ubsan/align-6.c: New test.
> * c-c++-common/ubsan/align-7.c: New test.
> * c-c++-common/ubsan/align-8.c: New test.
> * g++.dg/ubsan/null-1.C: Add dg-output.
> * g++.dg/ubsan/null-2.C: Likewise.
> * g++.dg/ubsan/null-3.C: New test.
> * g++.dg/ubsan/null-4.C: New test.
> * g++.dg/ubsan/null-5.C: New test.
Looks good, though:
> diff --git gcc/sanopt.c gcc/sanopt.c
> index 4483ff7..cb172d3 100644
> --- gcc/sanopt.c
> +++ gcc/sanopt.c
> @@ -130,7 +130,27 @@ sanopt_optimize_walker (basic_block bb, struct
> sanopt_ctx *ctx)
> /* At this point we shouldn't have any statements
> that aren't dominating the current BB. */
> tree align = gimple_call_arg (g, 2);
> - remove = tree_int_cst_le (cur_align, align);
> + int kind = tree_to_shwi (gimple_call_arg (g, 1));
> + /* If this is a NULL pointer check where we had segv
> + anyway, we can remove it. */
> + if (integer_zerop (align)
> + && (kind == UBSAN_LOAD_OF
> + || kind == UBSAN_STORE_OF
> + || kind == UBSAN_MEMBER_ACCESS))
> + remove = true;
> + /* Otherwise remove the check in non-recovering
> + mode, or if the stmts have same location. */
> + else if (integer_zerop (align))
> + remove = !(flag_sanitize_recover & SANITIZE_NULL)
> + || flag_sanitize_undefined_trap_on_error
> + || gimple_location (g)
> + == gimple_location (stmt);
> + else if (tree_int_cst_le (cur_align, align))
> + remove = !(flag_sanitize_recover
> + & SANITIZE_ALIGNMENT)
> + || flag_sanitize_undefined_trap_on_error
> + || gimple_location (g)
> + == gimple_location (stmt);
I wonder if for the case where you don't remove, gimple_bb (g) == gimple_bb
(stmt) and tree_int_cst_compare (cur_align, align) == 0 it wouldn't be
worthwhile to v.pop (); so that the vector doesn't grow up unnecessarily too
much. Maybe also bump the 30 limit to 100-200? Now that you don't walk the
whole vector each time, it is only a matter of memory consumption (perhaps
instead of having a constant limit for every vector it might be better to
just have a limit on the total size of all the sanopt vectors (say a few
megabytes)? And count towards that limit the allocated () size of the
vector, once you have some size allocated, not pushing stuff in there
doesn't make you consume more memory. Or just don't have a limit at all.
Jakub
