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

[Richard Guenther <richard.guenther@gmail.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 ----
>