Re: [PATCH] Fix size & type for cold partition names (hot-cold function partitioning)

[Caroline Tice <cmtice at google dot com>]

It took  me a while to get a test case I'm happy with, so I'm
re-submitting the whole patch for approval.

2015-03-27  Caroline Tice  <cmtice@google.com>

        * final.c (final_scan_insn): Change 'cold_function_name' to
        'cold_partition_name' and make it a global variable; also output
        assembly to give it a 'FUNC' type, if appropriate.
        * varasm.c (cold_partition_name): Declare and initialize global
        variable.
        (assemble_start_function): Re-set value for cold_partition_name.
        (assemble_end_function): Output assembly to calculate size of cold
        partition, and associate size with name, if appropriate.
        * varash.h (cold_partition_name): Add extern declaration for global
        variable.
        * testsuite/gcc.dg/tree-prof/cold_partition_label.c: Add dg-final-use
        to scan assembly for cold partition name and size.


I've attached the whole patch, with the testcase (which is the only
thing that has changed since the previous patch).

-- Caroline Tice
cmtice@google.com

On Mon, Dec 8, 2014 at 1:06 PM, Jeff Law <law@redhat.com> wrote:
> On 12/05/14 15:41, Caroline Tice wrote:
>>
>> When hot/cold function splitting occurs, a symbol is generated for the
>> cold partition, and gets output in the assembly & debug info, but the
>> symbol currently gets a size of 0 and a type of NOTYPE, as in this
>> example (on x86_64-linux) from the cold_partition_label test in the
>> testsuite:
>>
>> $ readelf -sW cold_partition_label.x02 | grep foo
>>      36: 0000000000400450     0 NOTYPE  LOCAL  DEFAULT   12 foo.cold.0
>>      58: 0000000000400490    43 FUNC    GLOBAL DEFAULT   12 foo
>> $
>>
>> This patch fixes this by calculating the right size for the partition,
>> and outputing the size and type fo the cold partition symbol.  After
>> applying this patch and looking at the same test, I get:
>>
>> $ readelf -sW cold_partition_label.x02 | grep foo
>>      36: 0000000000400450    29 FUNC    LOCAL  DEFAULT   12 foo.cold.0
>>      58: 0000000000400490    43 FUNC    GLOBAL DEFAULT   12 foo
>> $
>>
>> This patch has been tested by bootstrapping the compiler, running the
>> dejagnu testsuite with no regressions, and checked as shown above that
>> it fixes the original problem.  Is this patch OK to commit to ToT?
>>
>> -- Caroline Tice
>> cmtice@google.com
>>
>>   2014-12-05  Caroline Tice  <cmtice@google.com>
>>
>>          * final.c (final_scan_insn): Change 'cold_function_name' to
>>          'cold_partition_name' and make it a global variable; also output
>>          assembly to give it a 'FUNC' type, if appropriate.
>>          * varasm.c (cold_partition_name): Declare and initialize global
>>          variable.
>>          (assemble_start_function): Re-set value for cold_partition_name.
>>          (assemble_end_function): Output assembly to calculate size of
>> cold
>>          partition, and associate size with name, if appropriate.
>>          * varash.h (cold_partition_name): Add extern declaration for
>> global
>>          variable.
>
> OK for the trunk.  I'm ever-so-slightly worried about the PA and PTX ports
> are they're probably the most sensitive to types.  But we'll deal with them
> if they complain.
>
> Adding a testcase would be helpful.  I believe some of the LTO tests use
> readelf, so there should be some infrastructure you can factor out and
> reuse.
>
> jeff

Index: gcc/final.c
===================================================================
--- gcc/final.c	(revision 221669)
+++ gcc/final.c	(working copy)
@@ -2236,10 +2236,16 @@
 	     suffixing "cold" to the original function's name.  */
 	  if (in_cold_section_p)
 	    {
-	      tree cold_function_name
+	      cold_function_name
 		= clone_function_name (current_function_decl, "cold");
+#ifdef ASM_DECLARE_FUNCTION_NAME
+	      ASM_DECLARE_FUNCTION_NAME (asm_out_file,
+					 IDENTIFIER_POINTER (cold_function_name),
+					 current_function_decl);
+#else
 	      ASM_OUTPUT_LABEL (asm_out_file,
 				IDENTIFIER_POINTER (cold_function_name));
+#endif
 	    }
 	  break;
 
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c	(revision 221669)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c	(working copy)
@@ -35,4 +35,6 @@
   return 0;
 }
 
+/* { dg-final-use { scan-assembler "foo\[._\]+cold\[\._\]+0" } } */
+/* { dg-final-use { scan-assembler "size\[ \ta-zA-Z0-0\]+foo\[._\]+cold\[\._\]+0" } } */
 /* { dg-final-use { cleanup-saved-temps } } */
Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 221669)
+++ gcc/varasm.c	(working copy)
@@ -187,6 +187,13 @@
    at the cold section.  */
 bool in_cold_section_p;
 
+/* The following global holds the "function name" for the code in the
+   cold section of a function, if hot/cold function splitting is enabled
+   and there was actually code that went into the cold section.  A
+   pseudo function name is needed for the cold section of code for some
+   debugging tools that perform symbolization. */
+tree cold_function_name = NULL_TREE;
+
 /* A linked list of all the unnamed sections.  */
 static GTY(()) section *unnamed_sections;
 
@@ -1719,6 +1726,7 @@
       ASM_GENERATE_INTERNAL_LABEL (tmp_label, "LCOLDE", const_labelno);
       crtl->subsections.cold_section_end_label = ggc_strdup (tmp_label);
       const_labelno++;
+      cold_function_name = NULL_TREE;
     }
   else
     {
@@ -1856,6 +1864,10 @@
 
       save_text_section = in_section;
       switch_to_section (unlikely_text_section ());
+      if (cold_function_name != NULL_TREE)
+	ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
+				   IDENTIFIER_POINTER (cold_function_name),
+				   decl);
       ASM_OUTPUT_LABEL (asm_out_file, crtl->subsections.cold_section_end_label);
       if (first_function_block_is_cold)
 	switch_to_section (text_section);
Index: gcc/varasm.h
===================================================================
--- gcc/varasm.h	(revision 221669)
+++ gcc/varasm.h	(working copy)
@@ -20,6 +20,13 @@
 #ifndef GCC_VARASM_H
 #define GCC_VARASM_H
 
+/* The following global holds the "function name" for the code in the
+   cold section of a function, if hot/cold function splitting is enabled
+   and there was actually code that went into the cold section.  A
+   pseudo function name is needed for the cold section of code for some
+   debugging tools that perform symbolization. */
+extern tree cold_function_name;
+
 extern tree tree_output_constant_def (tree);
 extern void make_decl_rtl (tree);
 extern rtx make_decl_rtl_for_debug (tree);