Re: Fix pr61848, linux kernel miscompile

[Andrew Pinski <pinskia at gmail dot com>]

On Sun, Sep 14, 2014 at 9:02 PM, Alan Modra <amodra@gmail.com> wrote:
> This patch cures the linux kernel boot failure when compiled using
> trunk gcc.  (Andrew, apologies for hijacking your bugzilla, I started
> work on this before finding the bugzilla..)

It is ok that you took over as it looks like you have a more complete
patch than I had.
Does it make sense to add a few testcases for the section attribute too?

Thanks,
Andrew Pinski

>
> At its heart, the problem is caused by merge_decls merging from the
> old decl to the new decl, then copying back to the old decl and
> discarding the new.  When Jan moved some fields to the symtab,
> "copying back to the old decl" was lost for those fields.  Really,
> it would be best if merge_decls was rewritten to merge everything to
> the kept decl, but here I'm just doing that for fields accessed via
> decl_with_vis.symtab_node.
>
> I also make a few other fixes
> 1)  Trunk does not honour the last section attribute.
>         extern char foo;
>         char foo __attribute__ ((__section__(".machine")));
>         char foo __attribute__ ((__section__(".mymachine")));
>     results in a section of ".machine" for foo, rather than
>     ".mymachine", the result for previous compilers back to 2.95 and
>     possibly beyond.
> 1b) The comment about issuing "an error if the sections conflict"
>     being done "later in decl_attributes" is seriously out of date.
>     decl_attributes is called earlier on a fresh decl, so no error is
>     issued (which I think makes the code in handle_section_attribute
>     issuing this error, dead).  It's been that way since at least 2.95.
> 2)  TLS model attributes have never been merged as far as I can tell,
>     except for #pragma omp threadprivate variables.  I think
>         extern int __thread x;
>         int __thread x __attribute__ ((__tls_model__("local-exec")));
>     ought to result in a local-exec "x" rather than a default model
>     "x", but see the "isn't quite correct" comment below.  Fixing that
>     will, I think, require changing enum tls_model to make
>     TLS_MODEL_REAL different to TLS_MODEL_GLOBAL_DYNAMIC (which will
>     also fix the mismatch between enum tls_model and tls_model_names[]).
> 3)  The patch hunks below in cp/decl.c that just s/olddecl/newdecl/
>     are to fix "if (TREE_CODE (newdecl) == FUNCTION_DECL) ...
>     else switch (TREE_CODE (olddecl))" which looks horrible to me.
>     It's really just cosmetic since we know
>     TREE_CODE (newdecl) == TREE_CODE (olddecl) at this point.
>
> Bootstrapped and regression tested x86_64-linux.  OK to apply?
>
> gcc/c/
>         PR middle-end/61848
>         * c-decl.c (merge_decls): Don't merge section name or tls model
>         to newdecl symtab node, instead merge to olddecl.  Override
>         existing olddecl section name.  Set tls_model for all thread-local
>         vars, not just OMP thread-private ones.  Remove incorrect comment.
> gcc/cp/
>         PR middle-end/61848
>         * decl.c (merge_decls): Don't merge section name, comdat group or
>         tls model to newdecl symtab node, instead merge to olddecl.
>         Override existing olddecl section name.  Set tls_model for all
>         thread-local vars, not just OMP thread-private ones.  Remove
>         incorrect comment.
>
> Index: gcc/c/c-decl.c
> ===================================================================
> --- gcc/c/c-decl.c      (revision 215233)
> +++ gcc/c/c-decl.c      (working copy)
> @@ -2292,22 +2292,10 @@ merge_decls (tree newdecl, tree olddecl, tree newt
>
>    /* Merge the threadprivate attribute.  */
>    if (TREE_CODE (olddecl) == VAR_DECL && C_DECL_THREADPRIVATE_P (olddecl))
> -    {
> -      set_decl_tls_model (newdecl, DECL_TLS_MODEL (olddecl));
> -      C_DECL_THREADPRIVATE_P (newdecl) = 1;
> -    }
> +    C_DECL_THREADPRIVATE_P (newdecl) = 1;
>
>    if (CODE_CONTAINS_STRUCT (TREE_CODE (olddecl), TS_DECL_WITH_VIS))
>      {
> -      /* Merge the section attribute.
> -        We want to issue an error if the sections conflict but that
> -        must be done later in decl_attributes since we are called
> -        before attributes are assigned.  */
> -      if ((DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
> -         && DECL_SECTION_NAME (newdecl) == NULL
> -         && DECL_SECTION_NAME (olddecl))
> -       set_decl_section_name (newdecl, DECL_SECTION_NAME (olddecl));
> -
>        /* Copy the assembler name.
>          Currently, it can only be defined in the prototype.  */
>        COPY_DECL_ASSEMBLER_NAME (olddecl, newdecl);
> @@ -2517,6 +2505,20 @@ merge_decls (tree newdecl, tree olddecl, tree newt
>                   (char *) newdecl + sizeof (struct tree_decl_common),
>                   tree_code_size (TREE_CODE (olddecl)) - sizeof (struct tree_decl_common));
>           olddecl->decl_with_vis.symtab_node = snode;
> +
> +         if ((DECL_EXTERNAL (olddecl)
> +              || TREE_PUBLIC (olddecl)
> +              || TREE_STATIC (olddecl))
> +             && DECL_SECTION_NAME (newdecl) != NULL)
> +           set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl));
> +
> +         /* This isn't quite correct for something like
> +               int __thread x attribute ((tls_model ("local-exec")));
> +               extern int __thread x;
> +            as we'll lose the "local-exec" model.  */
> +         if (TREE_CODE (olddecl) == VAR_DECL
> +             && DECL_THREAD_LOCAL_P (newdecl))
> +           set_decl_tls_model (olddecl, DECL_TLS_MODEL (newdecl));
>           break;
>         }
>
> Index: gcc/cp/decl.c
> ===================================================================
> --- gcc/cp/decl.c       (revision 215233)
> +++ gcc/cp/decl.c       (working copy)
> @@ -1970,7 +1970,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>               if (!DECL_LANG_SPECIFIC (newdecl))
>                 retrofit_lang_decl (newdecl);
>
> -             set_decl_tls_model (newdecl, DECL_TLS_MODEL (olddecl));
>               CP_DECL_THREADPRIVATE_P (newdecl) = 1;
>             }
>         }
> @@ -2033,15 +2032,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>             }
>         }
>
> -      /* Merge the section attribute.
> -        We want to issue an error if the sections conflict but that must be
> -        done later in decl_attributes since we are called before attributes
> -        are assigned.  */
> -      if ((DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
> -         && DECL_SECTION_NAME (newdecl) == NULL
> -         && DECL_SECTION_NAME (olddecl) != NULL)
> -       set_decl_section_name (newdecl, DECL_SECTION_NAME (olddecl));
> -
>        if (TREE_CODE (newdecl) == FUNCTION_DECL)
>         {
>           DECL_NO_INSTRUMENT_FUNCTION_ENTRY_EXIT (newdecl)
> @@ -2086,19 +2076,6 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>    /* Merge the storage class information.  */
>    merge_weak (newdecl, olddecl);
>
> -  if ((TREE_CODE (olddecl) == FUNCTION_DECL || TREE_CODE (olddecl) == VAR_DECL)
> -      && (DECL_EXTERNAL (olddecl) || TREE_PUBLIC (olddecl) || TREE_STATIC (olddecl))
> -      && DECL_ONE_ONLY (olddecl))
> -    {
> -      struct symtab_node *symbol;
> -      if (TREE_CODE (olddecl) == FUNCTION_DECL)
> -       symbol = cgraph_node::get_create (newdecl);
> -      else
> -       symbol = varpool_node::get_create (newdecl);
> -      symbol->set_comdat_group (symtab_node::get
> -       (olddecl)->get_comdat_group ());
> -    }
> -
>    DECL_DEFER_OUTPUT (newdecl) |= DECL_DEFER_OUTPUT (olddecl);
>    TREE_PUBLIC (newdecl) = TREE_PUBLIC (olddecl);
>    TREE_STATIC (olddecl) = TREE_STATIC (newdecl) |= TREE_STATIC (olddecl);
> @@ -2452,12 +2429,12 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>      }
>    else
>      {
> -      size_t size = tree_code_size (TREE_CODE (olddecl));
> +      size_t size = tree_code_size (TREE_CODE (newdecl));
>
>        memcpy ((char *) olddecl + sizeof (struct tree_common),
>               (char *) newdecl + sizeof (struct tree_common),
>               sizeof (struct tree_decl_common) - sizeof (struct tree_common));
> -      switch (TREE_CODE (olddecl))
> +      switch (TREE_CODE (newdecl))
>         {
>         case LABEL_DECL:
>         case VAR_DECL:
> @@ -2469,7 +2446,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>           {
>              struct symtab_node *snode = NULL;
>
> -            if (TREE_CODE (olddecl) == VAR_DECL
> +            if (TREE_CODE (newdecl) == VAR_DECL
>                 && (TREE_STATIC (olddecl) || TREE_PUBLIC (olddecl) || DECL_EXTERNAL (olddecl)))
>               snode = symtab_node::get (olddecl);
>             memcpy ((char *) olddecl + sizeof (struct tree_decl_common),
> @@ -2476,7 +2453,7 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>                     (char *) newdecl + sizeof (struct tree_decl_common),
>                     size - sizeof (struct tree_decl_common)
>                     + TREE_CODE_LENGTH (TREE_CODE (newdecl)) * sizeof (char *));
> -            if (TREE_CODE (olddecl) == VAR_DECL)
> +            if (TREE_CODE (newdecl) == VAR_DECL)
>               olddecl->decl_with_vis.symtab_node = snode;
>           }
>           break;
> @@ -2488,6 +2465,38 @@ duplicate_decls (tree newdecl, tree olddecl, bool
>           break;
>         }
>      }
> +
> +  if (TREE_CODE (newdecl) == FUNCTION_DECL
> +      || TREE_CODE (newdecl) == VAR_DECL)
> +    {
> +      if (DECL_EXTERNAL (olddecl)
> +         || TREE_PUBLIC (olddecl)
> +         || TREE_STATIC (olddecl))
> +       {
> +         /* Merge the section attribute.
> +            We want to issue an error if the sections conflict but that must be
> +            done later in decl_attributes since we are called before attributes
> +            are assigned.  */
> +         if (DECL_SECTION_NAME (newdecl) != NULL)
> +           set_decl_section_name (olddecl, DECL_SECTION_NAME (newdecl));
> +
> +         if (DECL_ONE_ONLY (newdecl))
> +           {
> +             struct symtab_node *oldsym, *newsym;
> +             if (TREE_CODE (olddecl) == FUNCTION_DECL)
> +               oldsym = cgraph_node::get_create (olddecl);
> +             else
> +               oldsym = varpool_node::get_create (olddecl);
> +             newsym = symtab_node::get (newdecl);
> +             oldsym->set_comdat_group (newsym->get_comdat_group ());
> +           }
> +       }
> +
> +      if (TREE_CODE (newdecl) == VAR_DECL
> +         && DECL_THREAD_LOCAL_P (newdecl))
> +       set_decl_tls_model (olddecl, DECL_TLS_MODEL (newdecl));
> +    }
> +
>    DECL_UID (olddecl) = olddecl_uid;
>    if (olddecl_friend)
>      DECL_FRIEND_P (olddecl) = 1;
>
> --
> Alan Modra
> Australia Development Lab, IBM