On Sun, Sep 14, 2014 at 9:02 PM, Alan Modra <amo...@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
