Patchwork Fix g++.dg/torture/stackalign/eh-alloca-1.C with LTO plugin

login
register
mail settings
Submitter Jan Hubicka
Date Dec. 1, 2010, 7:53 p.m.
Message ID <20101201195338.GA21321@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/73895/
State New
Headers show

Comments

Jan Hubicka - Dec. 1, 2010, 7:53 p.m.
Hi,
this patch fixes the stackalign issues.  What happens here is that
dw2_force_const_mem produce an label that is supposed to point to given
constant in memory.  This is realized by dw2_output_indirect_constant_1 that
produce variable with the name smatching the label's name and appropriate
constructor.

This is used to encode personality routines for EH.

The catch is that the label name is "*.Lblah", while the static variable in LTO
mode ends up being "*.Lblah.1234" because we do the mangling of static vars.
This patch simply avoids all the mangling by enforcing the assembler name to be
same as label's assembler name.  It is what we want anyway.

I am not quite sure why this happens only with linker plugin. I assume that we
optimize the functions differently resulting in different EH info.

Regtested/bootstrapped x86_64-linux, OK?

Honza

	* dwarf2asm.c (dw2_output_indirect_constant_1): Set assembler name
	of the new decl.
Richard Guenther - Dec. 2, 2010, 2:18 p.m.
On Wed, 1 Dec 2010, Jan Hubicka wrote:

> Hi,
> this patch fixes the stackalign issues.  What happens here is that
> dw2_force_const_mem produce an label that is supposed to point to given
> constant in memory.  This is realized by dw2_output_indirect_constant_1 that
> produce variable with the name smatching the label's name and appropriate
> constructor.
> 
> This is used to encode personality routines for EH.
> 
> The catch is that the label name is "*.Lblah", while the static variable in LTO
> mode ends up being "*.Lblah.1234" because we do the mangling of static vars.
> This patch simply avoids all the mangling by enforcing the assembler name to be
> same as label's assembler name.  It is what we want anyway.
> 
> I am not quite sure why this happens only with linker plugin. I assume that we
> optimize the functions differently resulting in different EH info.
> 
> Regtested/bootstrapped x86_64-linux, OK?
> 
> Honza
> 
> 	* dwarf2asm.c (dw2_output_indirect_constant_1): Set assembler name
> 	of the new decl.
> Index: dwarf2asm.c
> ===================================================================
> --- dwarf2asm.c	(revision 167242)
> +++ dwarf2asm.c	(working copy)
> @@ -920,6 +920,7 @@ dw2_output_indirect_constant_1 (splay_tr
>  
>    sym_ref = gen_rtx_SYMBOL_REF (Pmode, sym);
>    sym = targetm.strip_name_encoding (sym);
> +  SET_DECL_ASSEMBLER_NAME (decl, id);

Shouldn't this be done before the DECL_ASSEMBLER_NAME use above?

Why doesn't it just work in mangled form?  We generate that decl
at this very place, so I don't see how we get the chance to mangle it
anyway.

Richard.
Jan Hubicka - Dec. 2, 2010, 3:04 p.m.
> On Wed, 1 Dec 2010, Jan Hubicka wrote:
> 
> > Hi,
> > this patch fixes the stackalign issues.  What happens here is that
> > dw2_force_const_mem produce an label that is supposed to point to given
> > constant in memory.  This is realized by dw2_output_indirect_constant_1 that
> > produce variable with the name smatching the label's name and appropriate
> > constructor.
> > 
> > This is used to encode personality routines for EH.
> > 
> > The catch is that the label name is "*.Lblah", while the static variable in LTO
> > mode ends up being "*.Lblah.1234" because we do the mangling of static vars.
> > This patch simply avoids all the mangling by enforcing the assembler name to be
> > same as label's assembler name.  It is what we want anyway.
> > 
> > I am not quite sure why this happens only with linker plugin. I assume that we
> > optimize the functions differently resulting in different EH info.
> > 
> > Regtested/bootstrapped x86_64-linux, OK?
> > 
> > Honza
> > 
> > 	* dwarf2asm.c (dw2_output_indirect_constant_1): Set assembler name
> > 	of the new decl.
> > Index: dwarf2asm.c
> > ===================================================================
> > --- dwarf2asm.c	(revision 167242)
> > +++ dwarf2asm.c	(working copy)
> > @@ -920,6 +920,7 @@ dw2_output_indirect_constant_1 (splay_tr
> >  
> >    sym_ref = gen_rtx_SYMBOL_REF (Pmode, sym);
> >    sym = targetm.strip_name_encoding (sym);
> > +  SET_DECL_ASSEMBLER_NAME (decl, id);
> 
> Shouldn't this be done before the DECL_ASSEMBLER_NAME use above?

Ah yes, though it is executed only at TREE_PUBLIC path that won't mangle.

> 
> Why doesn't it just work in mangled form?  We generate that decl
> at this very place, so I don't see how we get the chance to mangle it
> anyway.

I think it eventually gets into lto_set_decl_assembler_name that
uncionditionally mangles all statics.

Honza
> 
> Richard.
Jan Hubicka - Dec. 2, 2010, 3:07 p.m.
> Ah yes, though it is executed only at TREE_PUBLIC path that won't mangle.
> 
> > 
> > Why doesn't it just work in mangled form?  We generate that decl
> > at this very place, so I don't see how we get the chance to mangle it
> > anyway.
> 
> I think it eventually gets into lto_set_decl_assembler_name that
> uncionditionally mangles all statics.
... and dwarf2asm code first reffer to the value using the label it generates
and only later it produces the decl when it is outputting its "constant pool".
I would expect it to work in a way producing decl first collecting them in
hashtable but it is not implemented that way.
So the NAME must not be mangled during the production of VAR_DECL.


Honza
Richard Guenther - Dec. 2, 2010, 3:45 p.m.
On Thu, 2 Dec 2010, Jan Hubicka wrote:

> > On Wed, 1 Dec 2010, Jan Hubicka wrote:
> > 
> > > Hi,
> > > this patch fixes the stackalign issues.  What happens here is that
> > > dw2_force_const_mem produce an label that is supposed to point to given
> > > constant in memory.  This is realized by dw2_output_indirect_constant_1 that
> > > produce variable with the name smatching the label's name and appropriate
> > > constructor.
> > > 
> > > This is used to encode personality routines for EH.
> > > 
> > > The catch is that the label name is "*.Lblah", while the static variable in LTO
> > > mode ends up being "*.Lblah.1234" because we do the mangling of static vars.
> > > This patch simply avoids all the mangling by enforcing the assembler name to be
> > > same as label's assembler name.  It is what we want anyway.
> > > 
> > > I am not quite sure why this happens only with linker plugin. I assume that we
> > > optimize the functions differently resulting in different EH info.
> > > 
> > > Regtested/bootstrapped x86_64-linux, OK?
> > > 
> > > Honza
> > > 
> > > 	* dwarf2asm.c (dw2_output_indirect_constant_1): Set assembler name
> > > 	of the new decl.
> > > Index: dwarf2asm.c
> > > ===================================================================
> > > --- dwarf2asm.c	(revision 167242)
> > > +++ dwarf2asm.c	(working copy)
> > > @@ -920,6 +920,7 @@ dw2_output_indirect_constant_1 (splay_tr
> > >  
> > >    sym_ref = gen_rtx_SYMBOL_REF (Pmode, sym);
> > >    sym = targetm.strip_name_encoding (sym);
> > > +  SET_DECL_ASSEMBLER_NAME (decl, id);
> > 
> > Shouldn't this be done before the DECL_ASSEMBLER_NAME use above?
> 
> Ah yes, though it is executed only at TREE_PUBLIC path that won't mangle.
> 
> > 
> > Why doesn't it just work in mangled form?  We generate that decl
> > at this very place, so I don't see how we get the chance to mangle it
> > anyway.
> 
> I think it eventually gets into lto_set_decl_assembler_name that
> uncionditionally mangles all statics.

How so?  Maybe via TREE_CONSTANT_POOL_ADDRESS_P stuff?  And why
do we care?  We're rebuilding this decl anyway (and thus would have
a decl merging issue).

Your fix looks like a hack to work around a fundamental issue
elsewhere ...

Richard.
Jan Hubicka - Dec. 2, 2010, 4:05 p.m.
> 
> How so?  Maybe via TREE_CONSTANT_POOL_ADDRESS_P stuff?  And why

The code does not set TREE_CONSTANT_POOL_ADDRESS_P. It does sort of its own constant pool.

/* Put X, a SYMBOL_REF, in memory.  Return a SYMBOL_REF to the allocated
   memory.  Differs from force_const_mem in that a single pool is used for
   the entire unit of translation, and the memory is not guaranteed to be
   "near" the function in any interesting sense.  IS_PUBLIC controls whether
   the symbol can be shared across the entire application (or DSO).  *

As I've explained earlier, the problem is that the code is organized in a way
first producing label:

          ASM_GENERATE_INTERNAL_LABEL (label, "LDFCM", dw2_const_labelno);
          ++dw2_const_labelno;
          gcc_assert (!maybe_get_identifier (label));
          decl_id = get_identifier (label);

(in dw2_force_const_mem)
Then we happily use decl_id to reffer to the value.

At the end of compilation dw2_output_indirect_constants build the decl that
must match the decl_id computed above.  the decl is created as:

decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, id, ptr_type_node);
where ID is the DECL_ID produced and used earlier.

build_decl then store ID in DECL_NAME.  Later DECL_ASSEMBLER_NAME calls

tree
decl_assembler_name (tree decl)
{
  if (!DECL_ASSEMBLER_NAME_SET_P (decl))
    lang_hooks.set_decl_assembler_name (decl);
  return DECL_WITH_VIS_CHECK (decl)->decl_with_vis.assembler_name; 
}

and finally we get into

static void
lto_set_decl_assembler_name (tree decl)
{
  /* This is almost the same as lhd_set_decl_assembler_name, except that
     we need to uniquify file-scope names, even if they are not
     TREE_PUBLIC, to avoid conflicts between individual files.  */
  tree id;

  if (TREE_PUBLIC (decl))
    id = targetm.mangle_decl_assembler_name (decl, DECL_NAME (decl));
  else
    { 
      const char *name = IDENTIFIER_POINTER (DECL_NAME (decl));
      char *label;

      ASM_FORMAT_PRIVATE_NAME (label, name, DECL_UID (decl));
      id = get_identifier (label);
    }

  SET_DECL_ASSEMBLER_NAME (decl, id);
}

I guess LTO frontend might be first one that mangles even names starting with
'*', but the code above really wants to build a variable with assembler name
matching internal label produced earlier, so setting assembler name instead of
going through frontend hooks seems sane to me.

It would be perhaps bit saner to if we produced decls early, but I am not sure
I want to reorganize that code now...

Honza

> do we care?  We're rebuilding this decl anyway (and thus would have
> a decl merging issue).
> 
> Your fix looks like a hack to work around a fundamental issue
> elsewhere ...
> 
> Richard.
Richard Guenther - Dec. 2, 2010, 4:18 p.m.
On Thu, 2 Dec 2010, Jan Hubicka wrote:

