Re: PR c/43288 (ICE with __attribute__ ((common)))

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Mar 10, 2010 at 4:25 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> we now get ICE on the following:
> static int a __attribute__ ((common));
> because of sanity check in function_and_variable_visibility. ?Declaring the
> code invalid in handle_common_attribute is not enough, since the static and
> common can be merged together.
>
> It would be probably feasible to test this both at at handle_common_attribute
> and declaratio nmerging, this is just one of cases where frontend produce
> nonsential visibility flag that we regularize in function_and_variable_visibility.
>
> So instead of doing that I added code there. Problem is that varasm already
> does this regularization at two places and turning those into ICEs. ?These
> regularizations happen too late, and invalid combinations are still getting
> through the IPA passes. ?However turnging these to ICE also shows that
> assemble_alias might enforce this to happen early (i.e. before visibility
> pass). ?This is problem too: before visibility pass we should not produce
> DECL_RTLs. ?(visibility pass changes visibilities for i.e. -fwhole-program and
> computing DECL_RTL early will result in wrong assembly).
>
> The patch thus moves the DECL_RTL generation after visibility pass
>
> Bootstrapped/regsted at x86_64-linux and also at alphaev67-dec-osf5 (where patch
> was reported to cause bootstrap ICE earlier).
>
> OK for mainline? (with the testcase above)

Ok.

Thanks,
Richard.

