[PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Jakub Jelinek]

Hi!

Seems 2 functions in varasm.c just use TREE_PUBLIC on LABEL_DECLs together
with other kinds of decls, but as TREE_PUBLIC on LABEL_DECLs means now
something different, it breaks badly.
While I could change those 2 functions in varasm.c, I'm afraid other
functions might be doing something similar, so I think TREE_PRIVATE which is
used far less often is a better choice for the flag bit here.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-10-12  Jakub Jelinek  <jakub@redhat.com>

	PR c/77946
	* tree.h (FALLTHROUGH_LABEL_P): Use private_flag instead of
	public_flag.
	* varasm.c (default_binds_local_p_3): Formatting fix.

	* c-c++-common/Wimplicit-fallthrough-34.c: New test.

--- gcc/tree.h.jj	2016-10-11 20:50:53.000000000 +0200
+++ gcc/tree.h	2016-10-12 10:14:39.475938668 +0200
@@ -777,7 +777,7 @@ extern void omp_clause_range_check_faile
 /* Whether a case or a user-defined label is allowed to fall through to.
    This is used to implement -Wimplicit-fallthrough.  */
 #define FALLTHROUGH_LABEL_P(NODE) \
-  (LABEL_DECL_CHECK (NODE)->base.public_flag)
+  (LABEL_DECL_CHECK (NODE)->base.private_flag)
 
 /* Nonzero means this expression is volatile in the C sense:
    its address should be of type `volatile WHATEVER *'.
--- gcc/varasm.c.jj	2016-10-09 13:19:09.000000000 +0200
+++ gcc/varasm.c	2016-10-12 10:12:41.617430327 +0200
@@ -6856,8 +6856,8 @@ default_binds_local_p_3 (const_tree exp,
      FIXME: We can resolve the weakref case more curefuly by looking at the
      weakref alias.  */
   if (lookup_attribute ("weakref", DECL_ATTRIBUTES (exp))
-	   || (TREE_CODE (exp) == FUNCTION_DECL
-	       && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
+      || (TREE_CODE (exp) == FUNCTION_DECL
+	  && lookup_attribute ("ifunc", DECL_ATTRIBUTES (exp))))
     return false;
 
   /* Static variables are always local.  */
--- gcc/testsuite/c-c++-common/Wimplicit-fallthrough-34.c.jj	2016-10-12 10:19:30.726252500 +0200
+++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-34.c	2016-10-12 10:19:06.000000000 +0200
@@ -0,0 +1,12 @@
+/* PR c/77946 */
+/* { dg-do compile } */
+/* { dg-options "-Wimplicit-fallthrough" } */
+
+void
+foo (void)
+{
+  static void *p = &&lab;
+  goto *p;
+  /*FALLTHRU*/
+  lab:;
+}

	Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Marek Polacek]

On Thu, Oct 13, 2016 at 01:25:22AM +0200, Jakub Jelinek wrote:
> Hi!
> 
> Seems 2 functions in varasm.c just use TREE_PUBLIC on LABEL_DECLs together
> with other kinds of decls, but as TREE_PUBLIC on LABEL_DECLs means now
> something different, it breaks badly.
> While I could change those 2 functions in varasm.c, I'm afraid other
> functions might be doing something similar, so I think TREE_PRIVATE which is
> used far less often is a better choice for the flag bit here.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
 
Given this is a part of fallthru machinery: LGTM, but can't really approve.

	Marek

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Bernd Schmidt]

On 10/13/2016 01:25 AM, Jakub Jelinek wrote:
> Seems 2 functions in varasm.c just use TREE_PUBLIC on LABEL_DECLs together
> with other kinds of decls, but as TREE_PUBLIC on LABEL_DECLs means now
> something different, it breaks badly.

Which functions are these?

> 	PR c/77946
> 	* tree.h (FALLTHROUGH_LABEL_P): Use private_flag instead of
> 	public_flag.
> 	* varasm.c (default_binds_local_p_3): Formatting fix.
>
> 	* c-c++-common/Wimplicit-fallthrough-34.c: New test.

Ok. Let's hope this one works.


Bernd

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Jakub Jelinek]

On Thu, Oct 13, 2016 at 12:11:36PM +0200, Bernd Schmidt wrote:
> On 10/13/2016 01:25 AM, Jakub Jelinek wrote:
> >Seems 2 functions in varasm.c just use TREE_PUBLIC on LABEL_DECLs together
> >with other kinds of decls, but as TREE_PUBLIC on LABEL_DECLs means now
> >something different, it breaks badly.
> 
> Which functions are these?

The ones I've noticed were:

