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

Message ID 1518192504-49084-1-git-send-email-bonzini@gnu.org
State New
Headers show
Series
  • Improve dead code elimination with -fsanitize=address (PR84307)
Related show

Commit Message

Paolo Bonzini Feb. 9, 2018, 4:08 p.m.
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  <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.

Comments

Richard Biener Feb. 9, 2018, 4:40 p.m. | #1
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);
>+}
Jakub Jelinek Feb. 9, 2018, 4:53 p.m. | #2
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 <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. 

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

	Jakub
Paolo Bonzini Feb. 9, 2018, 5:23 p.m. | #3
On 09/02/2018 17:40, Richard Biener wrote:
> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> 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
Richard Biener Feb. 9, 2018, 6:01 p.m. | #4
On February 9, 2018 6:23:37 PM GMT+01:00, Paolo Bonzini <bonzini@gnu.org> wrote:
>On 09/02/2018 17:40, Richard Biener wrote:
>> On February 9, 2018 5:08:24 PM GMT+01:00, Paolo Bonzini
><bonzini@gnu.org> 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
Jakub Jelinek Feb. 9, 2018, 6:07 p.m. | #5
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
Richard Biener Feb. 9, 2018, 8:10 p.m. | #6
On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> 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
Richard Biener Feb. 12, 2018, 8:56 a.m. | #7
On Fri, Feb 9, 2018 at 9:10 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On February 9, 2018 7:07:45 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> 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
>
Paolo Bonzini Feb. 12, 2018, 12:02 p.m. | #8
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
Jakub Jelinek Feb. 12, 2018, 12:10 p.m. | #9
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
Paolo Bonzini Feb. 12, 2018, 11:41 p.m. | #10
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
Jakub Jelinek Feb. 12, 2018, 11:49 p.m. | #11
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
Martin Liška Feb. 13, 2018, 9:32 a.m. | #12
Hello.

It caused PR84340, I'm suggesting following fix:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3

Does it make sense?
Martin
Paolo Bonzini Feb. 13, 2018, 11:15 a.m. | #13
On 13/02/2018 10:32, Martin Liška wrote:
> Hello.
> 
> It caused PR84340, I'm suggesting following fix:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3

I don't think EAF_DIRECT is the issue.  You could think of ASAN_MARK as
writing a global variable, which it can do because it's not const.

The issue is that the ASAN_CHECK doesn't exist at early DSE time, and
thus causes the store to disappear.

Paolo
Jakub Jelinek Feb. 13, 2018, 11:21 a.m. | #14
On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote:
> On 13/02/2018 10:32, Martin Liška wrote:
> > Hello.
> > 
> > It caused PR84340, I'm suggesting following fix:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3
> 
> I don't think EAF_DIRECT is the issue.  You could think of ASAN_MARK as
> writing a global variable, which it can do because it's not const.
> 
> The issue is that the ASAN_CHECK doesn't exist at early DSE time, and
> thus causes the store to disappear.

That doesn't make sense to me, because the testcases regressed with the change
of "fn spec" attribute on ASAN_{CHECK,MARK}.
If it was DSE removing the stores before asan pass, then it would FAIL
before as well.

	Jakub
Paolo Bonzini Feb. 13, 2018, 12:55 p.m. | #15
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
Jakub Jelinek Feb. 13, 2018, 1 p.m. | #16
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
Jakub Jelinek Feb. 13, 2018, 1:35 p.m. | #17
On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote:
> On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote:
> > On 13/02/2018 10:32, Martin Liška wrote:
> > > Hello.
> > > 
> > > It caused PR84340, I'm suggesting following fix:
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84340#c3
> > 
> > I don't think EAF_DIRECT is the issue.  You could think of ASAN_MARK as
> > writing a global variable, which it can do because it's not const.
> > 
> > The issue is that the ASAN_CHECK doesn't exist at early DSE time, and
> > thus causes the store to disappear.
> 
> That doesn't make sense to me, because the testcases regressed with the change
> of "fn spec" attribute on ASAN_{CHECK,MARK}.
> If it was DSE removing the stores before asan pass, then it would FAIL
> before as well.

Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there
already from the gimplification.

For use after scope, I guess a lot of the stores after end of scope
(rather than reads) are something DSE could consider removing.
So, shall we just disable DSE on vars where their address "escapes"
through ASAN_MARK when -fsanitize-address-use-after-scope?
Generally, dead stores could be eliminable when stored before the
corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will
prevent those) and uneliminable when stored after ASAN_MARK poison.

For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and
NULL for ASAN_MARK for now.

	Jakub
Paolo Bonzini Feb. 13, 2018, 2:29 p.m. | #18
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 &x 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, &var, 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
Jakub Jelinek Feb. 13, 2018, 2:40 p.m. | #19
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  <jakub@redhat.com>

	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
Paolo Bonzini Feb. 13, 2018, 3:22 p.m. | #20
On 13/02/2018 14:35, Jakub Jelinek wrote:
> On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote:
>> On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote:
>>> The issue is that the ASAN_CHECK doesn't exist at early DSE time, and
>>> thus causes the store to disappear.
>>
>> If it was DSE removing the stores before asan pass, then it would FAIL
>> before as well.
> 
> Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there
> already from the gimplification.

Yeah, and it keeps everything alive.

> For use after scope, I guess a lot of the stores after end of scope
> (rather than reads) are something DSE could consider removing.
> So, shall we just disable DSE on vars where their address "escapes"
> through ASAN_MARK when -fsanitize-address-use-after-scope?

