public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] PR46667, fix section type conflicts
@ 2010-12-02  9:06 Chung-Lin Tang
  2010-12-03  9:52 ` Ramana Radhakrishnan
  2010-12-05 12:20 ` Jan Hubicka
  0 siblings, 2 replies; 10+ messages in thread
From: Chung-Lin Tang @ 2010-12-02  9:06 UTC (permalink / raw)
  To: gcc-patches

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

Hi,
this patch tries to fix the section type conflicts seen on ARM-Linux 
(which currently prevents libstdc++ from building on trunk) and the 
reported x86-64 fails when -freorder-blocks-and-partition is involved.

The ARM fails seem to be due to DECL_ONE_ONLY symbols being treated as 
added to the .text.unlikely section; I've added a call to 
resolve_unique_section() to fill in the DECL_SECTION_NAME and 
DECL_HAS_IMPLICIT_SECTION_NAME_P values as I see are needed.

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.

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.".

[-- Attachment #2: varasm.diff.txt --]
[-- Type: text/plain, Size: 1097 bytes --]

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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-02  9:06 [PATCH] PR46667, fix section type conflicts Chung-Lin Tang
@ 2010-12-03  9:52 ` Ramana Radhakrishnan
  2010-12-05 12:20 ` Jan Hubicka
  1 sibling, 0 replies; 10+ messages in thread
From: Ramana Radhakrishnan @ 2010-12-03  9:52 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: gcc-patches, honza


On Thu, 2010-12-02 at 17:05 +0800, Chung-Lin Tang wrote:
> Hi,
> this patch tries to fix the section type conflicts seen on ARM-Linux 
> (which currently prevents libstdc++ from building on trunk) and the 
> reported x86-64 fails when -freorder-blocks-and-partition is involved.
> 
> The ARM fails seem to be due to DECL_ONE_ONLY symbols being treated as 
> added to the .text.unlikely section; I've added a call to 
> resolve_unique_section() to fill in the DECL_SECTION_NAME and 
> DECL_HAS_IMPLICIT_SECTION_NAME_P values as I see are needed.

I can't approve or reject your patch but ..

I don't see how that is wrong. Looking at a backtrace it does appear as
though a decl that needs a unique section gets a .text.unlikely. I've
tested your patch and it restores the testing to something sane.

Can someone approve this part of the patch to unbreak ARM builds of
libstdc++ ? 

Thanks,
Ramana



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-02  9:06 [PATCH] PR46667, fix section type conflicts Chung-Lin Tang
  2010-12-03  9:52 ` Ramana Radhakrishnan
@ 2010-12-05 12:20 ` Jan Hubicka
  2010-12-07 17:17   ` Chung-Lin Tang
  2010-12-11 14:03   ` Jan Hubicka
  1 sibling, 2 replies; 10+ messages in thread
From: Jan Hubicka @ 2010-12-05 12:20 UTC (permalink / raw)
  To: Chung-Lin Tang; +Cc: gcc-patches

>
> 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;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-05 12:20 ` Jan Hubicka
@ 2010-12-07 17:17   ` Chung-Lin Tang
  2010-12-11 14:03   ` Jan Hubicka
  1 sibling, 0 replies; 10+ messages in thread
From: Chung-Lin Tang @ 2010-12-07 17:17 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches

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;
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-05 12:20 ` Jan Hubicka
  2010-12-07 17:17   ` Chung-Lin Tang
@ 2010-12-11 14:03   ` Jan Hubicka
  2010-12-13 17:00     ` Richard Guenther
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2010-12-11 14:03 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Chung-Lin Tang, gcc-patches, rguenther, rth

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 ();

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-11 14:03   ` Jan Hubicka
@ 2010-12-13 17:00     ` Richard Guenther
  2010-12-14 13:56       ` Jan Hubicka
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Guenther @ 2010-12-13 17:00 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Chung-Lin Tang, gcc-patches, rguenther, rth

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 ();
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-13 17:00     ` Richard Guenther
@ 2010-12-14 13:56       ` Jan Hubicka
  2011-01-08 19:12         ` Dave Korn
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2010-12-14 13:56 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Jan Hubicka, Chung-Lin Tang, gcc-patches, rguenther, rth

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2010-12-14 13:56       ` Jan Hubicka
@ 2011-01-08 19:12         ` Dave Korn
  2011-01-08 19:19           ` Jan Hubicka
  0 siblings, 1 reply; 10+ messages in thread
From: Dave Korn @ 2011-01-08 19:12 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Richard Guenther, Chung-Lin Tang, gcc-patches, rguenther, rth

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
-- 
(*) - http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47218

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2011-01-08 19:12         ` Dave Korn
@ 2011-01-08 19:19           ` Jan Hubicka
  2011-01-08 19:30             ` Dave Korn
  0 siblings, 1 reply; 10+ messages in thread
From: Jan Hubicka @ 2011-01-08 19:19 UTC (permalink / raw)
  To: Dave Korn
  Cc: Jan Hubicka, Richard Guenther, Chung-Lin Tang, gcc-patches,
	rguenther, rth

> 
>   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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] PR46667, fix section type conflicts
  2011-01-08 19:19           ` Jan Hubicka
@ 2011-01-08 19:30             ` Dave Korn
  0 siblings, 0 replies; 10+ messages in thread
From: Dave Korn @ 2011-01-08 19:30 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: Richard Guenther, Chung-Lin Tang, gcc-patches, rguenther, rth

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-01-08 19:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-02  9:06 [PATCH] PR46667, fix section type conflicts Chung-Lin Tang
2010-12-03  9:52 ` Ramana Radhakrishnan
2010-12-05 12:20 ` Jan Hubicka
2010-12-07 17:17   ` Chung-Lin Tang
2010-12-11 14:03   ` Jan Hubicka
2010-12-13 17:00     ` Richard Guenther
2010-12-14 13:56       ` Jan Hubicka
2011-01-08 19:12         ` Dave Korn
2011-01-08 19:19           ` Jan Hubicka
2011-01-08 19:30             ` Dave Korn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).