bool
default_binds_local_p_3 (const_tree exp, bool shlib, bool weak_dominate,
                         bool extern_protected_data, bool common_local_p)
{
  /* A non-decl is an entry in the constant pool.  */
  if (!DECL_P (exp))
    return true;
...
  /* Static variables are always local.  */
  if (! TREE_PUBLIC (exp))
    return true;
...
}

bool
decl_binds_to_current_def_p (const_tree decl)
{
  gcc_assert (DECL_P (decl));
  if (!targetm.binds_local_p (decl))
    return false;
  if (!TREE_PUBLIC (decl))
    return true;
...
}

both relied on TREE_PUBLIC be actually false for LABEL_DECLs, because
otherwise they have code later on that can't handle LABE_DECLs (plus callers
also not expecting LABEL_DECLs might not bind locally or might not bind to
the current def.

	Jakub

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Bernd Schmidt]

On 10/13/2016 12:20 PM, Jakub Jelinek wrote:

> both relied on TREE_PUBLIC be actually false for LABEL_DECLs, because
> otherwise they have code later on that can't handle LABE_DECLs (plus callers
> also not expecting LABEL_DECLs might not bind locally or might not bind to
> the current def.

Ok, thanks. Guess I'll be testing the following:

@@ -6867,6 +6877,7 @@ default_binds_local_p_3 (const_tree exp,
    /* Static variables are always local.  */
    if (! TREE_PUBLIC (exp))
      return true;
+  gcc_assert (TREE_CODE (exp) != LABEL_DECL);

    /* With resolution file in hand, take look into resolutions.
       We can't just return true for resolved_locally symbols,
@@ -6978,6 +6989,7 @@ decl_binds_to_current_def_p (const_tree
      return false;
    if (!TREE_PUBLIC (decl))
      return true;
+  gcc_assert (TREE_CODE (exp) != LABEL_DECL);

    /* When resolution is available, just use it.  */
    if (symtab_node *node = symtab_node::get (decl))


Bernd

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Bernd Schmidt]

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

On 10/13/2016 12:27 PM, Bernd Schmidt wrote:
> On 10/13/2016 12:20 PM, Jakub Jelinek wrote:
>
>> both relied on TREE_PUBLIC be actually false for LABEL_DECLs, because
>> otherwise they have code later on that can't handle LABE_DECLs (plus
>> callers
>> also not expecting LABEL_DECLs might not bind locally or might not
>> bind to
>> the current def.
>
> Ok, thanks. Guess I'll be testing the following:
>
The change below bootstrapped and tested ok on x86_64-linux. Ok to commit?


Bernd


[-- Attachment #2: va-assert.diff --]
[-- Type: text/x-patch, Size: 880 bytes --]

	* varasm.c (default_binds_local_p): Assert we don't have LABEL_DECLs.
	(decl_binds_to_current_def_p): Likewise.

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 240861)
+++ gcc/varasm.c	(working copy)
@@ -6867,6 +6873,7 @@ default_binds_local_p_3 (const_tree exp,
   /* Static variables are always local.  */
   if (! TREE_PUBLIC (exp))
     return true;
+  gcc_assert (TREE_CODE (exp) != LABEL_DECL);
 
   /* With resolution file in hand, take look into resolutions.
      We can't just return true for resolved_locally symbols,
@@ -6978,6 +6985,7 @@ decl_binds_to_current_def_p (const_tree
     return false;
   if (!TREE_PUBLIC (decl))
     return true;
+  gcc_assert (TREE_CODE (decl) != LABEL_DECL);
 
   /* When resolution is available, just use it.  */
   if (symtab_node *node = symtab_node::get (decl))

Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)

[Jakub Jelinek]

On Fri, Oct 14, 2016 at 11:05:22AM +0200, Bernd Schmidt wrote:
> On 10/13/2016 12:27 PM, Bernd Schmidt wrote:
> >On 10/13/2016 12:20 PM, Jakub Jelinek wrote:
> >
> >>both relied on TREE_PUBLIC be actually false for LABEL_DECLs, because
> >>otherwise they have code later on that can't handle LABE_DECLs (plus
> >>callers
> >>also not expecting LABEL_DECLs might not bind locally or might not
> >>bind to
> >>the current def.
> >
> >Ok, thanks. Guess I'll be testing the following:
> >
> The change below bootstrapped and tested ok on x86_64-linux. Ok to commit?

Perhaps gcc_checking_assert would be enough, but if you prefer gcc_assert,
it is ok too.

> 	* varasm.c (default_binds_local_p): Assert we don't have LABEL_DECLs.
> 	(decl_binds_to_current_def_p): Likewise.

	Jakub