On Mon, Sep 26, 2011 at 9:22 AM, Nathan Froyd <nfr...@mozilla.com> wrote:
> On 9/23/2011 6:03 PM, Sriraman Tallam wrote:
>>
>> This patch adds a new linker plugin to re-order functions.
>
> This is great stuff. We were experimenting with using the coverage files to
> generate an ordering for --section-ordering-file, but this might be even
> better, will have to experiment with it.
>
> A couple of comments on the code itself:
>
>> Index: function_reordering_plugin/callgraph.h
>> +inline static Edge_list *
>> +make_edge_list (Edge *e)
>> +{
>> + Edge_list *list = (Edge_list *)malloc (sizeof (Edge_list));
>
> If you are going to use libiberty via hashtab.h, you might as well make use
> of the *ALLOC family of macros to make this and other allocations a little
> neater.
>
>> +/*Represents an edge in the call graph. */
>> +struct __edge__
>
> I think the usual convention is to call this edge_d or something similar,
> avoiding the profusion of underscores.
>
>> +void
>> +map_section_name_to_index (char *section_name, void *handle, int shndx)
>> +{
>> + void **slot;
>> + char *function_name;
>> +
>> + if (is_prefix_of (".text.hot.", section_name))
>> + function_name = section_name + 10 /* strlen (".text.hot.") */;
>> + else if (is_prefix_of (".text.unlikely.", section_name))
>> + function_name = section_name + 15 /* strlen (".text.unlikely.") */;
>> + else if (is_prefix_of (".text.cold.", section_name))
>> + function_name = section_name + 11 /* strlen (".text.cold.") */;
>> + else if (is_prefix_of (".text.startup.", section_name))
>> + function_name = section_name + 14 /* strlen (".text.startup.") */;
>> + else
>> + function_name = section_name + 6 /*strlen (".text.") */;
>
> You don't handle plain .text; this is assuming -ffunction-sections, right?
> Can this limitation be easily removed?
You need to compile with -ffunction-sections or the individual
sections cannot be re-ordered. That is why I assumed a .text.* prefix
before every function section. Did you have something else in mind?
Thanks for your other comments. I will make those changes.
-Sri.
>
> I think it is customary to put the comment after the semicolon.
>
> Might just be easier to say something like:
>
> const char *sections[] = { ".text.hot", ... }
> char *function_name = NULL;
>
> for (i = 0; i < ARRAY_SIZE (sections); i++)
> if (is_prefix_of (sections[i], section_name))
> {
> function_name = section_name + strlen (sections[i]);
> break;
> }
>
> if (!function_name)
> function_name = section_name + 6; /* strlen (".text.") */
>
> I guess that's not much shorter.
>
>> +void
>> +cleanup ()
>> +{
>> + Node *node;
>> + Edge *edge;
>> +
>> + /* Free all nodes and edge_lists. */
>> + for (node = node_chain; node != NULL; )
>> + {
>> + Node *next_node = node->next;
>> + Edge_list *it;
>> + for (it = node->edge_list; it != NULL; )
>> + {
>> + Edge_list *next_it = it->next;
>> + free (it);
>> + it = next_it;
>> + }
>
> Write a free_edge_list function, since you do this once here and twice
> below.
>
> -Nathan
>
