| Message ID | 20101205122000.GA24047@atrey.karlin.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
Hi, I've tested your patch, and it also fixes the libstdc++ build on ARM-Linux. Can you see that it gets applied soon? Thanks! Chung-Lin On 2010/12/5 08:20 PM, Jan Hubicka wrote: >> >> The x86-64 failure seems to be calling get_named_section() during >> dwarf2out_init(), when current_function_decl is still NULL, with the >> resulting section flags containing SECTION_WRITE. I've added an >> additional condition testing the ".text." section name prefix when decl >> is NULL. > > I've fixed the problem in meanitme by preventing dwarf2out from consturcting > unlikely section until some functions are output. > > As I understand the problem is due the fact that current_function_section > is called at expansion time > #6 0x0000000000b009b8 in current_function_section () at > ../../combined/gcc/varasm.c:635 > #7 0x0000000000b39835 in arm_is_long_call_p (decl=0x2ba5b225cd00) at > ../../combined/gcc/config/arm/arm.c:5001 > #8 0x0000000000b39937 in arm_function_ok_for_sibcall (decl=0x2ba5b225cd00, > exp=0x2ba5b2a84370) at ../../combined/gcc/config/arm/arm.c:5033 > #9 0x00000000006b1a4b in expand_call (exp=0x2ba5b2a84370, target=0x0, > ignore=1) at ../../combined/gcc/calls.c:2306 > #10 0x00000000007614e0 in expand_expr_real_1 (exp=0x2ba5b2a84370, target=0x0, > tmode=VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0) at > ../../combined/gcc/expr.c:9299 > #11 0x00000000006c2796 in expand_gimple_stmt (stmt=0x2ba5b22caee0) at > ../../combined/gcc/cfgexpand.c:1916 > #12 0x00000000006c342b in expand_gimple_basic_block (bb=0x2ba5b253e548) at > ../../combined/gcc/cfgexpand.c:2154 > #13 0x00000000006c47b8 in gimple_expand_cfg () at > > while at this point, the current section does not have very good meaning. > Unique sections are resolved at assemble_function_start time and we don't > know about hot/cold partitioning yet. > > This seems to be sort of hack in ARM BE trying to second guess if the > function will end up in same section with arm_function_in_section_p > getting rid of some cases. > It is arguably hack, but I guess nothing prevents us from compuing > unique section early. Does the following (untested) patch help? > > Index: varasm.c > =================================================================== > --- varasm.c (revision 167472) > +++ varasm.c (working copy) > @@ -1548,8 +1548,6 @@ > if (CONSTANT_POOL_BEFORE_FUNCTION) > output_constant_pool (fnname, decl); > > - resolve_unique_section (decl, 0, flag_function_sections); > - > /* Make sure the not and cold text (code) sections are properly > aligned. This is necessary here in the case where the function > has both hot and cold sections, because we don't want to re-set > Index: cfgexpand.c > =================================================================== > --- cfgexpand.c (revision 167472) > +++ cfgexpand.c (working copy) > @@ -3949,6 +3949,10 @@ > crtl->preferred_stack_boundary = STACK_BOUNDARY; > cfun->cfg->max_jumptable_ents = 0; > > + /* Resovle the function section. Some targets, like ARM EABI rely on knowledge > + of the function section at exapnsion time to predict distance of calls. */ > + resolve_unique_section (current_function_decl, 0, flag_function_sections); > + > /* Expand the variables recorded during gimple lowering. */ > timevar_push (TV_VAR_EXPAND); > start_sequence (); >> >> This patch has been tested on the respective platforms with no >> regressions, and does now revive the ARM-Linux C++ build; however, I'm >> not sure whether this is a robust enough fix; it may just well be >> band-aid :P >> >> Thanks, >> Chung-Lin >> >> 2010-12-02 Chung-Lin Tang<cltang@codesourcery.com> >> >> PR middle-end/46667 >> * varasm.c (get_named_section): Add call to >> resolve_unique_section(). >> (default_function_section): Call get_named_section() for >> DECL_ONE_ONLY decls. >> (default_section_type_flags): Start off with SECTION_CODE as >> flags value when decl is NULL and section name starts with >> ".text.". > >> Index: varasm.c >> =================================================================== >> --- varasm.c (revision 167365) >> +++ varasm.c (working copy) >> @@ -380,6 +380,10 @@ >> unsigned int flags; >> >> gcc_assert (!decl || DECL_P (decl)); >> + >> + if (decl) >> + resolve_unique_section (decl, reloc, 0); >> + >> if (name == NULL) >> name = TREE_STRING_POINTER (DECL_SECTION_NAME (decl)); >> >> @@ -533,6 +537,9 @@ >> default_function_section (tree decl, enum node_frequency freq, >> bool startup, bool exit) >> { >> + if (decl&& DECL_ONE_ONLY (decl)) >> + return get_named_section (decl, NULL, 0); >> + >> /* Startup code should go to startup subsection unless it is >> unlikely executed (this happens especially with function splitting >> where we can split away unnecesary parts of static constructors. */ >> @@ -5934,7 +5941,8 @@ >> { >> unsigned int flags; >> >> - if (decl&& TREE_CODE (decl) == FUNCTION_DECL) >> + if ((decl&& TREE_CODE (decl) == FUNCTION_DECL) >> + || (!decl&& strncmp (name, ".text.", 6) == 0)) >> flags = SECTION_CODE; >> else if (decl&& decl_readonly_section (decl, reloc)) >> flags = 0; >
Hi, given that the patch was tested to solve the bootstrap problem, is it OK for mainline? The problem is that some backends needs to know function section at expansion time to guess if long/short jump variants are used. * varasm.c (assemble_start_function): Do not call resolve_unique_section. * cfgexpand.c (gimple_expand_cfg): Resolve it here. Honza > > Index: varasm.c > =================================================================== > --- varasm.c (revision 167472) > +++ varasm.c (working copy) > @@ -1548,8 +1548,6 @@ > if (CONSTANT_POOL_BEFORE_FUNCTION) > output_constant_pool (fnname, decl); > > - resolve_unique_section (decl, 0, flag_function_sections); > - > /* Make sure the not and cold text (code) sections are properly > aligned. This is necessary here in the case where the function > has both hot and cold sections, because we don't want to re-set > Index: cfgexpand.c > =================================================================== > --- cfgexpand.c (revision 167472) > +++ cfgexpand.c (working copy) > @@ -3949,6 +3949,10 @@ > crtl->preferred_stack_boundary = STACK_BOUNDARY; > cfun->cfg->max_jumptable_ents = 0; > > + /* Resovle the function section. Some targets, like ARM EABI rely on knowledge > + of the function section at exapnsion time to predict distance of calls. */ > + resolve_unique_section (current_function_decl, 0, flag_function_sections); > + > /* Expand the variables recorded during gimple lowering. */ > timevar_push (TV_VAR_EXPAND); > start_sequence ();
On Sat, Dec 11, 2010 at 2:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, > given that the patch was tested to solve the bootstrap problem, is it OK for mainline? > The problem is that some backends needs to know function section at expansion time to guess > if long/short jump variants are used. I think it makes sense, so consider it approved. Thanks, Richard. > * varasm.c (assemble_start_function): Do not call resolve_unique_section. > * cfgexpand.c (gimple_expand_cfg): Resolve it here. > > Honza >> >> Index: varasm.c >> =================================================================== >> --- varasm.c (revision 167472) >> +++ varasm.c (working copy) >> @@ -1548,8 +1548,6 @@ >> if (CONSTANT_POOL_BEFORE_FUNCTION) >> output_constant_pool (fnname, decl); >> >> - resolve_unique_section (decl, 0, flag_function_sections); >> - >> /* Make sure the not and cold text (code) sections are properly >> aligned. This is necessary here in the case where the function >> has both hot and cold sections, because we don't want to re-set >> Index: cfgexpand.c >> =================================================================== >> --- cfgexpand.c (revision 167472) >> +++ cfgexpand.c (working copy) >> @@ -3949,6 +3949,10 @@ >> crtl->preferred_stack_boundary = STACK_BOUNDARY; >> cfun->cfg->max_jumptable_ents = 0; >> >> + /* Resovle the function section. Some targets, like ARM EABI rely on knowledge >> + of the function section at exapnsion time to predict distance of calls. */ >> + resolve_unique_section (current_function_decl, 0, flag_function_sections); >> + >> /* Expand the variables recorded during gimple lowering. */ >> timevar_push (TV_VAR_EXPAND); >> start_sequence (); >
> On Sat, Dec 11, 2010 at 2:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote: > > Hi, > > given that the patch was tested to solve the bootstrap problem, is it OK for mainline? > > The problem is that some backends needs to know function section at expansion time to guess > > if long/short jump variants are used. > > I think it makes sense, so consider it approved. Comitted as revision 167795. Thanks, Honza
> > This caused PR47218: multiple definitions of non-virtual thunks appear at > final link-time. There is something different about the way non-virtual > thunks are output that causes this change to make them no longer emitted in > COMDAT sections: when I revert or apply your patch, and compile the testcase > from the dup PR47116(**), I see this difference in the generated source: > > > It turns out that resolve_unique_function() is not called at all for the > thunk function any more, where previously it was called from > assemble_start_function called from cgraph_expand_function. This is because > gimple_expand_cfg() isn't called for these thunk functions; they are emitted > through a call chain that looks like: I see, the thunks are still expanded on side. I guess we could just call the function from the thunk expansion as well. > > > (gdb) bt > > #0 assemble_start_function (decl=0x7fe32f00, > > fnname=0x7fe41860 "_ZThn4_N7FooBase3BarEv") > > at /gnu/gcc/gcc-patched/gcc/varasm.c:1524 > > #1 0x007aa73c in cgraph_expand_function (node=0x7ff80c30) > > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1328 I think I would just add the resolve_unique_section into expand_thunk in cgraphunit.c > > #2 0x007ad210 in cgraph_optimize () > > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1567 > > #3 0x007ad69a in cgraph_finalize_compilation_unit () > > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1031 > > #4 0x004ce825 in cp_write_global_declarations () > > at /gnu/gcc/gcc-patched/gcc/cp/decl2.c:3974 > > #5 0x0080ed6d in toplev_main (argc=14, argv=0x5079f78) > > at /gnu/gcc/gcc-patched/gcc/toplev.c:591 > > #6 0x0060699f in main (argc=Cannot access memory at address 0x0 > > ) at /gnu/gcc/gcc-patched/gcc/main.c:36 > > Got any ideas how to fix this? > > Possibly related: this chunk of code in cp/method.c#use_thunk() > > > if (TARGET_USE_LOCAL_THUNK_ALIAS_P (function) > > && targetm.have_named_sections) > > { > > resolve_unique_section (function, 0, flag_function_sections); > > > > if (DECL_SECTION_NAME (function) != NULL && DECL_ONE_ONLY (function)) > > { > > resolve_unique_section (thunk_fndecl, 0, flag_function_sections); > > > > /* Output the thunk into the same section as function. */ > > DECL_SECTION_NAME (thunk_fndecl) = DECL_SECTION_NAME (function); > > } > > } I have no knowledge of the code and have no idea why named sections would be special here, unforutnately... Honza > > I noticed that the inner if (...) condition was not triggering, either > before or after the patch. It seems the comdat group for the thunk's target > function is not yet set so DECL_ONE_ONLY is false at this point (calling > emit_associated_thunks from expand_or_defer_fn). It looks like the intention > here was to emit thunks in the same comdat section as their targets, but it > may have stopped working (or perhaps never worked)? > > cheers, > DaveK > -- > (*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47218
On 14/12/2010 13:07, Jan Hubicka wrote: >> On Sat, Dec 11, 2010 at 2:38 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >>> Hi, >>> given that the patch was tested to solve the bootstrap problem, is it OK for mainline? >>> The problem is that some backends needs to know function section at expansion time to guess >>> if long/short jump variants are used. >> I think it makes sense, so consider it approved. > > Comitted as revision 167795. > Thanks, > Honza This caused PR47218: multiple definitions of non-virtual thunks appear at final link-time. There is something different about the way non-virtual thunks are output that causes this change to make them no longer emitted in COMDAT sections: when I revert or apply your patch, and compile the testcase from the dup PR47116(**), I see this difference in the generated source: > $ diff -pu test0.s test0-fixed.s > --- test0.s 2011-01-08 18:30:04.968750000 +0000 > +++ test0-fixed.s 2011-01-08 18:27:40.234375000 +0000 > @@ -13,7 +13,8 @@ LFB2: > ret > .cfi_endproc > LFE2: > - .text > + .section .text$_ZThn4_N7FooBase3BarEv,"x" > + .linkonce discard > .p2align 4,,15 > .globl __ZThn4_N7FooBase3BarEv > .def __ZThn4_N7FooBase3BarEv; .scl 2; .type 32;.endef It turns out that resolve_unique_function() is not called at all for the thunk function any more, where previously it was called from assemble_start_function called from cgraph_expand_function. This is because gimple_expand_cfg() isn't called for these thunk functions; they are emitted through a call chain that looks like: > (gdb) bt > #0 assemble_start_function (decl=0x7fe32f00, > fnname=0x7fe41860 "_ZThn4_N7FooBase3BarEv") > at /gnu/gcc/gcc-patched/gcc/varasm.c:1524 > #1 0x007aa73c in cgraph_expand_function (node=0x7ff80c30) > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1328 > #2 0x007ad210 in cgraph_optimize () > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1567 > #3 0x007ad69a in cgraph_finalize_compilation_unit () > at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1031 > #4 0x004ce825 in cp_write_global_declarations () > at /gnu/gcc/gcc-patched/gcc/cp/decl2.c:3974 > #5 0x0080ed6d in toplev_main (argc=14, argv=0x5079f78) > at /gnu/gcc/gcc-patched/gcc/toplev.c:591 > #6 0x0060699f in main (argc=Cannot access memory at address 0x0 > ) at /gnu/gcc/gcc-patched/gcc/main.c:36 Got any ideas how to fix this? Possibly related: this chunk of code in cp/method.c#use_thunk() > if (TARGET_USE_LOCAL_THUNK_ALIAS_P (function) > && targetm.have_named_sections) > { > resolve_unique_section (function, 0, flag_function_sections); > > if (DECL_SECTION_NAME (function) != NULL && DECL_ONE_ONLY (function)) > { > resolve_unique_section (thunk_fndecl, 0, flag_function_sections); > > /* Output the thunk into the same section as function. */ > DECL_SECTION_NAME (thunk_fndecl) = DECL_SECTION_NAME (function); > } > } I noticed that the inner if (...) condition was not triggering, either before or after the patch. It seems the comdat group for the thunk's target function is not yet set so DECL_ONE_ONLY is false at this point (calling emit_associated_thunks from expand_or_defer_fn). It looks like the intention here was to emit thunks in the same comdat section as their targets, but it may have stopped working (or perhaps never worked)? cheers, DaveK
On 08/01/2011 19:03, Jan Hubicka wrote: > I see, the thunks are still expanded on side. I guess we could just call the function > from the thunk expansion as well. >>> (gdb) bt >>> #0 assemble_start_function (decl=0x7fe32f00, >>> fnname=0x7fe41860 "_ZThn4_N7FooBase3BarEv") >>> at /gnu/gcc/gcc-patched/gcc/varasm.c:1524 >>> #1 0x007aa73c in cgraph_expand_function (node=0x7ff80c30) >>> at /gnu/gcc/gcc-patched/gcc/cgraphunit.c:1328 > > I think I would just add the resolve_unique_section into expand_thunk in cgraphunit.c Thanks, I'll give that a try. > I have no knowledge of the code and have no idea why named sections would > be special here, unforutnately... Nah, me neither, and I think it's an older can of worms in any case... cheers, DaveK
Index: varasm.c =================================================================== --- varasm.c (revision 167472) +++ varasm.c (working copy) @@ -1548,8 +1548,6 @@ if (CONSTANT_POOL_BEFORE_FUNCTION) output_constant_pool (fnname, decl); - resolve_unique_section (decl, 0, flag_function_sections); - /* Make sure the not and cold text (code) sections are properly aligned. This is necessary here in the case where the function has both hot and cold sections, because we don't want to re-set Index: cfgexpand.c =================================================================== --- cfgexpand.c (revision 167472) +++ cfgexpand.c (working copy) @@ -3949,6 +3949,10 @@ crtl->preferred_stack_boundary = STACK_BOUNDARY; cfun->cfg->max_jumptable_ents = 0; + /* Resovle the function section. Some targets, like ARM EABI rely on knowledge + of the function section at exapnsion time to predict distance of calls. */ + resolve_unique_section (current_function_decl, 0, flag_function_sections); + /* Expand the variables recorded during gimple lowering. */ timevar_push (TV_VAR_EXPAND); start_sequence ();