| Message ID | 20140915040242.GG17693@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
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
On Mon, 15 Sep 2014, Alan Modra 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..) Please include testcases in your patch for each case that you fix.
On Mon, Sep 15, 2014 at 08:36:58PM +0000, Joseph S. Myers wrote: > On Mon, 15 Sep 2014, Alan Modra 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..) > > Please include testcases in your patch for each case that you fix. gcc testsuite additions? I decline. It is too soon. If you had read my patch submission you'll see that at some stage gcc was supposed to warn on conflicting section attributes, but hasn't done so for a very long time. There needs to be some agreement on which direction we should go before I'm willing to spend even a small amount of time on the testsuite. Also, a test for merging tls model attributes runs into the problem that this can only be done in a target independent way by looking at dumps, and the tls model dump is currently broken. Come to think of it, what if I decline to make any testsuite additions? I'm asking because you're a steering committee member, and set policy. I fully agree that testsuite additions are desirable, but, is *requiring* such testsuite additions a good idea? Will that really improve gcc over time? (Herding cats comes to mind. I think you'll just drive away gcc contributors.)
On Tue, 16 Sep 2014, Alan Modra wrote: > gcc testsuite additions? I decline. It is too soon. If you had read > my patch submission you'll see that at some stage gcc was supposed to > warn on conflicting section attributes, but hasn't done so for a very > long time. There needs to be some agreement on which direction we > should go before I'm willing to spend even a small amount of time on > the testsuite. Also, a test for merging tls model attributes runs The point of testsuite additions is to verify the visible changes in behavior intended to be caused by the patch (and, as applicable, that the behavior doesn't change in other related areas where it's not meant to change), rather than to test something that GCC doesn't do either before or after the patch. If the lack of tests is because the patch is an RFC about what the desired behavior is, rather than an actual submission for inclusion, then it's helpful to say so in the patch submission. > into the problem that this can only be done in a target independent > way by looking at dumps, and the tls model dump is currently broken. If there is a reason some aspect of the change can't readily be tested, that should be stated in the patch submission (along with examples of the affected code that can't readily be put into suitable form for the testsuite). > Come to think of it, what if I decline to make any testsuite > additions? I'm asking because you're a steering committee member, and Then the patch isn't ready for review. Documentation and testcases are the first thing I look at when reviewing C front-end changes; the testcases are the primary evidence that the patch does what it's meant to do, and without them I won't generally try to review the code changes. There's no requirement for test-driven development, but personally I prefer to write the documentation and tests before the rest of the patch (and make sure the tests do fail with the unmodified compiler, unless they are tests of related cases that already work but I want to make sure don't get broken) - though in the course of implementing the patch I expect to find other related cases that result in more tests being written, and to modify exactly what I expect from the tests I wrote earlier. (I also find it a pain when backporting patches to packages that don't expect testcases as a norm for all patches if the author didn't include testsuite coverage with their patch, as it makes it much harder to tell if the backport is working properly. Or if a problem was caused by a patch that was committed without testcases - again, it's hard to tell if a fix affects the fix to the original issue the patch was supposed to address.)
On Tue, Sep 16, 2014 at 01:03:50PM +0000, Joseph S. Myers wrote: > The point of testsuite additions is to verify the visible changes in Understood. A long time ago I worked on mil-spec systems.. > > Come to think of it, what if I decline to make any testsuite > > additions? > > Then the patch isn't ready for review. Documentation and testcases are > the first thing I look at when reviewing C front-end changes; the > testcases are the primary evidence that the patch does what it's meant to > do, and without them I won't generally try to review the code changes. I gave you two testcases. I happen to know how nice it is to have testcases when reviewing patches. Yes, they weren't testsuite patches. Now I'd normally just grumble a bit to myself and comply with a request for a testsuite patch, but I'm a little annoyed. The testsuite isn't my personal itch. I could have just bisected until finding Jan's patch, and asked him nicely to please fix his breakage. This is what most people do, and pointing the finger at a particular patch is useful. I went further and analysed exactly why the patch was at fault. I could have stopped there. The breakage wasn't really affecting me, and it wasn't my bug after all. Instead I thought I'd have a go at fixing the problem, without bothering Jan: The loss of section attribute looked quite easy to fix. So I threw together a patch, verified it worked, then knowing that there is more than one merge_decls in the gcc source code, fixed the same problem for C++. At that point I found Andrew's bugzilla. Andrew is competent, I could have just attached the patch I had to his bugzilla and left him to it. However it occurred to me that Jan's changes might have broken something else, so I poked around and found some other fields that had been moved to symtab_node. No big deal, I fixed that breakage as well. (I'm not sure I have them all actually. symtab_node fields and varpool_node fields are not clearly demarcated into those private to cgraph and those that matter externally.) I could have stopped there too. The final piece was wondering why Jan had added an extra test on DECL_SECTION, seeing that gcc has not reported an error on a changed section attribute for a very long time, and noticing that tls model wasn't generally merged. This is obviously where I went wrong. Silly me. I should have just pointed out these problems and asked you as C front end maintainer to please think about fixing them. That way you would have written the patch, and presumably the testsuite, and everyone would be happy! In fact, maybe I should have just avoided all this effort and just asked Jan to please fix his bug. BTW, in my patch submission at https://gcc.gnu.org/ml/gcc-patches/2014-09/msg01146.html I said "trunk does not honour the last section attribute". What I really should have said was "trunk, with a straight-forward fix for complete loss of section attribute, does not honour the last section attribute".
On 09/16/14 00:03, Alan Modra wrote: > gcc testsuite additions? I decline. It is too soon. If you had read > my patch submission you'll see that at some stage gcc was supposed to > warn on conflicting section attributes, but hasn't done so for a very > long time. There needs to be some agreement on which direction we > should go before I'm willing to spend even a small amount of time on > the testsuite. I think the direction for this ought to be quite clear, warn if we've got conflicting section attributes :-) One could make the argument that if we had a test for conflicting section attributes that this code wouldn't have bit-rotted so badly. I think for #1 in your original message, we should be issuing a warning of some kind as clearly something isn't right when we've asked for the same object to be in two different sections. > Also, a test for merging tls model attributes runs > into the problem that this can only be done in a target independent > way by looking at dumps, and the tls model dump is currently broken. Target independent tests are always best, but pulling together one that is specific to a target is usually still good enough. Especially if it's an x86 target given how often those are tested by the developer community as a whole. I think for #1 in your posting, verifying that we issue the proper warning should be sufficient. Use the dg framework test and this marker: /* { dg-require-named-sections } */ I'm not familiar enough with the TLS stuff to know what the right behaviour ought to be. > > Come to think of it, what if I decline to make any testsuite > additions? I'm asking because you're a steering committee member, and > set policy. I fully agree that testsuite additions are desirable, > but, is *requiring* such testsuite additions a good idea? Will that > really improve gcc over time? (Herding cats comes to mind. I think > you'll just drive away gcc contributors.) There's no policy that requires testsuites, but for bugfixes most maintainers ask for them, or at least an explanation why they can't be provided. GCC's testsuite is primarily a regression driven suite and these are great examples of behavior regressions that never should have happened, but did because of holes in the suite. That's obviously something we ought to correct ;-) Jeff
On Thu, Sep 18, 2014 at 10:02:25PM -0600, Jeff Law wrote: > On 09/16/14 00:03, Alan Modra wrote: > >gcc testsuite additions? I decline. It is too soon. If you had read > >my patch submission you'll see that at some stage gcc was supposed to > >warn on conflicting section attributes, but hasn't done so for a very > >long time. There needs to be some agreement on which direction we > >should go before I'm willing to spend even a small amount of time on > >the testsuite. > I think the direction for this ought to be quite clear, warn if we've got > conflicting section attributes :-) One could make the argument that if we > had a test for conflicting section attributes that this code wouldn't have > bit-rotted so badly. OK. That's what I thought too. Of course, the patch that I submitted doesn't warn, and gcc has lacked the warning for at least 15 years.. > I think for #1 in your original message, we should be issuing a warning of > some kind as clearly something isn't right when we've asked for the same > object to be in two different sections. > > > >Also, a test for merging tls model attributes runs > >into the problem that this can only be done in a target independent > >way by looking at dumps, and the tls model dump is currently broken. > Target independent tests are always best, but pulling together one that is > specific to a target is usually still good enough. Especially if it's an > x86 target given how often those are tested by the developer community as a > whole. > > I think for #1 in your posting, verifying that we issue the proper warning > should be sufficient. Use the dg framework test and this marker: > > /* { dg-require-named-sections } */ Thanks! That helps, and I see someone has attached a testcase to the bugzilla in an attempt to move this forward. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61848#c18 I'll be happy if someone else takes over the bugzilla. I've been on vacation, and should spend time over the next few weeks on the binutils 2.25 release rather than trying to push gcc patches.
On 10/15/14 22:51, Alan Modra wrote: >> Target independent tests are always best, but pulling together one that is >> specific to a target is usually still good enough. Especially if it's an >> x86 target given how often those are tested by the developer community as a >> whole. >> >> I think for #1 in your posting, verifying that we issue the proper warning >> should be sufficient. Use the dg framework test and this marker: >> >> /* { dg-require-named-sections } */ > > Thanks! That helps, and I see someone has attached a testcase to the > bugzilla in an attempt to move this forward. > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61848#c18 > > I'll be happy if someone else takes over the bugzilla. I've been on > vacation, and should spend time over the next few weeks on the > binutils 2.25 release rather than trying to push gcc patches. Looks like Markus will pick this up, which is obviously fine with me :-) Getting 2.25 out the door is good and simplifies some things I've got to deal with, so happy to see that moving forward. jeff
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;
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..) 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.