> Honza
>
> ? ? ? ?PR c/43288
> ? ? ? ?* ipa.c (function_and_variable_visibility) Normalize COMMON bits.
> ? ? ? ?* varasm.c (get_variable_section): Don't do that here...
> ? ? ? ?(make_decl_rtl): ... and here.
> ? ? ? ?(do_assemble_alias): Produce decl RTL.
> ? ? ? ?(assemble_alias): Likewise.
> Index: ipa.c
> ===================================================================
> *** ipa.c ? ? ? (revision 155961)
> --- ipa.c ? ? ? (working copy)
> *************** function_and_variable_visibility (bool w
> *** 409,420 ****
> ? ? ? ? ? ? ? ? ? ? ? ? ? && !DECL_EXTERNAL (node->decl)
> ? ? ? ? ? ? ? ? ? ? ? ? ? && !node->local.externally_visible);
> ? ? ?}
> ? ?for (vnode = varpool_nodes_queue; vnode; vnode = vnode->next_needed)
> ? ? ?{
> ? ? ? ?if (!vnode->finalized)
> ? ? ? ? ?continue;
> - ? ? ? gcc_assert ((!DECL_WEAK (vnode->decl) && !DECL_COMMON (vnode->decl))
> - ? ? ? ? ? ? ? ? ? ? ? ? || TREE_PUBLIC (vnode->decl) || DECL_EXTERNAL (vnode->decl));
> ? ? ? ?if (vnode->needed
> ? ? ? ? ?&& (DECL_COMDAT (vnode->decl) || TREE_PUBLIC (vnode->decl))
> ? ? ? ? ?&& (!whole_program
> --- 409,446 ----
> ? ? ? ? ? ? ? ? ? ? ? ? ? && !DECL_EXTERNAL (node->decl)
> ? ? ? ? ? ? ? ? ? ? ? ? ? && !node->local.externally_visible);
> ? ? ?}
> + ? for (vnode = varpool_nodes; vnode; vnode = vnode->next)
> + ? ? {
> + ? ? ? /* weak flag makes no sense on local variables. ?*/
> + ? ? ? gcc_assert (!DECL_WEAK (vnode->decl)
> + ? ? ? ? ? ? ? ? ? ? ? ? || TREE_PUBLIC (vnode->decl) || DECL_EXTERNAL (vnode->decl));
> + ? ? ? /* In several cases declarations can not be common:
> +
> + ? ? ? ?- when declaration has initializer
> + ? ? ? ?- when it is in weak
> + ? ? ? ?- when it has specific section
> + ? ? ? ?- when it resides in non-generic address space.
> + ? ? ? ?- if declaration is local, it will get into .local common section
> + ? ? ? ? ?so common flag is not needed. ?Frontends still produce these in
> + ? ? ? ? ?certain cases, such as for:
> +
> + ? ? ? ? ? ?static int a __attribute__ ((common))
> +
> + ? ? ? ?Canonicalize things here and clear the redundant flag. ?*/
> + ? ? ? if (DECL_COMMON (vnode->decl)
> + ? ? ? ? && (!(TREE_PUBLIC (vnode->decl) || DECL_EXTERNAL (vnode->decl))
> + ? ? ? ? ? ? || (DECL_INITIAL (vnode->decl)
> + ? ? ? ? ? ? ? ? && DECL_INITIAL (vnode->decl) != error_mark_node)
> + ? ? ? ? ? ? || DECL_WEAK (vnode->decl)
> + ? ? ? ? ? ? || DECL_SECTION_NAME (vnode->decl) != NULL
> + ? ? ? ? ? ? || ! (ADDR_SPACE_GENERIC_P
> + ? ? ? ? ? ? ? ? ? (TYPE_ADDR_SPACE (TREE_TYPE (vnode->decl))))))
> + ? ? ? DECL_COMMON (vnode->decl) = 0;
> + ? ? }
> ? ?for (vnode = varpool_nodes_queue; vnode; vnode = vnode->next_needed)
> ? ? ?{
> ? ? ? ?if (!vnode->finalized)
> ? ? ? ? ?continue;
> ? ? ? ?if (vnode->needed
> ? ? ? ? ?&& (DECL_COMDAT (vnode->decl) || TREE_PUBLIC (vnode->decl))
> ? ? ? ? ?&& (!whole_program
> Index: varasm.c
> ===================================================================
> *** varasm.c ? ?(revision 155961)
> --- varasm.c ? ?(working copy)
> *************** get_variable_section (tree decl, bool pr
> *** 1174,1185 ****
> ? ?if (TREE_TYPE (decl) != error_mark_node)
> ? ? ?as = TYPE_ADDR_SPACE (TREE_TYPE (decl));
>
> ! ? /* If the decl has been given an explicit section name, or it resides
> ! ? ? ?in a non-generic address space, then it isn't common, and shouldn't
> ! ? ? ?be handled as such. ?*/
> ! ? if (DECL_COMMON (decl) && DECL_SECTION_NAME (decl) == NULL
> ! ? ? ? && ADDR_SPACE_GENERIC_P (as))
> ? ? ?{
> ? ? ? ?if (DECL_THREAD_LOCAL_P (decl))
> ? ? ? ?return tls_comm_section;
> ? ? ? ?/* This cannot be common bss for an emulated TLS object without
> --- 1174,1186 ----
> ? ?if (TREE_TYPE (decl) != error_mark_node)
> ? ? ?as = TYPE_ADDR_SPACE (TREE_TYPE (decl));
>
> ! ? if (DECL_COMMON (decl))
> ? ? ?{
> + ? ? ? /* If the decl has been given an explicit section name, or it resides
> + ? ? ? ?in a non-generic address space, then it isn't common, and shouldn't
> + ? ? ? ?be handled as such. ?*/
> + ? ? ? gcc_assert (DECL_SECTION_NAME (decl) == NULL
> + ? ? ? ? ? ? ? ? && ADDR_SPACE_GENERIC_P (as));
> ? ? ? ?if (DECL_THREAD_LOCAL_P (decl))
> ? ? ? ?return tls_comm_section;
> ? ? ? ?/* This cannot be common bss for an emulated TLS object without
> *************** make_decl_rtl (tree decl)
> *** 1434,1448 ****
>
> ? ?/* Specifying a section attribute on a variable forces it into a
> ? ? ? non-.bss section, and thus it cannot be common. ?*/
> ! ? if (TREE_CODE (decl) == VAR_DECL
> ! ? ? ? && DECL_SECTION_NAME (decl) != NULL_TREE
> ! ? ? ? && DECL_INITIAL (decl) == NULL_TREE
> ! ? ? ? && DECL_COMMON (decl))
> ! ? ? DECL_COMMON (decl) = 0;
>
> ? ?/* Variables can't be both common and weak. ?*/
> ! ? if (TREE_CODE (decl) == VAR_DECL && DECL_WEAK (decl))
> ! ? ? DECL_COMMON (decl) = 0;
>
> ? ?if (use_object_blocks_p () && use_blocks_for_decl_p (decl))
> ? ? ?x = create_block_symbol (name, get_block_for_decl (decl), -1);
> --- 1435,1450 ----
>
> ? ?/* Specifying a section attribute on a variable forces it into a
> ? ? ? non-.bss section, and thus it cannot be common. ?*/
> ! ? gcc_assert (!(TREE_CODE (decl) == VAR_DECL
> ! ? ? ? ? ? ? && DECL_SECTION_NAME (decl) != NULL_TREE
> ! ? ? ? ? ? ? && DECL_INITIAL (decl) == NULL_TREE
> ! ? ? ? ? ? ? && DECL_COMMON (decl))
> ! ? ? ? ? ? ? || !DECL_COMMON (decl));
>
> ? ?/* Variables can't be both common and weak. ?*/
> ! ? gcc_assert (TREE_CODE (decl) != VAR_DECL
> ! ? ? ? ? ? ? || !DECL_WEAK (decl)
> ! ? ? ? ? ? ? || !DECL_COMMON (decl));
>
> ? ?if (use_object_blocks_p () && use_blocks_for_decl_p (decl))
> ? ? ?x = create_block_symbol (name, get_block_for_decl (decl), -1);
> *************** do_assemble_alias (tree decl, tree targe
> *** 5446,5451 ****
> --- 5448,5457 ----
> ? ?if (TREE_ASM_WRITTEN (decl))
> ? ? ?return;
>
> + ? /* We must force creation of DECL_RTL for debug info generation, even though
> + ? ? ?we don't use it here. ?*/
> + ? make_decl_rtl (decl);
> +
> ? ?TREE_ASM_WRITTEN (decl) = 1;
> ? ?TREE_ASM_WRITTEN (DECL_ASSEMBLER_NAME (decl)) = 1;
>
> *************** assemble_alias (tree decl, tree target)
> *** 5663,5672 ****
> ?# endif
> ?#endif
> ? ? ?}
> -
> - ? /* We must force creation of DECL_RTL for debug info generation, even though
> - ? ? ?we don't use it here. ?*/
> - ? make_decl_rtl (decl);
> ? ?TREE_USED (decl) = 1;
>
> ? ?/* A quirk of the initial implementation of aliases required that the user
> --- 5669,5674 ----
>