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

[Jakub Jelinek]

On Fri, Feb 09, 2018 at 05:08:24PM +0100, Paolo Bonzini wrote:
>   PR sanitizer/84307
>   * gcc.dg/asan/pr84307.c: New test.

BTW, your testcase shows a more severe problem, that we actually don't
handle compound literals correctly.

C99 says that:
"If the compound literal occurs outside the body of a function, the object
has static storage duration; otherwise, it has automatic storage duration
associated with the enclosing block."
but if we create an object with automatic storage duration, we don't
actually put that object into the scope of the enclosing block, but of the
enclosing function, which explains the weird ASAN_MARK UNPOISON present, but
corresponding ASAN_MARK POISON not present.  The following testcase should
IMHO FAIL with -fsanitize=address on the second bar call, but doesn't, even
at -O0 without any DSE.  When optimizing we because of this don't emit
CLOBBER stmts when the compound literal object goes out of scope, and with
-fsanitize=address -fsanitize-address-use-after-scope we don't emit the
POISON.

struct S { int s; } *p;

static inline void
foo (struct S *x)
{
  p = x;
}

static inline void
bar (void)
{
  p->s = 5;
}

int
main ()
{
  {
foo (&(struct S) { 1 });
bar ();
  }
  {
foo (&(struct S) { 2 });
  }
  bar ();
  return 0;
}

The following untested patch seems to cure thatm will see how much it will
break.

2018-02-13  Jakub Jelinek  

PR sanitizer/84340
* c-decl.c (build_compound_literal): Call pushdecl (decl) even when
it is not TREE_STATIC.

--- gcc/c/c-decl.c.jj   2018-01-03 10:20:20.114537949 +0100
+++ gcc/c/c-decl.c  2018-02-13 15:17:47.091186077 +0100
@@ -5348,6 +5348,8 @@ build_compound_literal (location_t loc,
   pushdecl (decl);
   rest_of_decl_compilation (decl, 1, 0);
 }
+  else
+pushdecl (decl);
 
   if (non_const)
 {


Jakub

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

[Paolo Bonzini]

On 13/02/2018 14:00, Jakub Jelinek wrote:
>> Certainly, for now I'll revert.
> Reversion is not the right thing, the "fn spec" attributes were clearly
> incorrect. So, we should change them to something more conservative that
> will work.

That would only be "all dots", that is no fnspec at all.  Martin
suggested removing EAF_DIRECT, but I don't think I agree with his
reasoning.  Besides, aliasing doesn't see the shadow memory at all (see
call_may_clobber_ref_p_1), so it's okay to ignore it for the sake of
fnspecs.

>> But can you expand on why it's too early?  Indeed I suppose it may
>> affect inlining decisions, on the other hand it seems dangerous to apply
>> instrumentation after pretty much any optimization pass.
>
> It will prevent pretty much all optimizations.  We don't want -O2
> -fsanitize=address to be unusably slow, if people want to catch everything,
> they can always use -O0 -fsanitize=address.  The current placement of the
> passes has been a result of long discussions if I remember well.

I'm not sure it will be that bad, together with the fnspec.  Consider
that PR84340 is latent in current GCC; the testcases work because GCC
thinks that the  pointer escaped, and thus treated the stores as not
dead.  In other words, -fsanitize=address -O2 _currently_ lacks an awful
lot of aliasing-based optimizations such as DSE, because all variables
are marked as escaping after the initial ASAN_MARK(UNPOISON, , sz).

With some luck (that we can ascertain between now and stage 1) the
negative effects of pass placement balance with the positive effects of
the fnspec.  But I agree that it requires some discussion and benchmarking.

Paolo

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

[Jakub Jelinek]