> > 
> > How so?  Maybe via TREE_CONSTANT_POOL_ADDRESS_P stuff?  And why
> 
> The code does not set TREE_CONSTANT_POOL_ADDRESS_P. It does sort of its own constant pool.
> 
> /* Put X, a SYMBOL_REF, in memory.  Return a SYMBOL_REF to the allocated
>    memory.  Differs from force_const_mem in that a single pool is used for
>    the entire unit of translation, and the memory is not guaranteed to be
>    "near" the function in any interesting sense.  IS_PUBLIC controls whether
>    the symbol can be shared across the entire application (or DSO).  *
> 
> As I've explained earlier, the problem is that the code is organized in a way
> first producing label:
> 
>           ASM_GENERATE_INTERNAL_LABEL (label, "LDFCM", dw2_const_labelno);
>           ++dw2_const_labelno;
>           gcc_assert (!maybe_get_identifier (label));
>           decl_id = get_identifier (label);
> 
> (in dw2_force_const_mem)
> Then we happily use decl_id to reffer to the value.
> 
> At the end of compilation dw2_output_indirect_constants build the decl that
> must match the decl_id computed above.  the decl is created as:
> 
> decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, id, ptr_type_node);
> where ID is the DECL_ID produced and used earlier.
> 
> build_decl then store ID in DECL_NAME.  Later DECL_ASSEMBLER_NAME calls
> 
> tree
> decl_assembler_name (tree decl)
> {
>   if (!DECL_ASSEMBLER_NAME_SET_P (decl))
>     lang_hooks.set_decl_assembler_name (decl);
>   return DECL_WITH_VIS_CHECK (decl)->decl_with_vis.assembler_name; 
> }
> 
> and finally we get into
> 
> static void
> lto_set_decl_assembler_name (tree decl)
> {
>   /* This is almost the same as lhd_set_decl_assembler_name, except that
>      we need to uniquify file-scope names, even if they are not
>      TREE_PUBLIC, to avoid conflicts between individual files.  */
>   tree id;
> 
>   if (TREE_PUBLIC (decl))
>     id = targetm.mangle_decl_assembler_name (decl, DECL_NAME (decl));
>   else
>     { 
>       const char *name = IDENTIFIER_POINTER (DECL_NAME (decl));
>       char *label;
> 
>       ASM_FORMAT_PRIVATE_NAME (label, name, DECL_UID (decl));
>       id = get_identifier (label);
>     }
> 
>   SET_DECL_ASSEMBLER_NAME (decl, id);
> }
> 
> I guess LTO frontend might be first one that mangles even names starting with
> '*', but the code above really wants to build a variable with assembler name
> matching internal label produced earlier, so setting assembler name instead of
> going through frontend hooks seems sane to me.
> 
> It would be perhaps bit saner to if we produced decls early, but I am not sure
> I want to reorganize that code now...

Ok, well, at least move setting the assembler name to before we
possibly call DECL_ASSEMBLER_NAME.

Richard.

Patch

Index: dwarf2asm.c
===================================================================
--- dwarf2asm.c	(revision 167242)
+++ dwarf2asm.c	(working copy)
@@ -920,6 +920,7 @@  dw2_output_indirect_constant_1 (splay_tr
 
   sym_ref = gen_rtx_SYMBOL_REF (Pmode, sym);
   sym = targetm.strip_name_encoding (sym);
+  SET_DECL_ASSEMBLER_NAME (decl, id);
   if (TREE_PUBLIC (decl) && USE_LINKONCE_INDIRECT)
     fprintf (asm_out_file, "\t.hidden %sDW.ref.%s\n", user_label_prefix, sym);
   assemble_variable (decl, 1, 1, 1);