Re: [PATCH] - Proper hot/cold partitioning fixes patch.

[Zack Weinberg <zack at codesourcery dot com>]

Caroline Tice <ctice@apple.com> writes:

> This patch is the correct fix for the hot/cold partitioning problems
> that people expressed concerns about.

This looks good to me.  I have a small number of concerns below, but
they are minor; given the length of time this has been broken, please
check the patch in as is and then send a followup patch to address
those issues.

I apologize for the delay in reviewing this patch; I have had no spare
time lately.

> +   if (!cfun)
> +     internal_error ("Attempt to switch text sections without any code.");

We're trying to have a consistent style for ICE checks.  Please make
this "gcc_assert (cfun);", and similarly for other places where you
call internal_error.

> +   const char * hot_section_label;
> +   const char * cold_section_label;
> +   const char * hot_section_end_label;
> +   const char * cold_section_end_label;

No space between the * and the variable name.

> !   if (cfun
> !       && current_function_decl)
>       {
> !       if (!cfun->unlikely_text_section_name)
>   	{
> ! 	  if (flag_function_sections
> ! 	      && DECL_SECTION_NAME (current_function_decl))
> ! 	    {
> ! 	      name = xstrdup (TREE_STRING_POINTER (DECL_SECTION_NAME 
> ! 						   (current_function_decl)));
> ! 	      stripped_name = targetm.strip_name_encoding (name);
> ! 	      len = strlen (stripped_name);
> ! 	      buffer = (char *) xmalloc (len + 10);
> ! 	      sprintf (buffer, "%s%s", stripped_name, "_unlikely");
> ! 	      cfun->unlikely_text_section_name = ggc_strdup (buffer);
> ! 	      free (buffer);
> ! 	      free ((char *) name);
> ! 	    }
> ! 	  else
> ! 	    cfun->unlikely_text_section_name = 
> ! 	                                UNLIKELY_EXECUTED_TEXT_SECTION_NAME;
>   	}
>       }
> +   else
> +    internal_error 
> +      ("initialize_cold_section_name called without valid current_function_decl.");

In general the use of alloca is discouraged, but this is a perfect
example of a place where it *should* be used.  Like so:

   assert (cfun && current_function_decl);
   if (cfun->unlikely_text_section_name)
     return;

   if (flag_function_sections && DECL_SECTION_NAME (current_function_decl))
     {
       name = alloca (TREE_STRING_LENGTH (DECL_SECTION_NAME
                                          (current_function_decl)));
       strcpy (name, TREE_STRING_POINTER (DECL_SECTION_NAME
                                          (current_function_decl)));
       stripped_name = targetm.strip_name_encoding (name);
       buffer = alloca (strlen (stripped_name) + 10);
       strcpy (buffer, stripped_name);
       strcat (buffer, "_unlikely");
       cfun->unlikely_text_section_name = ggc_strdup (buffer);
    }
  else
    cfun->unlikely_text_section_name = UNLIKELY_EXECUTED_TEXT_SECTION_NAME;

It's still not pretty, though.  Seems to me there ought to be a better
way, but I don't know one.

>     if (flag_reorder_blocks_and_partition)
>       {
> +       enum in_section save_text_section;
> + 
>         save_text_section = in_section;
>         unlikely_text_section ();
> !       ASM_OUTPUT_LABEL (asm_out_file, cfun->cold_section_end_label);
>         text_section ();
> !       ASM_OUTPUT_LABEL (asm_out_file, cfun->hot_section_end_label);
>         if (save_text_section == in_unlikely_executed_text)
>   	unlikely_text_section ();
>       }

Shouldn't you just unconditionally switch back to whatever section
save_text_section says you were in?

zw