Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)
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
Re: [PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)
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 * 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)
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)
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)
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)
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
[PATCH] Fix -Wimplicit-fallthrough ICE (PR c/77946)
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 JelinekPR 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.0 +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.0 +0200 +++ gcc/varasm.c2016-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.jj2016-10-12 10:19:30.726252500 +0200 +++ gcc/testsuite/c-c++-common/Wimplicit-fallthrough-34.c 2016-10-12 10:19:06.0 +0200 @@ -0,0 +1,12 @@ +/* PR c/77946 */ +/* { dg-do compile } */ +/* { dg-options "-Wimplicit-fallthrough" } */ + +void +foo (void) +{ + static void *p = & + goto *p; + /*FALLTHRU*/ + lab:; +} Jakub