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