Re: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

2016-11-10 Thread Jakub Jelinek 
On Thu, Nov 10, 2016 at 12:02:45PM +0100, Martin Liška wrote:
> >From fb4b852a17656309e6acfb8da97cf9bce4b3b176 Mon Sep 17 00:00:00 2001
> From: marxin 
> Date: Wed, 9 Nov 2016 11:52:00 +0100
> Subject: [PATCH] Create live_switch_vars conditionally (PR sanitizer/78270)
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-11-09  Martin Liska  
> 

Missing
PR sanitizer/78270
note.

>   * gcc.dg/asan/pr78269.c: New test.

Misnamed test, should be pr78270.c.
> 
> gcc/ChangeLog:
> 
> 2016-11-09  Martin Liska  
> 

Also missing the PR line.

>   * gimplify.c (gimplify_switch_expr): Create live_switch_vars
>   only when SWITCH_BODY is a BIND_EXPR.

Ok with those nits fixed.

Jakub

Re: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

2016-11-10 Thread Martin Liška 
On 11/09/2016 02:47 PM, Martin Liška wrote:
> On 11/09/2016 02:29 PM, Jakub Jelinek wrote:
>> On Wed, Nov 09, 2016 at 02:16:45PM +0100, Martin Liška wrote:
>>> As shown in the attached test-case, the assert cannot always be true.
>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>
>>> Ready to be installed?
>>> Martin
>>
>>> >From b55459461f3f7396a094be6801082715ddb4b30d Mon Sep 17 00:00:00 2001
>>> From: marxin <mli...@suse.cz>
>>> Date: Wed, 9 Nov 2016 11:52:00 +0100
>>> Subject: [PATCH] Remove unneeded gcc_assert in gimplifier (PR 
>>> sanitizer/78270)
>>>
>>> gcc/ChangeLog:
>>>
>>> 2016-11-09  Martin Liska  <mli...@suse.cz>
>>>
>>> PR sanitizer/78270
>>> * gimplify.c (gimplify_switch_expr):
>>
>> No description on what you've changed.
>>
>> That said, I'm not 100% sure it is the right fix.
>> As the testcase shows, for switch without GIMPLE_BIND wrapping the body
>> we can have variables that are in scope from the switch onwards.
>> I bet we could also have variables that go out of scope, say if in the
>> compound literal's initializer there is ({ ... }) that declares variables.
>> I doubt you can have a valid case/default label in those though, so
>> perhaps it would be simpler not to create live_switch_vars at all
>> if SWITCH_BODY is not a BIND_EXPR?
> 
> I like the approach you introduced! I'll re-trigger regression tests and
> send a newer version of patch.
> 
> Martin

Sending the patch.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

> 
>>
>>  Jakub
>>
> 

>From fb4b852a17656309e6acfb8da97cf9bce4b3b176 Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 9 Nov 2016 11:52:00 +0100
Subject: [PATCH] Create live_switch_vars conditionally (PR sanitizer/78270)

gcc/testsuite/ChangeLog:

2016-11-09  Martin Liska  <mli...@suse.cz>

	* gcc.dg/asan/pr78269.c: New test.

gcc/ChangeLog:

2016-11-09  Martin Liska  <mli...@suse.cz>

	* gimplify.c (gimplify_switch_expr): Create live_switch_vars
	only when SWITCH_BODY is a BIND_EXPR.