On Tue, Feb 13, 2018 at 01:55:23PM +0100, Paolo Bonzini wrote:
> On 13/02/2018 00:49, Jakub Jelinek wrote:
> > On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote:
> >> On 09/02/2018 19:07, Jakub Jelinek wrote:
> >>> On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
> > which indeed fixes the testcase and seems not to break asan.exp.
> 
>  Huh. Need to double check why that makes sense ;) 
> >>>
> >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
> >>> is the second one, the first one is an integer argument with flags.
> >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on the
> >>> referenced variable, before unpoison it is generally inaccessible and 
> >>> after
> >>> poison too.
> >>
> >> This was too optimistic. :(
> >>
> >> In use-after-scope-types-1.C, after the patch FRE+DSE are able to
> >> optimize away the problematic read.  In general it seems to me that the
> >> sanitizer passes should be before DSE if we want ASAN builtins to have
> >> precise info, otherwise some reads or stores might not be
> >> instrumented---GCC was being lucky here.
> >>
> >> The obvious change here is:
> > 
> > That is way too early, I don't think this is a good idea.
> > Certainly not in stage4.
> 
> Certainly, for now I'll revert.

Reversion is not the right thing, the "fn spec" attributes were clearly
incorrect. So, we should change them to something more conservative that
will work.

> But can you expand on why it's too early?  Indeed I suppose it may
> affect inlining decisions, on the other hand it seems dangerous to apply
> instrumentation after pretty much any optimization pass.

It will prevent pretty much all optimizations.  We don't want -O2
-fsanitize=address to be unusably slow, if people want to catch everything,
they can always use -O0 -fsanitize=address.  The current placement of the
passes has been a result of long discussions if I remember well.

Jakub

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

[Paolo Bonzini]

On 13/02/2018 00:49, Jakub Jelinek wrote:
> On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote:
>> On 09/02/2018 19:07, Jakub Jelinek wrote:
>>> On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
> which indeed fixes the testcase and seems not to break asan.exp.

 Huh. Need to double check why that makes sense ;) 
>>>
>>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
>>> is the second one, the first one is an integer argument with flags.
>>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on the
>>> referenced variable, before unpoison it is generally inaccessible and after
>>> poison too.
>>
>> This was too optimistic. :(
>>
>> In use-after-scope-types-1.C, after the patch FRE+DSE are able to
>> optimize away the problematic read.  In general it seems to me that the
>> sanitizer passes should be before DSE if we want ASAN builtins to have
>> precise info, otherwise some reads or stores might not be
>> instrumented---GCC was being lucky here.
>>
>> The obvious change here is:
> 
> That is way too early, I don't think this is a good idea.
> Certainly not in stage4.

Certainly, for now I'll revert.

But can you expand on why it's too early?  Indeed I suppose it may
affect inlining decisions, on the other hand it seems dangerous to apply
instrumentation after pretty much any optimization pass.

Thanks,

Paolo

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

[Jakub Jelinek]

On Tue, Feb 13, 2018 at 12:41:28AM +0100, Paolo Bonzini wrote:
> On 09/02/2018 19:07, Jakub Jelinek wrote:
> > On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
> >>> which indeed fixes the testcase and seems not to break asan.exp.
> >>
> >> Huh. Need to double check why that makes sense ;) 
> > 
> > I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
> > is the second one, the first one is an integer argument with flags.
> > And ASAN_MARK, both poison and unpoison, works kind like a clobber on the
> > referenced variable, before unpoison it is generally inaccessible and after
> > poison too.
> 
> This was too optimistic. :(
> 
> In use-after-scope-types-1.C, after the patch FRE+DSE are able to
> optimize away the problematic read.  In general it seems to me that the
> sanitizer passes should be before DSE if we want ASAN builtins to have
> precise info, otherwise some reads or stores might not be
> instrumented---GCC was being lucky here.
> 
> The obvious change here is:

That is way too early, I don't think this is a good idea.
Certainly not in stage4.

Jakub

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

[Paolo Bonzini]

On 09/02/2018 19:07, Jakub Jelinek wrote:
> On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
>>> which indeed fixes the testcase and seems not to break asan.exp.
>>
>> Huh. Need to double check why that makes sense ;) 
> 
> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
> is the second one, the first one is an integer argument with flags.
> And ASAN_MARK, both poison and unpoison, works kind like a clobber on the
> referenced variable, before unpoison it is generally inaccessible and after
> poison too.

