[patch] Use LDPL_FATAL instead of assert

[patch] Use LDPL_FATAL instead of assert

[Rafael Espindola]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

The plugin currently asserts on all errors. This patch changes most of them
into calls to the message callback. The patch also makes the cleanup_handler
succeed when being called in consequence of an earlier error.

2009-10-15  Rafael Avila de Espindola  <espindola@google.com>

	* lto-plugin.c (message): New variable.
	(check): New function.
	(parse_table_entry, translate, write_resolution,add_output_files,
	exec_lto_wrapper,claim_file_handler, onload): Use check instead of
	assert.
	(cleanup_handler): Use check instead of assert. Remove the arguments
	file if it exists.

Cheers,
-- 
Rafael Ávila de Espíndola

[-- Attachment #2: message.patch --]
[-- Type: text/x-diff, Size: 8859 bytes --]

diff --git a/lto-plugin/lto-plugin.c b/lto-plugin/lto-plugin.c
index ae484a9..bfb9925 100644
--- a/lto-plugin/lto-plugin.c
+++ b/lto-plugin/lto-plugin.c
@@ -82,6 +82,7 @@ static ld_plugin_get_symbols get_symbols;
 static ld_plugin_register_cleanup register_cleanup;
 static ld_plugin_add_input_file add_input_file;
 static ld_plugin_add_input_library add_input_library;
+static ld_plugin_message message;
 
 static struct plugin_file_info *claimed_files = NULL;
 static unsigned int num_claimed_files = 0;
@@ -99,6 +100,23 @@ static bool debug;
 static bool nop;
 static char *resolution_file = NULL;
 
+static void
+check (bool gate, enum ld_plugin_level level, const char *text)
+{
+  if (gate)
+    return;
+
+  if (message)
+    message (level, text);
+  else
+    {
+      /* If there is no nicer way to inform the user, fallback to stderr. */
+      fprintf (stderr, "%s\n", text);
+      if (level == LDPL_FATAL)
+	abort ();
+    }
+}
+
 /* Parse an entry of the IL symbol table. The data to be parsed is pointed
    by P and the result is written in ENTRY. The slot number is stored in SLOT.
    Returns the address of the next entry. */
@@ -142,12 +160,12 @@ parse_table_entry (char *p, struct ld_plugin_symbol *entry, uint32_t *slot)
     entry->comdat_key = strdup (entry->comdat_key);
 
   t = *p;
-  assert (t <= 4);
+  check (t <= 4, LDPL_FATAL, "Invalid symbol kind found.");
   entry->def = translate_kind[t];
   p++;
 
   t = *p;
-  assert (t <= 3);
+  check (t <= 3, LDPL_FATAL, "Invalid symbol visibility found.");
   entry->visibility = translate_visibility[t];
   p++;
 
@@ -218,9 +236,9 @@ translate (Elf_Data *symtab, struct plugin_symtab *out)
     {
       n++;
       syms = realloc (syms, n * sizeof (struct ld_plugin_symbol));
-      assert (syms);
+      check (syms, LDPL_FATAL, "Could not allocate memory.");
       slots = realloc (slots, n * sizeof (uint32_t));
-      assert (slots);
+      check (slots, LDPL_FATAL, "Could not allocate memory.");
       data = parse_table_entry (data, &syms[n - 1], &slots[n - 1]);
     }
 
@@ -297,7 +315,7 @@ write_resolution (void)
     return;
 
   f = fopen (resolution_file, "w");
-  assert (f);
+  check (f, LDPL_FATAL, "Could not open file.");
 
   fprintf (f, "%d\n", num_claimed_files);
 
@@ -329,7 +347,7 @@ write_resolution (void)
 static void
 add_output_files (FILE *f)
 {
-  char fname[1000]; /* FIXME: Is this big enough? */
+  char fname[1000]; /* FIXME: Remove this restriction. */
 
   for (;;)
     {
@@ -339,7 +357,7 @@ add_output_files (FILE *f)
 	break;
 
       len = strlen (s);
-      assert (s[len - 1] == '\n');
+      check (s[len - 1] == '\n', LDPL_FATAL, "File name too long.");
       s[len - 1] = '\0';
 
       num_output_files++;
@@ -367,16 +385,16 @@ exec_lto_wrapper (char *argv[])
 
   /* Write argv to a file to avoid a command line that is too long. */
   t = asprintf (&at_args, "@%s/arguments", temp_obj_dir_name);
-  assert (t >= 0);
+  check (t >= 0, LDPL_FATAL, "asprintf failed.");
 
   args_name = at_args + 1;
   args = fopen (args_name, "w");
-  assert (args);
+  check (args, LDPL_FATAL, "Could not open arguments file.");
 
   t = writeargv (&argv[1], args);
-  assert (t == 0);
+  check (t == 0, LDPL_FATAL, "Could not write arguments.");
   t = fclose (args);
-  assert (t == 0);
+  check (t == 0, LDPL_FATAL, "Could not close arguments file.");
 
   new_argv[0] = argv[0];
   new_argv[1] = at_args;
@@ -392,25 +410,26 @@ exec_lto_wrapper (char *argv[])
 
 
   pex = pex_init (PEX_USE_PIPES, "lto-wrapper", NULL);
-  assert (pex != NULL);
+  check (pex != NULL, LDPL_FATAL, "Could not pex_init lto-wrapper.");
 
   errmsg = pex_run (pex, 0, new_argv[0], new_argv, NULL, NULL, &t);
-  assert (errmsg == NULL);
-  assert (t == 0);
+  check (errmsg == NULL, LDPL_FATAL, "Could not run lto-wrapper.");
+  check (t == 0, LDPL_FATAL, "Could not run lto-wrapper.");
 
   wrapper_output = pex_read_output (pex, 0);
-  assert (wrapper_output);
+  check (wrapper_output, LDPL_FATAL, "Could not read lto-wrapper output.");
 
   add_output_files (wrapper_output);
 
   t = pex_get_status (pex, 1, &status);
-  assert (t == 1);
-  assert (WIFEXITED (status) && WEXITSTATUS (status) == 0);
+  check (t == 1, LDPL_FATAL, "Could not get lto-wrapper exit status.");
+  check (WIFEXITED (status) && WEXITSTATUS (status) == 0,
+	 LDPL_FATAL,          "lto-wrapper failed.");
 
   pex_free (pex);
 
   t = unlink (args_name);
-  assert (t == 0);
+  check (t == 0, LDPL_FATAL, "Could not unlink arguments file.");
   free (at_args);
 }
 
@@ -493,8 +512,12 @@ all_symbols_read_handler (void)
 static enum ld_plugin_status
 cleanup_handler (void)
 {
+  /* Note: we cannot use LDPL_FATAL in here as that would produce
+     an infinite loop. */
   int t;
   unsigned i;
+  char *arguments;
+  struct stat buf;
 
   for (i = 0; i < num_claimed_files; i++)
     {
@@ -502,11 +525,23 @@ cleanup_handler (void)
       if (info->temp)
 	{
 	  t = unlink (info->name);
-	  assert (t == 0);
+	  check (t == 0, LDPL_ERROR, "Could not unlink temporary file.");
 	}
     }
+
+  /* If we are being called from an error handler, it is possible
+     that the arguments file is still exists. */
+  t = asprintf (&arguments, "%s/arguments", temp_obj_dir_name);
+  check (t >= 0, LDPL_ERROR, "asprintf failed.");
+  if (stat(arguments, &buf) == 0)
+    {
+      t = unlink (arguments);
+      check (t == 0, LDPL_ERROR, "Could not unlink arguments file.");
+    }
+  free (arguments);
+
   t = rmdir (temp_obj_dir_name);
-  assert (t == 0);
+  check (t == 0, LDPL_ERROR, "Could not remove temporary directory.");
 
   free_2 ();
   return LDPS_OK;
@@ -535,13 +570,13 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
       char *objname;
       int t = asprintf (&objname, "%s/obj%d.o",
 			temp_obj_dir_name, objnum);
-      assert (t >= 0);
+      check (t >= 0, LDPL_FATAL, "asprintf failed.");
       objnum++;
 
       fd = open (objname, O_RDWR | O_CREAT, 0666);
-      assert (fd > 0);
+      check (fd > 0, LDPL_FATAL, "Could not open/create temporary file.");
       offset = lseek (file->fd, file->offset, SEEK_SET);
-      assert (offset == file->offset);
+      check (offset == file->offset, LDPL_FATAL, "Could not seek.");
       while (size > 0)
 	{
 	  ssize_t r, written;
@@ -549,7 +584,8 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 	  off_t s = sizeof (buf) < size ? sizeof (buf) : size;
 	  r = read (file->fd, buf, s);
 	  written = write (fd, buf, r);
-	  assert (written = r);
+	  check (written == r, LDPL_FATAL,
+		 "Could not write to temporary file.");
 	  size -= r;
 	}
       lto_file.name = objname;
@@ -579,7 +615,7 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
 
   status = add_symbols (file->handle, lto_file.symtab.nsyms,
 			lto_file.symtab.syms);
-  assert (status == LDPS_OK);
+  check (status == LDPS_OK, LDPL_FATAL, "Could not add symbols.");
 
   *claimed = 1;
   num_claimed_files++;
@@ -594,7 +630,7 @@ claim_file_handler (const struct ld_plugin_input_file *file, int *claimed)
   if (file->offset != 0)
     {
       int t = unlink (lto_file.name);
-      assert (t == 0);
+      check (t == 0, LDPL_FATAL, "Could not unlink file.");
     }
   free (lto_file.name);
 
@@ -646,13 +682,16 @@ onload (struct ld_plugin_tv *tv)
   char *t;
 
   unsigned version = elf_version (EV_CURRENT);
-  assert (version != EV_NONE);
+  check (version != EV_NONE, LDPL_FATAL, "Invalid elf version.");
 
   p = tv;
   while (p->tv_tag)
     {
       switch (p->tv_tag)
 	{
+        case LDPT_MESSAGE:
+          message = p->tv_u.tv_message;
+          break;
 	case LDPT_REGISTER_CLAIM_FILE_HOOK:
 	  register_claim_file = p->tv_u.tv_register_claim_file;
 	  break;
@@ -683,22 +722,25 @@ onload (struct ld_plugin_tv *tv)
       p++;
     }
 
-  assert (register_claim_file);
-  assert (add_symbols);
+  check (register_claim_file, LDPL_FATAL, "register_claim_file not found.");
+  check (add_symbols, LDPL_FATAL, "add_symbols not found.");
   status = register_claim_file (claim_file_handler);
-  assert (status == LDPS_OK);
+  check (status == LDPS_OK, LDPL_FATAL,
+	 "Could not register the claim_file callback.");
 
   if (register_cleanup)
     {
       status = register_cleanup (cleanup_handler);
-      assert (status == LDPS_OK);
+      check (status == LDPS_OK, LDPL_FATAL,
+	     "Could not register the cleanup callback.");
     }
 
   if (register_all_symbols_read)
     {
-      assert (get_symbols);
+      check (get_symbols, LDPL_FATAL, "get_symbols not found.");
       status = register_all_symbols_read (all_symbols_read_handler);
-      assert (status == LDPS_OK);
+      check (status == LDPS_OK, LDPL_FATAL,
+	     "Could not register the all_symbols_read callback.");
     }
 
   temp_obj_dir_name = strdup ("tmp_objectsXXXXXX");

Re: [patch] Use LDPL_FATAL instead of assert

[Joseph S. Myers]

On Thu, 15 Oct 2009, Rafael Espindola wrote:

> +static void
> +check (bool gate, enum ld_plugin_level level, const char *text)
> +{
> +  if (gate)
> +    return;
> +
> +  if (message)
> +    message (level, text);

The following may not need fixing for the initial commit, but it would 
still be a good idea to fix them:

Is this callback interface defined to take translated or untranslated 
text?  If untranslated, there would be a problem with the callback knowing 
which textual domain to use for translation, so I'd guess it should be 
defined to take translated messages.  This means you should be translating 
the messages first, using dgettext to use the right domain (which I think 
should be "gcc" rather than inventing yet another domain for a few 
messages).  You also need to call bindtextdomain - see how cpplib does 
things for an example of one domain being used in a library in a program 
that mainly uses another domain (cpplib and gcc there, gcc and whatever 
domain the linker uses - it appears to be "gold" - here).  Then 
gcc/po/exgettext needs to get the messages extracted for translation into 
gcc/po/gcc.pot.

> +  check (t <= 4, LDPL_FATAL, "Invalid symbol kind found.");

See the GNU Coding Standards for diagnostics, which do not start with a 
capital letter or end with ".".  Likewise for many other diagnostics.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Use LDPL_FATAL instead of assert

[Cary Coutant]

> The following may not need fixing for the initial commit, but it would
> still be a good idea to fix them:
>
> Is this callback interface defined to take translated or untranslated
> text?  If untranslated, there would be a problem with the callback knowing
> which textual domain to use for translation, so I'd guess it should be
> defined to take translated messages.  This means you should be translating
> the messages first, using dgettext to use the right domain (which I think
> should be "gcc" rather than inventing yet another domain for a few
> messages).  You also need to call bindtextdomain - see how cpplib does
> things for an example of one domain being used in a library in a program
> that mainly uses another domain (cpplib and gcc there, gcc and whatever
> domain the linker uses - it appears to be "gold" - here).  Then
> gcc/po/exgettext needs to get the messages extracted for translation into
> gcc/po/gcc.pot.

Yes, the current interface assumes that they're already translated,
but it might simplify the plugin writer's job if we extended the
plugin interface so that the plugin could register its text domain
with the linker's plugin manager. Then the plugin could use
N_()/gettext_noop() so the strings can still be extracted, but the
linker would be responsible for doing the actual call to dgettext
using the provided domain. The bindtextdomain() call would still need
to be done in the plugin's "onload" routine.

>> +  check (t <= 4, LDPL_FATAL, "Invalid symbol kind found.");
>
> See the GNU Coding Standards for diagnostics, which do not start with a
> capital letter or end with ".".  Likewise for many other diagnostics.

I was going to say that perhaps we should follow gold's standards for
diagnostics instead, but this API is not intended to be specific to
one particular host application, so it probably does make sense to
conform to the same coding standards as the compiler itself.

-cary

Re: [patch] Use LDPL_FATAL instead of assert

[Joseph S. Myers]

On Thu, 15 Oct 2009, Cary Coutant wrote:

> I was going to say that perhaps we should follow gold's standards for
> diagnostics instead, but this API is not intended to be specific to
> one particular host application, so it probably does make sense to
> conform to the same coding standards as the compiler itself.

And if gold is intended to be part of the GNU system, it should also be 
following the standard formats, so "ld: error" or "ld:object-file: error" 
or similar; when building a program, the diagnostics from compiler, 
assembler and linker should all be following the uniform style.  I'm 
presuming that gold will add the "ld: " or similar part to the message 
passed in from the plugin.  (When the callback is missing, perhaps the 
plugin should say "lto-plugin: " when sending the message to stderr 
itself.)

Just looking at the gold source code, I don't see any obvious deviations 
from the GNU style there, so there may not be a conflict of standards 
anyway.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Use LDPL_FATAL instead of assert

[Cary Coutant]

> The plugin currently asserts on all errors. This patch changes most of them
> into calls to the message callback. The patch also makes the cleanup_handler
> succeed when being called in consequence of an earlier error.
>
> 2009-10-15  Rafael Avila de Espindola  <espindola@google.com>
>
>        * lto-plugin.c (message): New variable.
>        (check): New function.
>        (parse_table_entry, translate, write_resolution,add_output_files,
>        exec_lto_wrapper,claim_file_handler, onload): Use check instead of
>        assert.
>        (cleanup_handler): Use check instead of assert. Remove the arguments
>        file if it exists.

+  check (WIFEXITED (status) && WEXITSTATUS (status) == 0,
+	 LDPL_FATAL,          "lto-wrapper failed.");

There's a bunch of extraneous spaces here.

+  /* Note: we cannot use LDPL_FATAL in here as that would produce
+     an infinite loop. */

Good point! That should probably be documented in the header and the
wiki page. (Hope you didn't discover that the hard way!)

+  check (version != EV_NONE, LDPL_FATAL, "Invalid elf version.");

"elf" should be "ELF".

Other than Joseph's comment about message style (and provided it's OK
to defer the I18N stuff), the rest looks good.

-cary

Re: [patch] Use LDPL_FATAL instead of assert

[Cary Coutant]

> +  /* Note: we cannot use LDPL_FATAL in here as that would produce
> +     an infinite loop. */
>
> Good point! That should probably be documented in the header and the
> wiki page. (Hope you didn't discover that the hard way!)

Or... the linker's plugin manager should avoid calling the cleanup
handler while it's already running. I'll work up a patch for that.

-cary

Re: [patch] Use LDPL_FATAL instead of assert

[Joseph S. Myers]

On Thu, 15 Oct 2009, Cary Coutant wrote:

> Other than Joseph's comment about message style (and provided it's OK
> to defer the I18N stuff), the rest looks good.

If you defer the i18n issues (which I leave up to the plugin maintainers), 
please file a PR for them and mark it as blocking PR 40883.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [patch] Use LDPL_FATAL instead of assert

[Rafael Espindola]

> Yes, the current interface assumes that they're already translated,
> but it might simplify the plugin writer's job if we extended the
> plugin interface so that the plugin could register its text domain
> with the linker's plugin manager. Then the plugin could use
> N_()/gettext_noop() so the strings can still be extracted, but the
> linker would be responsible for doing the actual call to dgettext
> using the provided domain. The bindtextdomain() call would still need
> to be done in the plugin's "onload" routine.

I think it is better to just have the plugin translate the message. This keeps
the linker/plugin interface simpler.

>>> +  check (t <= 4, LDPL_FATAL, "Invalid symbol kind found.");
>>
>> See the GNU Coding Standards for diagnostics, which do not start with a
>> capital letter or end with ".".  Likewise for many other diagnostics.
>
> I was going to say that perhaps we should follow gold's standards for
> diagnostics instead, but this API is not intended to be specific to
> one particular host application, so it probably does make sense to
> conform to the same coding standards as the compiler itself.

Agree. Will update to use the gcc style.

> -cary
>

Cheers,
-- 
Rafael Ávila de Espíndola

Re: [patch] Use LDPL_FATAL instead of assert

[Ian Lance Taylor]

"Joseph S. Myers" <joseph@codesourcery.com> writes:

> On Thu, 15 Oct 2009, Cary Coutant wrote:
>
>> I was going to say that perhaps we should follow gold's standards for
>> diagnostics instead, but this API is not intended to be specific to
>> one particular host application, so it probably does make sense to
>> conform to the same coding standards as the compiler itself.
>
> And if gold is intended to be part of the GNU system, it should also be 
> following the standard formats, so "ld: error" or "ld:object-file: error" 
> or similar; when building a program, the diagnostics from compiler, 
> assembler and linker should all be following the uniform style.  I'm 
> presuming that gold will add the "ld: " or similar part to the message 
> passed in from the plugin.  (When the callback is missing, perhaps the 
> plugin should say "lto-plugin: " when sending the message to stderr 
> itself.)
>
> Just looking at the gold source code, I don't see any obvious deviations 
> from the GNU style there, so there may not be a conflict of standards 
> anyway.

Yes, gold is intended to follow the GNU style, just like gcc.  Cases
where gold fails to follow this style should be fixed.  I see that
there is one such case in Plugin_manager::add_input_file; that line is
doubly wrong since gold_error adds a newline anyhow.

Ian