diff mbox series

Fix fallthrough attribute ignorance w/ -fsanitize=address (PR sanitizer/82792).

Message ID a0c79867-a755-fcb8-e2a7-a7ca0d2aca66@suse.cz
State New
Headers show
Series Fix fallthrough attribute ignorance w/ -fsanitize=address (PR sanitizer/82792). | expand

Commit Message

Martin Liška Nov. 8, 2017, 7:07 a.m. UTC 
Hello.

Quite simple patch that ignores IFN_ASAN_CHECK call that is introduced in gimplifier.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

gcc/ChangeLog:

2017-11-07  Martin Liska  <mliska@suse.cz>

	PR sanitizer/82792
	* gimplify.c (expand_FALLTHROUGH_r): Skip IFN_ASAN_MARK.

gcc/testsuite/ChangeLog:

2017-11-07  Martin Liska  <mliska@suse.cz>

	PR sanitizer/82792
	* g++.dg/asan/pr82792.C: New test.
---
 gcc/gimplify.c                      |  8 ++++++--
 gcc/testsuite/g++.dg/asan/pr82792.C | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/asan/pr82792.C

Comments

Jakub Jelinek Nov. 8, 2017, 7:47 a.m. UTC | #1 
On Wed, Nov 08, 2017 at 08:07:42AM +0100, Martin Liška wrote:
> Hello.
> 
> Quite simple patch that ignores IFN_ASAN_CHECK call that is introduced in gimplifier.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin
> 
> gcc/ChangeLog:
> 
> 2017-11-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR sanitizer/82792
> 	* gimplify.c (expand_FALLTHROUGH_r): Skip IFN_ASAN_MARK.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-11-07  Martin Liska  <mliska@suse.cz>
> 
> 	PR sanitizer/82792
> 	* g++.dg/asan/pr82792.C: New test.

Ok, thanks.

	Jakub
Marek Polacek Nov. 8, 2017, 8:37 a.m. UTC | #2 
On Wed, Nov 08, 2017 at 08:07:42AM +0100, Martin Liška wrote:
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index c4fd5038d92..9563d825a6a 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -2223,7 +2223,8 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>  	  while (!gsi_end_p (gsi2))
>  	    {
>  	      stmt = gsi_stmt (gsi2);
> -	      if (gimple_code (stmt) == GIMPLE_LABEL)
> +	      enum gimple_code gc = gimple_code (stmt);
> +	      if (gc == GIMPLE_LABEL)
>  		{
>  		  tree label = gimple_label_label (as_a <glabel *> (stmt));
>  		  if (gimple_has_location (stmt) && DECL_ARTIFICIAL (label))
> @@ -2232,8 +2233,11 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>  		      break;
>  		    }
>  		}
> +	      else if (gc == GIMPLE_CALL
> +		       && gimple_call_internal_p (stmt, IFN_ASAN_MARK))
> +		;

I thought you could simply use gimple_call_internal_p (stmt, IFN_ASAN_MARK)
here.

> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/asan/pr82792.C
> @@ -0,0 +1,32 @@
> +/* PR sanitizer/82792 */
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize=address" } */

And shouldn't this use -Wextra or -Wimplicit-fallthrough to check whether
the warning is really gone?

	Marek
Martin Liška Nov. 8, 2017, 11:17 a.m. UTC | #3 
On 11/08/2017 09:37 AM, Marek Polacek wrote:
> On Wed, Nov 08, 2017 at 08:07:42AM +0100, Martin Liška wrote:
>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>> index c4fd5038d92..9563d825a6a 100644
>> --- a/gcc/gimplify.c
>> +++ b/gcc/gimplify.c
>> @@ -2223,7 +2223,8 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>>  	  while (!gsi_end_p (gsi2))
>>  	    {
>>  	      stmt = gsi_stmt (gsi2);
>> -	      if (gimple_code (stmt) == GIMPLE_LABEL)
>> +	      enum gimple_code gc = gimple_code (stmt);
>> +	      if (gc == GIMPLE_LABEL)
>>  		{
>>  		  tree label = gimple_label_label (as_a <glabel *> (stmt));
>>  		  if (gimple_has_location (stmt) && DECL_ARTIFICIAL (label))
>> @@ -2232,8 +2233,11 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
>>  		      break;
>>  		    }
>>  		}
>> +	      else if (gc == GIMPLE_CALL
>> +		       && gimple_call_internal_p (stmt, IFN_ASAN_MARK))
>> +		;
> 
> I thought you could simply use gimple_call_internal_p (stmt, IFN_ASAN_MARK)
> here.

Hi Marek.

You're right! Are you fine with attached patch?



> 
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/asan/pr82792.C
>> @@ -0,0 +1,32 @@
>> +/* PR sanitizer/82792 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-fsanitize=address" } */
> 
> And shouldn't this use -Wextra or -Wimplicit-fallthrough to check whether
> the warning is really gone?

I don't think so:

/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/pr82792.C: In function ‘int test(int, int)’:
/home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/pr82792.C:25:7: warning: attribute ‘fallthrough’ not preceding a case label or default label
       __attribute ((fallthrough));
       ^~~~~~~~~~~

It's not driven by any -W...

