Re: [PATCH] Improve dead code elimination with -fsanitize=address (PR84307)

[Richard Biener <richard dot guenther at gmail dot com>]

On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote:
>Hi all,
>
>in this PR, a dead reference to a function pointer cannot be optimized
>out by the compiler because some ASAN_MARK UNPOISON calls, which are
>placed before the store, cause the containing struct to escape.
>(Without -fsanitize=address, the dead code is eliminated by the first
>DSE pass).
>
>The fix, which works at least for this testcase, is to copy part of the
>sanopt dead code elimination pass early, so that the compiler can see
>fewer UNPOISON calls.  I am not sure this is general enough, due to
>the very limited data flow analysis done by
>sanitize_asan_mark_unpoison.
>Another possibility which I considered but did not implement is to mark
>the UNPOISON calls so that they do not cause the parameter to escape.

I'd do this, thus assign proper fnspec attributes to the asan functions. 

Richard. 

>Any suggestions on how to proceed here (or approval is welcome too :))?
>
>Paolo
>
>2018-02-09  Paolo Bonzini  <bonzini@gnu.org>
>
>	* passes.def: add pass_sandce before first DSE pass.
>	* sanopt.c (pass_data_sandce, pass_sandce, make_pass_sandce): New.
>	* tree-pass.h (make_pass_sandce): Declare it.
>
>testsuite:
>2018-02-09  Paolo Bonzini  <bonzini@gnu.org>
>
>	PR sanitizer/84307
>	* gcc.dg/asan/pr84307.c: New test.
>
>diff --git a/gcc/passes.def b/gcc/passes.def
>index 9802f08..180df50 100644
>--- a/gcc/passes.def
>+++ b/gcc/passes.def
>@@ -244,6 +244,7 @@ along with GCC; see the file COPYING3.  If not see
> 	 only examines PHIs to discover const/copy propagation
> 	 opportunities.  */
>       NEXT_PASS (pass_phi_only_cprop);
>+      NEXT_PASS (pass_sandce);
>       NEXT_PASS (pass_dse);
>       NEXT_PASS (pass_reassoc, true /* insert_powi_p */);
>       NEXT_PASS (pass_dce);
>diff --git a/gcc/sanopt.c b/gcc/sanopt.c
>index cd94638..23b8e6e 100644
>--- a/gcc/sanopt.c
>+++ b/gcc/sanopt.c
>@@ -906,6 +906,32 @@ sanopt_optimize (function *fun, bool
>*contains_asan_mark)
> 
> namespace {
> 
>+const pass_data pass_data_sandce =
>+{
>+  GIMPLE_PASS, /* type */
>+  "sandce", /* name */
>+  OPTGROUP_NONE, /* optinfo_flags */
>+  TV_NONE, /* tv_id */
>+  ( PROP_ssa | PROP_cfg | PROP_gimple_leh ), /* properties_required */
>+  0, /* properties_provided */
>+  0, /* properties_destroyed */
>+  0, /* todo_flags_start */
>+  TODO_update_ssa, /* todo_flags_finish */
>+};
>+
>+class pass_sandce : public gimple_opt_pass
>+{
>+public:
>+  pass_sandce (gcc::context *ctxt)
>+    : gimple_opt_pass (pass_data_sandce, ctxt)
>+  {}
>+
>+  /* opt_pass methods: */
>+  virtual bool gate (function *) { return flag_sanitize &
>SANITIZE_ADDRESS; }
>+  virtual unsigned int execute (function *);
>+
>+}; // class pass_sanopt
>+
> const pass_data pass_data_sanopt =
> {
>   GIMPLE_PASS, /* type */
>@@ -1244,6 +1270,31 @@ sanitize_rewrite_addressable_params (function
>*fun)
> }
> 
> unsigned int
>+pass_sandce::execute (function *fun)
>+{
>+  basic_block bb;
>+  bool contains_asan_mark = false;
>+
>+  /* Try to remove redundant unpoisoning.  */
>+  gimple_stmt_iterator gsi;
>+  FOR_EACH_BB_FN (bb, fun)
>+    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>+      {
>+	gimple *stmt = gsi_stmt (gsi);
>+    	if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
>+    	  {
>+    	    contains_asan_mark = true;
>+    	    break;
>+	  }
>+      }
>+
>+  if (contains_asan_mark)
>+    sanitize_asan_mark_unpoison ();
>+
>+  return 0;
>+}
>+
>+unsigned int
> pass_sanopt::execute (function *fun)
> {
>   basic_block bb;
>@@ -1367,6 +1418,12 @@ pass_sanopt::execute (function *fun)
> } // anon namespace
> 
> gimple_opt_pass *
>+make_pass_sandce (gcc::context *ctxt)
>+{
>+  return new pass_sandce (ctxt);
>+}
>+
>+gimple_opt_pass *
> make_pass_sanopt (gcc::context *ctxt)
> {
>   return new pass_sanopt (ctxt);
>diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
>index 93a6a99..d5eb055 100644
>--- a/gcc/tree-pass.h
>+++ b/gcc/tree-pass.h
>@@ -350,6 +350,7 @@ extern gimple_opt_pass *make_pass_tsan
>(gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_tsan_O0 (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_sancov (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_sancov_O0 (gcc::context *ctxt);
>+extern gimple_opt_pass *make_pass_sandce (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_lower_cf (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_refactor_eh (gcc::context *ctxt);
> extern gimple_opt_pass *make_pass_lower_eh (gcc::context *ctxt);
>diff --git a/gcc/testsuite/gcc.dg/asan/pr84307.c
>b/gcc/testsuite/gcc.dg/asan/pr84307.c
>new file mode 100644
>index 0000000..6e1a197
>--- /dev/null
>+++ b/gcc/testsuite/gcc.dg/asan/pr84307.c
>@@ -0,0 +1,21 @@
>+/* PR middle-end/83185 */
>+/* { dg-do link } */
>+/* { dg-options "-O1" } */
>+
>+struct f {
>+  void (*func)(void);
>+};
>+
>+extern void link_error(void);
>+extern int printf(const char *f, ...);
>+
>+static inline struct f *gimme_null(struct f *result)
>+{
>+  return 0;
>+}
>+
>+int main(int argc, char **argv)
>+{
>+  struct f *x = gimme_null(&(struct f) { .func = link_error });
>+  printf("%p", x);
>+}