| Message ID | 20101201195338.GA21321@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
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.
> 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.
> 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
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.
> > 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.
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.
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);