diff mbox

[google,4.7] Minor Function reordering plugin enhancements.

Message ID CAAs8Hmzj9oP7cXxGZAr8Ek_x-K+fWWBNpOsqYTY3RRH2_D+Dag@mail.gmail.com
State New
Headers show

Commit Message

Sriraman Tallam Feb. 14, 2013, 12:41 a.m. UTC
Updated patch attached.

Thanks
Sri

On Wed, Feb 13, 2013 at 4:39 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> 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

Comments

Xinliang David Li Feb. 14, 2013, 1:03 a.m. UTC | #1
ok.

thanks,

David

On Wed, Feb 13, 2013 at 4:41 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> Updated patch attached.
>
> Thanks
> Sri
>
> On Wed, Feb 13, 2013 at 4:39 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>> 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
diff mbox

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,20 +231,27 @@  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");
+    }
 
+  /* 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_FATAL ("Segment splitting API not available for split_segment\n");
+    }
+
   /* Register handlers.  */
   assert ((*register_all_symbols_read_hook) (all_symbols_read_hook)
 	   == LDPS_OK);
@@ -246,9 +273,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 +339,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;