But the stores _are_ dead; it's only the ASAN_CHECK that isn't.  Hence
the idea of doing the entire instrumentation very early.

> Generally, dead stores could be eliminable when stored before the
> corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will
> prevent those) and uneliminable when stored after ASAN_MARK poison.
> 
> For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and
> NULL for ASAN_MARK for now.

I'm a bit scared of that even, :) especially in stage4.  If you think
it's safe enough, I can give it a shot, but honestly I wouldn't have
much time to deal with the fallout now (hence the quick revert).

Paolo
Martin Liška Feb. 13, 2018, 4:48 p.m. | #21
On 02/13/2018 04:22 PM, Paolo Bonzini wrote:
> On 13/02/2018 14:35, Jakub Jelinek wrote:
>> On Tue, Feb 13, 2018 at 12:21:55PM +0100, Jakub Jelinek wrote:
>>> On Tue, Feb 13, 2018 at 12:15:36PM +0100, Paolo Bonzini wrote:
>>>> The issue is that the ASAN_CHECK doesn't exist at early DSE time, and
>>>> thus causes the store to disappear.
>>>
>>> If it was DSE removing the stores before asan pass, then it would FAIL
>>> before as well.
>>
>> Sorry, while ASAN_CHECK is introduced late, ASAN_MARK is present there
>> already from the gimplification.
> 
> Yeah, and it keeps everything alive.
> 
>> For use after scope, I guess a lot of the stores after end of scope
>> (rather than reads) are something DSE could consider removing.
>> So, shall we just disable DSE on vars where their address "escapes"
>> through ASAN_MARK when -fsanitize-address-use-after-scope?
> 
> But the stores _are_ dead; it's only the ASAN_CHECK that isn't.  Hence
> the idea of doing the entire instrumentation very early.
> 
>> Generally, dead stores could be eliminable when stored before the
>> corresponding ASAN_MARK poison (but even ASAN_MARK with "..W.." will
>> prevent those) and uneliminable when stored after ASAN_MARK poison.
>>
>> For the "fn spec" for now, I'd just go with "..R.." for ASAN_CHECK and
>> NULL for ASAN_MARK for now.
> 
> I'm a bit scared of that even, :) especially in stage4.  If you think
> it's safe enough, I can give it a shot, but honestly I wouldn't have
> much time to deal with the fallout now (hence the quick revert).

Let me do it tomorrow.

Martin

> 
> Paolo
>
Martin Liška Feb. 16, 2018, 9:53 a.m. | #22
Hi.

I'm sending patch that does what Jakub suggested.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
I consider the previous email as approval and I'm going to install the patch.

Thanks,
Martin
From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 15 Feb 2018 14:41:46 +0100
Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307).

gcc/ChangeLog:

2018-02-16  Martin Liska  <mliska@suse.cz>

	PR sanitizer/84307
	* internal-fn.def (ASAN_CHECK): Set proper flags.
	(ASAN_MARK): Likewise.
---
 gcc/internal-fn.def | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
index 5970d0e472c..4080e1698ea 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, NULL)
 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)
Jakub Jelinek Feb. 16, 2018, 9:55 a.m. | #23
On Fri, Feb 16, 2018 at 10:53:58AM +0100, Martin Liška wrote:
> Hi.
> 
> I'm sending patch that does what Jakub suggested.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> I consider the previous email as approval and I'm going to install the patch.

Ok, thanks.

> >From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001
> From: marxin <mliska@suse.cz>
> Date: Thu, 15 Feb 2018 14:41:46 +0100
> Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307).
> 
> gcc/ChangeLog:
> 
> 2018-02-16  Martin Liska  <mliska@suse.cz>
> 
> 	PR sanitizer/84307
> 	* internal-fn.def (ASAN_CHECK): Set proper flags.
> 	(ASAN_MARK): Likewise.
> ---
>  gcc/internal-fn.def | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
> index 5970d0e472c..4080e1698ea 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, NULL)
>  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)
> -- 
> 2.16.1
> 


	Jakub
Martin Liška Feb. 16, 2018, 10:58 a.m. | #24
On 02/16/2018 10:55 AM, Jakub Jelinek wrote:
> On Fri, Feb 16, 2018 at 10:53:58AM +0100, Martin Liška wrote:
>> Hi.
>>
>> I'm sending patch that does what Jakub suggested.
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>> I consider the previous email as approval and I'm going to install the patch.
> 
> Ok, thanks.
> 
>> >From b306ad522ed0f8a010647d0eb1d9c36b102e92c3 Mon Sep 17 00:00:00 2001
>> From: marxin <mliska@suse.cz>
>> Date: Thu, 15 Feb 2018 14:41:46 +0100
>> Subject: [PATCH] Set proper internal functions fnspec (PR sanitizer/84307).
>>
>> gcc/ChangeLog:
>>
>> 2018-02-16  Martin Liska  <mliska@suse.cz>
>>
>> 	PR sanitizer/84307
>> 	* internal-fn.def (ASAN_CHECK): Set proper flags.
>> 	(ASAN_MARK): Likewise.
>> ---
>>  gcc/internal-fn.def | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def
>> index 5970d0e472c..4080e1698ea 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, NULL)
>>  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)
>> -- 
>> 2.16.1
>>
> 
> 
> 	Jakub
> 

Btw. do we want such patch to be backported?

Martin
Jakub Jelinek Feb. 16, 2018, 10:59 a.m. | #25
On Fri, Feb 16, 2018 at 11:58:07AM +0100, Martin Liška wrote:
> Btw. do we want such patch to be backported?

No.

	Jakub

Patch

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);
+}