Martin

> 
> 	Marek
>
From 2ce0450a68bc8024ab4cbc4eeb867fdeb1579a04 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 8 Nov 2017 12:15:50 +0100
Subject: [PATCH] Simplify call of gimple_call_internal_p.

gcc/ChangeLog:

2017-11-08  Martin Liska  <mliska@suse.cz>

	* gimplify.c (expand_FALLTHROUGH_r): Simplify usage
	of gimple_call_internal_p.
---
 gcc/gimplify.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 9563d825a6a..e9168785fc0 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2223,8 +2223,7 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 	  while (!gsi_end_p (gsi2))
 	    {
 	      stmt = gsi_stmt (gsi2);
-	      enum gimple_code gc = gimple_code (stmt);
-	      if (gc == GIMPLE_LABEL)
+	      if (gimple_code (stmt) == GIMPLE_LABEL)
 		{
 		  tree label = gimple_label_label (as_a <glabel *> (stmt));
 		  if (gimple_has_location (stmt) && DECL_ARTIFICIAL (label))
@@ -2233,8 +2232,7 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 		      break;
 		    }
 		}
-	      else if (gc == GIMPLE_CALL
-		       && gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	      else if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
 		;
 	      else
 		/* Something other is not expected.  */
Marek Polacek Nov. 8, 2017, 11:42 a.m. UTC | #4 
On Wed, Nov 08, 2017 at 12:17:29PM +0100, Martin Liška wrote:
> On 11/08/2017 09:37 AM, Marek Polacek wrote:
> > On Wed, Nov 08, 2017 at 08:07:42AM +0100, Martin Liška wrote:
> >> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> >> index c4fd5038d92..9563d825a6a 100644
> >> --- a/gcc/gimplify.c
> >> +++ b/gcc/gimplify.c
> >> @@ -2223,7 +2223,8 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> >>  	  while (!gsi_end_p (gsi2))
> >>  	    {
> >>  	      stmt = gsi_stmt (gsi2);
> >> -	      if (gimple_code (stmt) == GIMPLE_LABEL)
> >> +	      enum gimple_code gc = gimple_code (stmt);
> >> +	      if (gc == GIMPLE_LABEL)
> >>  		{
> >>  		  tree label = gimple_label_label (as_a <glabel *> (stmt));
> >>  		  if (gimple_has_location (stmt) && DECL_ARTIFICIAL (label))
> >> @@ -2232,8 +2233,11 @@ expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
> >>  		      break;
> >>  		    }
> >>  		}
> >> +	      else if (gc == GIMPLE_CALL
> >> +		       && gimple_call_internal_p (stmt, IFN_ASAN_MARK))
> >> +		;
> > 
> > I thought you could simply use gimple_call_internal_p (stmt, IFN_ASAN_MARK)
> > here.
> 
> Hi Marek.
> 
> You're right! Are you fine with attached patch?

Yes.

> > And shouldn't this use -Wextra or -Wimplicit-fallthrough to check whether
> > the warning is really gone?
> 
> I don't think so:
> 
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/pr82792.C: In function ‘int test(int, int)’:
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/asan/pr82792.C:25:7: warning: attribute ‘fallthrough’ not preceding a case label or default label
>        __attribute ((fallthrough));
>        ^~~~~~~~~~~

Oh, I see now.

	Marek
diff mbox series

Patch

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c4fd5038d92..9563d825a6a 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -2223,7 +2223,8 @@  expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 	  while (!gsi_end_p (gsi2))
 	    {
 	      stmt = gsi_stmt (gsi2);
-	      if (gimple_code (stmt) == GIMPLE_LABEL)
+	      enum gimple_code gc = gimple_code (stmt);
+	      if (gc == GIMPLE_LABEL)
 		{
 		  tree label = gimple_label_label (as_a <glabel *> (stmt));
 		  if (gimple_has_location (stmt) && DECL_ARTIFICIAL (label))
@@ -2232,8 +2233,11 @@  expand_FALLTHROUGH_r (gimple_stmt_iterator *gsi_p, bool *handled_ops_p,
 		      break;
 		    }
 		}
+	      else if (gc == GIMPLE_CALL
+		       && gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+		;
 	      else
-		/* Something other than a label.  That's not expected.  */
+		/* Something other is not expected.  */
 		break;
 	      gsi_next (&gsi2);
 	    }
diff --git a/gcc/testsuite/g++.dg/asan/pr82792.C b/gcc/testsuite/g++.dg/asan/pr82792.C
new file mode 100644
index 00000000000..99f1c35328c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/pr82792.C
@@ -0,0 +1,32 @@ 
+/* PR sanitizer/82792 */
+/* { dg-do compile } */
+/* { dg-options "-fsanitize=address" } */
+
+extern int
+test (int i, int j)
+{
+  long c;
+  (c) = 1;
+  switch (i)
+    {
+    case 1:
+      if (j)
+	{
+	  c = 1;
+	}
+      goto default_case;
+    case 2:
+      {
+	if (j)
+	  {
+	    c = 0;
+	  }
+      }
+      __attribute ((fallthrough));
+    default_case:
+    default:
+      c = 0;
+      break;
+    }
+  return 0;
+}