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

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

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

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 <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

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 <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

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)

[Richard Biener]

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

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

[Richard Biener]

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
>

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)

[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

Fallout: PR84340

[Martin Liška]

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

Re: Fallout: PR84340

[Paolo Bonzini]

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

Re: Fallout: PR84340

[Jakub Jelinek]

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

Re: Fallout: PR84340

[Jakub Jelinek]

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

Re: Fallout: PR84340

[Paolo Bonzini]

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

Re: Fallout: PR84340

[Martin Liška]

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
> 

Re: Fallout: PR84340

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 224 bytes --]

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

[-- Attachment #2: 0001-Set-proper-internal-functions-fnspec-PR-sanitizer-84.patch --]
[-- Type: text/x-patch, Size: 1393 bytes --]

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

Re: Fallout: PR84340

[Jakub Jelinek]

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

Re: Fallout: PR84340

[Martin Liška]

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

Re: Fallout: PR84340

[Jakub Jelinek]

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

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 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 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 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 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

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 <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

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  <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

[PATCH] Fix up compound literal handling in C FE (PR sanitizer/84307)

[Jakub Jelinek]

On Tue, Feb 13, 2018 at 03:40:20PM +0100, Jakub Jelinek wrote:
> 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.

Here is the full patch that passed bootstrap/regtest on x86_64-linux and
i686-linux, ok for stage1?

The mark_addressable part is needed to fix
gcc.c-torture/compile/compound-literal-3.c, where we marked the underlying
automatic temporary of the compound literal as addressable too late and
the gimplifier already set DECL_GIMPLE_REG_P on it when seeing it in the
block scope.

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.
	* c-typeck.c (c_mark_addressable) <case COMPOUND_LITERAL_EXPR>: Mark
	not just the COMPOUND_LITERAL_EXPR node itself addressable, but also
	its COMPOUND_LITERAL_EXPR_DECL.

--- gcc/c/c-decl.c.jj	2018-02-13 21:21:35.447978191 +0100
+++ gcc/c/c-decl.c	2018-02-13 22:13:02.030148159 +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)
     {
--- gcc/c/c-typeck.c.jj	2018-02-10 00:15:36.398163493 +0100
+++ gcc/c/c-typeck.c	2018-02-13 22:24:06.294104876 +0100
@@ -4821,6 +4821,10 @@ c_mark_addressable (tree exp, bool array
 	break;
 
       case COMPOUND_LITERAL_EXPR:
+	TREE_ADDRESSABLE (x) = 1;
+	TREE_ADDRESSABLE (COMPOUND_LITERAL_EXPR_DECL (x)) = 1;
+	return true;
+
       case CONSTRUCTOR:
 	TREE_ADDRESSABLE (x) = 1;
 	return true;


	Jakub

Re: [PATCH] Fix up compound literal handling in C FE (PR sanitizer/84307)

[Joseph Myers]

On Wed, 14 Feb 2018, Jakub Jelinek wrote:

> 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.
> 	* c-typeck.c (c_mark_addressable) <case COMPOUND_LITERAL_EXPR>: Mark
> 	not just the COMPOUND_LITERAL_EXPR node itself addressable, but also
> 	its COMPOUND_LITERAL_EXPR_DECL.

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix up compound literal handling in C FE (PR sanitizer/84307)

[Martin Liška]

On 02/14/2018 11:03 PM, Joseph Myers wrote:
> On Wed, 14 Feb 2018, Jakub Jelinek wrote:
> 
>> 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.
>> 	* c-typeck.c (c_mark_addressable) <case COMPOUND_LITERAL_EXPR>: Mark
>> 	not just the COMPOUND_LITERAL_EXPR node itself addressable, but also
>> 	its COMPOUND_LITERAL_EXPR_DECL.
> 
> OK.
> 

Jakub you probably forgot to install the patch. Can you please do it?
Should I create a new test-case which you mentioned in previous email?

Martin

Re: [PATCH] Fix up compound literal handling in C FE (PR sanitizer/84307)

[Jakub Jelinek]

On Mon, Feb 26, 2018 at 11:00:48AM +0100, Martin Liška wrote:
> On 02/14/2018 11:03 PM, Joseph Myers wrote:
> > On Wed, 14 Feb 2018, Jakub Jelinek wrote:
> > 
> >> 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.
> >> 	* c-typeck.c (c_mark_addressable) <case COMPOUND_LITERAL_EXPR>: Mark
> >> 	not just the COMPOUND_LITERAL_EXPR node itself addressable, but also
> >> 	its COMPOUND_LITERAL_EXPR_DECL.
> > 
> > OK.
> > 
> 
> Jakub you probably forgot to install the patch. Can you please do it?

No, I was asking for ack for stage1, so it is waiting for GCC9.
This isn't really a regression and feels risky for GCC8 at this stage, might
break invalid programs that try to use compound literals after they go out
of scope.

	Jakub

Re: [PATCH] Fix up compound literal handling in C FE (PR sanitizer/84307)

[Martin Liška]

On 02/26/2018 11:04 AM, Jakub Jelinek wrote:
> On Mon, Feb 26, 2018 at 11:00:48AM +0100, Martin Liška wrote:
>> On 02/14/2018 11:03 PM, Joseph Myers wrote:
>>> On Wed, 14 Feb 2018, Jakub Jelinek wrote:
>>>
>>>> 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.
>>>> 	* c-typeck.c (c_mark_addressable) <case COMPOUND_LITERAL_EXPR>: Mark
>>>> 	not just the COMPOUND_LITERAL_EXPR node itself addressable, but also
>>>> 	its COMPOUND_LITERAL_EXPR_DECL.
>>>
>>> OK.
>>>
>>
>> Jakub you probably forgot to install the patch. Can you please do it?
> 
> No, I was asking for ack for stage1, so it is waiting for GCC9.
> This isn't really a regression and feels risky for GCC8 at this stage, might
> break invalid programs that try to use compound literals after they go out
> of scope.
> 
> 	Jakub
> 

Ah, I see!

Thanks,
Martin