Patchwork [google,4.7] Minor Function reordering plugin enhancements.

login
register
mail settings
Submitter Sriraman Tallam
Date Feb. 14, 2013, 12:19 a.m.
Message ID <CAAs8Hmy3Xa1GxNeDRmmFO5fo6wMb0vdV4ZhnzTO4PU4+mDtTdA@mail.gmail.com>
Download mbox | patch
Permalink /patch/220305/
State New
Headers show

Comments

Sriraman Tallam - Feb. 14, 2013, 12:19 a.m.
Hi,

   I have attached a patch for the reordering plugin to display
appropriate error messages when incorrect options are passed or when
some API is unavailable.

   The plugin will use the ld_plugin_message linker API to flag errors
(fatal or non-fatal) when incorrect options are passed or when the
required API is not available in the invoking linker. If the message
API is missing, then it falls back to using fprintf.

Thanks
Sri
Xinliang David Li - Feb. 14, 2013, 12:32 a.m.
when split_segment is specified but the API does not exist, why not
making it fatal ? Will it crash at some point when the null pointer is
referenced later?

David

On Wed, Feb 13, 2013 at 4:19 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Hi,
>
>    I have attached a patch for the reordering plugin to display
> appropriate error messages when incorrect options are passed or when
> some API is unavailable.
>
>    The plugin will use the ld_plugin_message linker API to flag errors
> (fatal or non-fatal) when incorrect options are passed or when the
> required API is not available in the invoking linker. If the message
> API is missing, then it falls back to using fprintf.
>
> Thanks
> Sri
Sriraman Tallam - Feb. 14, 2013, 12:39 a.m.
On Wed, Feb 13, 2013 at 4:32 PM, Xinliang David Li <davidxl@google.com> wrote:
> when split_segment is specified but the API does not exist, why not
> making it fatal ? Will it crash at some point when the null pointer is
> referenced later?

It cannot be referenced later as the handlers will not get registered.
I will make this fatal too however.

Thanks
Sri

>
> David
>
> On Wed, Feb 13, 2013 at 4:19 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> Hi,
>>
>>    I have attached a patch for the reordering plugin to display
>> appropriate error messages when incorrect options are passed or when
>> some API is unavailable.
>>
>>    The plugin will use the ld_plugin_message linker API to flag errors
>> (fatal or non-fatal) when incorrect options are passed or when the
>> required API is not available in the invoking linker. If the message
>> API is missing, then it falls back to using fprintf.
>>
>> Thanks
>> Sri

Patch

Index: function_reordering_plugin.c
===================================================================
--- function_reordering_plugin.c	(revision 196036)
+++ function_reordering_plugin.c	(working copy)
@@ -69,6 +69,7 @@  enum ld_plugin_status claim_file_hook (const struc
                                        int *claimed);
 enum ld_plugin_status all_symbols_read_hook ();
 
+static ld_plugin_message message = NULL;
 static ld_plugin_register_claim_file register_claim_file_hook = NULL;
 static ld_plugin_register_all_symbols_read
   register_all_symbols_read_hook = NULL;
@@ -88,8 +89,6 @@  static ld_plugin_unique_segment_for_sections uniqu
 
 static char *out_file = NULL;
 
-static int is_api_exist = 0;
-
 /* The plugin does nothing when no-op is 1.  */
 static int no_op = 0;
 
@@ -105,12 +104,30 @@  void get_filename (const char *name)
   strcpy (out_file, name);
 }
 
+/* MSG_FATAL prints a format string and aborts.  Uses the plugin API if
+   available, otherwise falls back to using fprintf.  */
+
+#define MSG_FATAL(...) \
+  if (message) { \
+    message (LDPL_FATAL, __VA_ARGS__); } \
+  else { \
+    fprintf (stderr, "fatal: " __VA_ARGS__); abort (); }
+
+/* MSG_ERROR prints a format string. Uses the plugin API if
+   available, otherwise falls back to using fprintf.  */
+
+#define MSG_ERROR(...) \
+  if (message) { \
+    message (LDPL_ERROR, __VA_ARGS__); } \
+  else { \
+    fprintf (stderr, "error: " __VA_ARGS__); }
+
 /* Process options to plugin.  Options with prefix "group=" are special.
    They specify the type of grouping. The option "group=none" makes the
    plugin do nothing.   Options with prefix "file=" set the output file
    where the final function order must be stored.  Option "segment=none"
    does not place the cold code in a separate ELF segment.  */