This was too optimistic. :(

In use-after-scope-types-1.C, after the patch FRE+DSE are able to
optimize away the problematic read.  In general it seems to me that the
sanitizer passes should be before DSE if we want ASAN builtins to have
precise info, otherwise some reads or stores might not be
instrumented---GCC was being lucky here.

The obvious change here is:

Index: passes.def
===
--- passes.def  (revision 257584)
+++ passes.def  (working copy)
@@ -95,6 +95,9 @@
  NEXT_PASS (pass_fre);
  NEXT_PASS (pass_early_vrp);
  NEXT_PASS (pass_merge_phi);
+ NEXT_PASS (pass_sancov);
+ NEXT_PASS (pass_asan);
+ NEXT_PASS (pass_tsan);
   NEXT_PASS (pass_dse);
  NEXT_PASS (pass_cd_dce);
  NEXT_PASS (pass_early_ipa_sra);
@@ -259,9 +262,6 @@
   NEXT_PASS (pass_walloca, false);
   NEXT_PASS (pass_pre);
   NEXT_PASS (pass_sink_code);
-  NEXT_PASS (pass_sancov);
-  NEXT_PASS (pass_asan);
-  NEXT_PASS (pass_tsan);
   NEXT_PASS (pass_dce);
   /* Pass group that runs when 1) enabled, 2) there are loops
 in the function.  Make sure to run pass_fix_loops before

which seems to work (this time for real... not sure what went wrong in
my previous testing) but it's a pretty large change that I'd like to run
by you guys before posting it.

Paolo

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

[Jakub Jelinek]

On Mon, Feb 12, 2018 at 01:02:20PM +0100, Paolo Bonzini wrote:
> On 12/02/2018 09:56, Richard Biener wrote:
> >>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
> >>> is the second one, the first one is an integer argument with flags.
> >>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on
> >>> the
> >>> referenced variable, before unpoison it is generally inaccessible and
> >>> after
> >>> poison too.
> >> Ah, indeed.
> > Which was an approval as well, in case you want to push this right now.
> 
> Oh cool.  I was going to look at ubsan builtins too, I'll post that
> separately.  Ok for GCC 7 too?

Please wait with GCC 7 backport at least 2 weeks after it is committed to
trunk.

Jakub

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

[Paolo Bonzini]

On 12/02/2018 09:56, Richard Biener wrote:
>>> I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
>>> is the second one, the first one is an integer argument with flags.
>>> And ASAN_MARK, both poison and unpoison, works kind like a clobber on
>>> the
>>> referenced variable, before unpoison it is generally inaccessible and
>>> after
>>> poison too.
>> Ah, indeed.
> Which was an approval as well, in case you want to push this right now.

Oh cool.  I was going to look at ubsan builtins too, I'll post that
separately.  Ok for GCC 7 too?

Thanks,

Paolo

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

[Richard Biener]

On Fri, Feb 9, 2018 at 9:10 PM, Richard Biener
 wrote:
> On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek  
> wrote:
>>On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
>>> >which indeed fixes the testcase and seems not to break asan.exp.
>>>
>>> Huh. Need to double check why that makes sense ;)
>>
>>I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
>>is the second one, the first one is an integer argument with flags.
>>And ASAN_MARK, both poison and unpoison, works kind like a clobber on
>>the
>>referenced variable, before unpoison it is generally inaccessible and
>>after
>>poison too.
>
> Ah, indeed.

Which was an approval as well, in case you want to push this right now.

Richard.

> Richard.
>
>>   Jakub
>

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

[Richard Biener]

On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek  
wrote:
>On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
>> >which indeed fixes the testcase and seems not to break asan.exp.
>> 
>> Huh. Need to double check why that makes sense ;) 
>
>I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
>is the second one, the first one is an integer argument with flags.
>And ASAN_MARK, both poison and unpoison, works kind like a clobber on
>the
>referenced variable, before unpoison it is generally inaccessible and
>after
>poison too.

Ah, indeed. 

Richard. 

>   Jakub

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

[Jakub Jelinek]

On Fri, Feb 09, 2018 at 07:01:08PM +0100, Richard Biener wrote:
> >which indeed fixes the testcase and seems not to break asan.exp.
> 
> Huh. Need to double check why that makes sense ;) 

I think it does, for both ASAN_CHECK and ASAN_MARK the pointer argument
is the second one, the first one is an integer argument with flags.
And ASAN_MARK, both poison and unpoison, works kind like a clobber on the
referenced variable, before unpoison it is generally inaccessible and after
poison too.

Jakub

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

[Jakub Jelinek]

On Fri, Feb 09, 2018 at 05:40:09PM +0100, Richard Biener wrote:
> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini  
> 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. 

It already uses ".R.." "fn spec".

Jakub

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

[Richard Biener]

On February 9, 2018 6:23:37 PM GMT+01:00, Paolo Bonzini  wrote:
>On 09/02/2018 17:40, Richard Biener wrote:
>> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini
> wrote:
>>> 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. 
>
>Hmm, actually that might be as simple as fixing a typo:
>
>diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
>index 5970d0e..15d6151 100644
>--- a/gcc/internal-fn.def
>+++ b/gcc/internal-fn.def
>@@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW,
>".R.")
> DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)
> DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
>DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW,
>NULL)
>-DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW,
>".R...")
>-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..")
>+DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW,
>"..R..")
>+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.")
>DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS,
>NULL)
>DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS,
>NULL)
>DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW,
>NULL)
>
>which indeed fixes the testcase and seems not to break asan.exp.

Huh. Need to double check why that makes sense ;) 

>'W' is needed to avoid breaking the pr78541.c testcase, and I think it
>makes sense since ASAN_MARK is "writing" the state of the object
>(in the test case FRE moves a dereference across a poisoning).
>
>I'll look at it next week.  Someone maybe should take a look at ubsan
>fnspecs too.
>
>Paolo

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

[Paolo Bonzini]

On 09/02/2018 17:40, Richard Biener wrote:
> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini  
> wrote:
>> 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. 

Hmm, actually that might be as simple as fixing a typo:

diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
index 5970d0e..15d6151 100644
--- a/gcc/internal-fn.def
+++ b/gcc/internal-fn.def
@@ -255,8 +255,8 @@ DEF_INTERNAL_FN (UBSAN_PTR, ECF_LEAF | ECF_NOTHROW, ".R.")
 DEF_INTERNAL_FN (UBSAN_OBJECT_SIZE, ECF_LEAF | ECF_NOTHROW, NULL)
 DEF_INTERNAL_FN (ABNORMAL_DISPATCHER, ECF_NORETURN, NULL)
 DEF_INTERNAL_FN (BUILTIN_EXPECT, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)
-DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, ".R...")
-DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, ".R..")
+DEF_INTERNAL_FN (ASAN_CHECK, ECF_TM_PURE | ECF_LEAF | ECF_NOTHROW, "..R..")
+DEF_INTERNAL_FN (ASAN_MARK, ECF_LEAF | ECF_NOTHROW, "..W.")
 DEF_INTERNAL_FN (ASAN_POISON, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ASAN_POISON_USE, ECF_LEAF | ECF_NOTHROW | ECF_NOVOPS, NULL)
 DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL)

which indeed fixes the testcase and seems not to break asan.exp.
'W' is needed to avoid breaking the pr78541.c testcase, and I think it
makes sense since ASAN_MARK is "writing" the state of the object
(in the test case FRE moves a dereference across a poisoning).

I'll look at it next week.  Someone maybe should take a look at ubsan
fnspecs too.

Paolo

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

[Richard Biener]

On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini  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  
>
>   * 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  
>
>   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 ())
>+  {
>+  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 000..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 

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

[Paolo Bonzini]

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.

Any suggestions on how to proceed here (or approval is welcome too :))?

Paolo

2018-02-09  Paolo Bonzini  

* 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  

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 ())
+  {
+   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 000..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);
+}