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