-void
+static int
 process_option (const char *name)
 {
   const char *option_group = "group=";
@@ -124,13 +141,13 @@  process_option (const char *name)
 	no_op = 1;
       else
 	no_op = 0;
-      return;
+      return 0;
     }
   /* Check if option is "file=" */
   else if (strncmp (name, option_file, strlen (option_file)) == 0)
     {
       get_filename (name + strlen (option_file));
-      return;
+      return 0;
     }
   /* Check if options is "split_segment=[yes|no]"  */
   else if (strncmp (name, option_segment, strlen (option_segment)) == 0)
@@ -139,19 +156,18 @@  process_option (const char *name)
       if (strcmp (option_val, "no") == 0)
 	{
 	  split_segment = 0;
-	  return;
+	  return 0;
 	}
       else if (strcmp (option_val, "yes") == 0)
 	{
 	  split_segment = 1;
-	  return;
+	  return 0;
 	}
     }
 
-  /* Unknown option, set no_op to 1.  */
-  no_op = 1;
-  fprintf (stderr, "Unknown option to function reordering plugin :%s\n",
-	   name);
+  /* Flag error on unknown plugin option.  */
+  MSG_ERROR ("Unknown option to function reordering plugin :%s\n", name);
+  return 1;
 }
 
 /* Plugin entry point.  */
@@ -168,10 +184,14 @@  onload (struct ld_plugin_tv *tv)
         case LDPT_GOLD_VERSION:
           break;
         case LDPT_OPTION:
-	  process_option (entry->tv_u.tv_string);
+	  if (process_option (entry->tv_u.tv_string) == 1)
+	    return LDPS_ERR;
 	  /* If no_op is set, do not do anything else.  */
 	  if (no_op) return LDPS_OK;
 	  break;
+	case LDPT_MESSAGE:
+	  message = *entry->tv_u.tv_message;
+	  break;
         case LDPT_REGISTER_CLAIM_FILE_HOOK:
 	  register_claim_file_hook = *entry->tv_u.tv_register_claim_file;
           break;
@@ -211,21 +231,29 @@  onload (struct ld_plugin_tv *tv)
 
   assert (!no_op);
 
-  if (register_all_symbols_read_hook != NULL
-      && register_claim_file_hook != NULL
-      && get_input_section_count != NULL
-      && get_input_section_type != NULL
-      && get_input_section_name != NULL
-      && get_input_section_contents != NULL
-      && update_section_order != NULL
-      && allow_section_ordering != NULL
-      && allow_unique_segment_for_sections != NULL
-      && unique_segment_for_sections != NULL)
-    is_api_exist = 1;
-  else
-    return LDPS_OK;
+  /* If the API for code reordering is missing, abort!  */
+  if (register_all_symbols_read_hook == NULL
+      || register_claim_file_hook == NULL
+      || get_input_section_count == NULL
+      || get_input_section_type == NULL
+      || get_input_section_name == NULL
+      || get_input_section_contents == NULL
+      || update_section_order == NULL
+      || allow_section_ordering == NULL)
+    {
+      MSG_FATAL ("API for code reordering not available\n");
+    }
 
-  /* Register handlers.  */
+  /* If segment splitting is desired and the API is missing, flag error.  */
+  if (split_segment == 1
+      && (allow_unique_segment_for_sections == NULL
+          || unique_segment_for_sections == NULL))
+    {
+      MSG_ERROR ("Segment splitting API not available for --split_segment\n");
+      return LDPS_ERR;
+    }
+
+  /* Register handlers.  */  
   assert ((*register_all_symbols_read_hook) (all_symbols_read_hook)
 	   == LDPS_OK);
   assert ((*register_claim_file_hook) (claim_file_hook)
@@ -246,9 +274,6 @@  claim_file_hook (const struct ld_plugin_input_file
 
   (void) claimed;
 
-  /* Plugin APIs are supported if this is called.  */
-  assert (is_api_exist);
-
   if (is_ordering_specified == 0)
     {
       /* Inform the linker to prepare for section reordering.  */
@@ -315,9 +340,6 @@  all_symbols_read_hook (void)
   unsigned int *shndx;
   FILE *fp = NULL;
 
-  /* Plugin APIs are supported if this is called.  */
-  assert (is_api_exist);
-
   if (is_callgraph_empty ())
     return LDPS_OK;