---
 gcc/gimplify.c  | 20 +++-
 gcc/testsuite/gcc.dg/asan/pr78269.c | 13 +
 2 files changed, 28 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pr78269.c

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index d392450..da60c05 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2241,7 +2241,7 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
 {
   vec labels;
   vec saved_labels;
-  hash_set *saved_live_switch_vars;
+  hash_set *saved_live_switch_vars = NULL;
   tree default_case = NULL_TREE;
   gswitch *switch_stmt;
 
@@ -2253,8 +2253,14 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
  labels.  Save all the things from the switch body to append after.  */
   saved_labels = gimplify_ctxp->case_labels;
   gimplify_ctxp->case_labels.create (8);
-  saved_live_switch_vars = gimplify_ctxp->live_switch_vars;
-  gimplify_ctxp->live_switch_vars = new hash_set (4);
+
+  /* Do not create live_switch_vars if SWITCH_BODY is not a BIND_EXPR.  */
+  if (TREE_CODE (SWITCH_BODY (switch_expr)) == BIND_EXPR)
+	{
+	  saved_live_switch_vars = gimplify_ctxp->live_switch_vars;
+	  gimplify_ctxp->live_switch_vars = new hash_set (4);
+	}
+
   bool old_in_switch_expr = gimplify_ctxp->in_switch_expr;
   gimplify_ctxp->in_switch_expr = true;
 
@@ -2269,8 +2275,12 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
 
   labels = gimplify_ctxp->case_labels;
   gimplify_ctxp->case_labels = saved_labels;
-  gcc_assert (gimplify_ctxp->live_switch_vars->elements () == 0);
-  delete gimplify_ctxp->live_switch_vars;
+
+  if (gimplify_ctxp->live_switch_vars)
+	{
+	  gcc_assert (gimplify_ctxp->live_switch_vars->elements () == 0);
+	  delete gimplify_ctxp->live_switch_vars;
+	}
   gimplify_ctxp->live_switch_vars = saved_live_switch_vars;
 
   preprocess_case_label_vec_for_gimple (labels, index_type,
diff --git a/gcc/testsuite/gcc.dg/asan/pr78269.c b/gcc/testsuite/gcc.dg/asan/pr78269.c
new file mode 100644
index 000..55840b0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr78269.c
@@ -0,0 +1,13 @@
+// { dg-do compile }
+// { dg-additional-options "-Wno-switch-unreachable" }
+
+typedef struct
+{
+} bdaddr_t;
+
+int a;
+void fn1 ()
+{
+  switch (a)
+&(bdaddr_t){};
+}
-- 
2.10.1

Re: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

2016-11-09 Thread Martin Liška 
On 11/09/2016 02:29 PM, Jakub Jelinek wrote:
> On Wed, Nov 09, 2016 at 02:16:45PM +0100, Martin Liška wrote:
>> As shown in the attached test-case, the assert cannot always be true.
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>> Martin
> 
>> >From b55459461f3f7396a094be6801082715ddb4b30d Mon Sep 17 00:00:00 2001
>> From: marxin <mli...@suse.cz>
>> Date: Wed, 9 Nov 2016 11:52:00 +0100
>> Subject: [PATCH] Remove unneeded gcc_assert in gimplifier (PR 
>> sanitizer/78270)
>>
>> gcc/ChangeLog:
>>
>> 2016-11-09  Martin Liska  <mli...@suse.cz>
>>
>>  PR sanitizer/78270
>>  * gimplify.c (gimplify_switch_expr):
> 
> No description on what you've changed.
> 
> That said, I'm not 100% sure it is the right fix.
> As the testcase shows, for switch without GIMPLE_BIND wrapping the body
> we can have variables that are in scope from the switch onwards.
> I bet we could also have variables that go out of scope, say if in the
> compound literal's initializer there is ({ ... }) that declares variables.
> I doubt you can have a valid case/default label in those though, so
> perhaps it would be simpler not to create live_switch_vars at all
> if SWITCH_BODY is not a BIND_EXPR?

I like the approach you introduced! I'll re-trigger regression tests and
send a newer version of patch.

Martin

> 
>   Jakub
>

Re: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

2016-11-09 Thread Jakub Jelinek 
On Wed, Nov 09, 2016 at 02:16:45PM +0100, Martin Liška wrote:
> As shown in the attached test-case, the assert cannot always be true.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin

> >From b55459461f3f7396a094be6801082715ddb4b30d Mon Sep 17 00:00:00 2001
> From: marxin <mli...@suse.cz>
> Date: Wed, 9 Nov 2016 11:52:00 +0100
> Subject: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)
> 
> gcc/ChangeLog:
> 
> 2016-11-09  Martin Liska  <mli...@suse.cz>
> 
>   PR sanitizer/78270
>   * gimplify.c (gimplify_switch_expr):

No description on what you've changed.

That said, I'm not 100% sure it is the right fix.
As the testcase shows, for switch without GIMPLE_BIND wrapping the body
we can have variables that are in scope from the switch onwards.
I bet we could also have variables that go out of scope, say if in the
compound literal's initializer there is ({ ... }) that declares variables.
I doubt you can have a valid case/default label in those though, so
perhaps it would be simpler not to create live_switch_vars at all
if SWITCH_BODY is not a BIND_EXPR?

Jakub

[PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

2016-11-09 Thread Martin Liška 
As shown in the attached test-case, the assert cannot always be true.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin
>From b55459461f3f7396a094be6801082715ddb4b30d Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Wed, 9 Nov 2016 11:52:00 +0100
Subject: [PATCH] Remove unneeded gcc_assert in gimplifier (PR sanitizer/78270)

gcc/ChangeLog:

2016-11-09  Martin Liska  <mli...@suse.cz>

	PR sanitizer/78270
	* gimplify.c (gimplify_switch_expr):

gcc/testsuite/ChangeLog:

2016-11-09  Martin Liska  <mli...@suse.cz>

	PR sanitizer/78270
	* gcc.dg/asan/pr78269.c: New test.
---
 gcc/gimplify.c  |  1 -
 gcc/testsuite/gcc.dg/asan/pr78269.c | 13 +
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.dg/asan/pr78269.c

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index e5930e6..15976e2 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2266,7 +2266,6 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pre_p)
 
   labels = gimplify_ctxp->case_labels;
   gimplify_ctxp->case_labels = saved_labels;
-  gcc_assert (gimplify_ctxp->live_switch_vars->elements () == 0);
   delete gimplify_ctxp->live_switch_vars;
   gimplify_ctxp->live_switch_vars = saved_live_switch_vars;
 
diff --git a/gcc/testsuite/gcc.dg/asan/pr78269.c b/gcc/testsuite/gcc.dg/asan/pr78269.c
new file mode 100644
index 000..55840b0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/asan/pr78269.c
@@ -0,0 +1,13 @@
+// { dg-do compile }
+// { dg-additional-options "-Wno-switch-unreachable" }
+
+typedef struct
+{
+} bdaddr_t;
+
+int a;
+void fn1 ()
+{
+  switch (a)
+&(bdaddr_t){};
+}
-- 
2.10.1