diff mbox

put exception tables for comdat functions in comdat, too

Message ID 501F3CA0.3040106@codesourcery.com
State New
Headers show

Commit Message

Sandra Loosemore Aug. 6, 2012, 3:40 a.m. UTC 
For C++ functions that are emitted to a comdat section (template 
instantiations and the like), the corresponding exception handling 
information is presently being emitted to the non-comdat 
.gcc_except_table section.  This means that you can end up with multiple 
copies of the exception tables in the executable from different objects, 
even though the linker can remove all but one copy of the function it 
goes with.

We do have a test case for this, but I don't know how to mash it into 
the Dejagnu framework.  My hand-testing involved comparing the 
before/after output of readelf to see the new exception table sections 
in the group, and verifying that the size of the executable is smaller 
with the patch than without it.

Otherwise, I bootstrapped and regression tested on x86_64, and also 
tested on mipsisa32r2-sde-elf since I had an existing build tree for 
that set up.  This patch has also been present in our local source base 
for a while so it's been well-tested in that context.  OK for mainline?

-Sandra


2012-08-04  Paul Brook  <paul@codesourcery.com>
	    Sandra Loosemore  <sandra@codesourcery.com>

	gcc/
	* except.c (switch_to_exception_section): Place tables for
	DECL_ONE_ONLY functions in comdat groups.

Comments

Richard Henderson Aug. 6, 2012, 5:45 p.m. UTC | #1 
On 08/05/2012 08:40 PM, Sandra Loosemore wrote:
> 2012-08-04  Paul Brook  <paul@codesourcery.com>
>         Sandra Loosemore  <sandra@codesourcery.com>
> 
>     gcc/
>     * except.c (switch_to_exception_section): Place tables for
>     DECL_ONE_ONLY functions in comdat groups.

Mostly ok.

> -	      s = get_section (section_name, flags, NULL);
> +	      s = get_section (section_name, flags, current_function_decl);

Not correct, since we're not putting the function in that section.
We have no decl for the actual eh data.

Though that might be a good cleanup for except.c...



r~
Sandra Loosemore Aug. 7, 2012, 12:45 a.m. UTC | #2 
On 08/06/2012 11:45 AM, Richard Henderson wrote:
> On 08/05/2012 08:40 PM, Sandra Loosemore wrote:
>> 2012-08-04  Paul Brook<paul@codesourcery.com>
>>          Sandra Loosemore<sandra@codesourcery.com>
>>
>>      gcc/
>>      * except.c (switch_to_exception_section): Place tables for
>>      DECL_ONE_ONLY functions in comdat groups.
>
> Mostly ok.
>
>> -	      s = get_section (section_name, flags, NULL);
>> +	      s = get_section (section_name, flags, current_function_decl);
>
> Not correct, since we're not putting the function in that section.
> We have no decl for the actual eh data.

This use of the function decl doesn't indicate that we're putting the 
function in this section; it represents the group symbol for the comdat 
group the section belongs in.  Without this part of the patch, there is 
nothing to tie the section being used for the eh tables for fnname to 
the comdat group for fnname, and default_elf_asm_named_section segfaults 
on the assumption that all comdat sections have a non-null decl for the 
purposes of the GroupName,comdat assembler syntax.

Compare to default_function_rodata_section in varasm.c for an existing 
similar call to get_section.

OK to commit the patch as originally posted?

-Sandra
Richard Henderson Aug. 7, 2012, 3:28 p.m. UTC | #3 
On 08/06/2012 05:45 PM, Sandra Loosemore wrote:
> OK to commit the patch as originally posted?

Yes.

I mis-read skimming the part of that function about relocations.


r~
diff mbox

Patch

Index: gcc/except.c
===================================================================
--- gcc/except.c	(revision 190149)
+++ gcc/except.c	(working copy)
@@ -2777,11 +2777,16 @@  switch_to_exception_section (const char 
 	    flags = SECTION_WRITE;
 
 #ifdef HAVE_LD_EH_GC_SECTIONS
-	  if (flag_function_sections)
+	  if (flag_function_sections
+	      || (DECL_ONE_ONLY (current_function_decl) && HAVE_COMDAT_GROUP))
 	    {
 	      char *section_name = XNEWVEC (char, strlen (fnname) + 32);
+	      /* The EH table must match the code section, so only mark
+		 it linkonce if we have COMDAT groups to tie them together.  */
+	      if (DECL_ONE_ONLY (current_function_decl) && HAVE_COMDAT_GROUP)
+		flags |= SECTION_LINKONCE;
 	      sprintf (section_name, ".gcc_except_table.%s", fnname);
-	      s = get_section (section_name, flags, NULL);
+	      s = get_section (section_name, flags, current_function_decl);
 	      free (section_name);
 	    }
 	  else