http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494
--- Comment #33 from rguenther at suse dot de <rguenther at suse dot de> 2013-03-05 15:48:03 UTC --- On Tue, 5 Mar 2013, ebotcazou at gcc dot gnu.org wrote: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=50494 > > --- Comment #32 from Eric Botcazou <ebotcazou at gcc dot gnu.org> 2013-03-05 > 15:32:15 UTC --- > > But in all places I found we check TREE_ASM_WRITTEN on DECL_INITIAL > > of the SYMBOL_REF_DECL ... > > Nope, maybe_output_constant_def_contents has: > > rtx symbol = XEXP (desc->rtl, 0); > tree exp = desc->value; > > if (flag_syntax_only) > return; > > if (TREE_ASM_WRITTEN (exp)) > /* Already output; don't do it again. */ > return; > > so the DECL_INITIAL of the SYMBOL_REF_DECL must be desc->value. Ah, ok ... too many smart TREE_ASM_WRITTEN bits around ... > > So it must be pure luck that we survived LTO bootstrap before my > > patch (as far as I understand things). Before my patch we created > > yet another decl for the constant pool entry. With my patch > > we will have one less (we still have the decls from the constant > > pool entries that end up being shared in the LTRANS unit). > > We use LTO on heavy Ada applications with an unmodified 4.7 compiler (modulo a > patch for -g) so I don't think that luck has anything to do here. Fine, so the above might be the only issue. > > So in the end I can agree to that my patch doesn't really fix > > the original issue fully. So we can as well revert it and > > instead avoid messing with alignment of the constant pool entries. > > That would be my preference. Which of course pessimizes code generation and probably causes some testsuite fallout for ppc (the original reported spurious fails). > > Hmm, maybe. Then, why do we copy the constant in the first place ... > > > > Thus, > > > > Index: varasm.c > > =================================================================== > > --- varasm.c (revision 196462) > > +++ varasm.c (working copy) > > @@ -3087,7 +3087,7 @@ build_constant_desc (tree exp, tree decl > > int labelno; > > > > desc = ggc_alloc_constant_descriptor_tree (); > > - desc->value = copy_constant (exp); > > + desc->value = exp; > > > > /* Propagate marked-ness to copied constant. */ > > if (flag_mudflap && mf_marked_p (exp)) > > > > should be an "equivalent" fix. > > This call to copy_constant has been there for ages though. so a little bit of > archeology would probably be in order before removing it. ;) Did that, it's there since forever - well, I traced it back to the point we only deferred string constants: 37459 jakub p = (struct deferred_string *) 37459 jakub xmalloc (sizeof (struct deferred_string)); 37459 jakub 37459 jakub p->exp = copy_constant (exp); 37459 jakub p->label = desc->label; I'm LTO bootstrapping desc->value = decl ? exp : copy_constant (exp); and doing a regular bootstrap with the copy_constant completely removed at the moment. Just curious ... > In the meantime, I've successfully bootstrapped my patchlet so we can also go > for it. I'm fine with that. As I concluded that the original fix didn't fix the alignment issue (well, not for all possible cases at least) reverting the original fix works for me as well. I'm working on a patch to avoid re-aligning constant pool entries.
