diff mbox

Handle short reads and EINTR in lto-plugin/simple-object

Message ID alpine.LSU.2.11.1403281417560.29142@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener March 28, 2014, 1:30 p.m. UTC 
On Wed, 26 Mar 2014, Richard Biener wrote:

> On March 26, 2014 4:51:58 PM CET, Ian Lance Taylor <iant@google.com> wrote:
> >On Wed, Mar 26, 2014 at 8:38 AM, Richard Biener <rguenther@suse.de>
> >wrote:
> >>
> >> -  got = read (descriptor, buffer, size);
> >> -  if (got < 0)
> >> +  do
> >>      {
> >> -      *errmsg = "read";
> >> -      *err = errno;
> >> -      return 0;
> >> +      got = read (descriptor, buffer, size);
> >> +      if (got < 0
> >> +         && errno != EINTR)
> >> +       {
> >> +         *errmsg = "read";
> >> +         *err = errno;
> >> +         return 0;
> >> +       }
> >> +      else
> >> +       {
> >> +         buffer += got;
> >> +         size -= got;
> >> +       }
> >
> >This appears to do the wrong thing if got < 0 && errno == EINTR.  In
> >that case it should not add got to buffer and size.
> 
> Uh, indeed. Will fix.
> 
> >> -  if (offset != lseek (obj->file->fd, offset, SEEK_SET)
> >> -       || length != read (obj->file->fd, secdata, length))
> >> +  if (!simple_object_internal_read (obj->file->fd, offset,
> >> +                                   secdata, length, &errmsg, &err))
> >
> >Hmmm, internal_read is meant to be, well, internal.  It's not declared
> >anywhere as far as I can see.
> 
> I can duplicate the stuff as well.
> 
> >Are you really seeing EINTR reads here?  That seems very odd to me,
> >since we are always just reading a local file.  But if you are seeing
> >it, I guess we should handle it.
> 
> Well, it's a shot in the dark... I definitely know short reads and EINTR happens more in virtual machines though. So handling it is an improvement.
> 
> I'll see if it fixes my problems and report back.

Ok, updated patch as below.  The patch _does_ fix the issues I run into
(well, previously 1 out of 4 compiles succeeded, now 4 out of 4 succeed,
whatever that proves ;))

LTO bootstrapped and tested on x86_64-unknown-linux-gnu, ok for trunk?

Thanks,
Richard.

2014-03-26  Richard Biener  <rguenther@suse.de>

	libiberty/
	* simple-object.c (simple_object_internal_read): Handle
	EINTR and short reads.

	lto-plugin/
	* lto-plugin.c (process_symtab): Use simple_object_internal_read.

Comments

Ian Lance Taylor March 28, 2014, 1:56 p.m. UTC | #1 
On Fri, Mar 28, 2014 at 6:30 AM, Richard Biener <rguenther@suse.de> wrote:
>
> 2014-03-26  Richard Biener  <rguenther@suse.de>
>
>         libiberty/
>         * simple-object.c (simple_object_internal_read): Handle
>         EINTR and short reads.
>
>         lto-plugin/
>         * lto-plugin.c (process_symtab): Use simple_object_internal_read.

This is OK.

Thanks.

Ian
diff mbox

Patch

Index: libiberty/simple-object.c
===================================================================
--- libiberty/simple-object.c	(revision 208812)
+++ libiberty/simple-object.c	(working copy)
@@ -63,8 +63,6 @@  simple_object_internal_read (int descrip
 			     unsigned char *buffer, size_t size,
 			     const char **errmsg, int *err)
 {
-  ssize_t got;
-
   if (lseek (descriptor, offset, SEEK_SET) < 0)
     {
       *errmsg = "lseek";
@@ -72,15 +70,26 @@  simple_object_internal_read (int descrip
       return 0;
     }
 
-  got = read (descriptor, buffer, size);
-  if (got < 0)
+  do
     {
-      *errmsg = "read";
-      *err = errno;
-      return 0;
+      ssize_t got = read (descriptor, buffer, size);
+      if (got == 0)
+	break;
+      else if (got > 0)
+	{
+	  buffer += got;
+	  size -= got;
+	}
+      else if (errno != EINTR)
+	{
+	  *errmsg = "read";
+	  *err = errno;
+	  return 0;
+	}
     }
+  while (size > 0);
 
-  if ((size_t) got < size)
+  if (size > 0)
     {
       *errmsg = "file too short";
       *err = 0;
Index: lto-plugin/lto-plugin.c
===================================================================
--- lto-plugin/lto-plugin.c	(revision 208812)
+++ lto-plugin/lto-plugin.c	(working copy)
@@ -39,6 +39,7 @@  along with this program; see the file CO
 #include <stdint.h>
 #endif
 #include <assert.h>
+#include <errno.h>
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
@@ -817,7 +818,7 @@  process_symtab (void *data, const char *
 {
   struct plugin_objfile *obj = (struct plugin_objfile *)data;
   char *s;
-  char *secdata;
+  char *secdatastart, *secdata;
 
   if (strncmp (name, LTO_SECTION_PREFIX, LTO_SECTION_PREFIX_LEN) != 0)
     return 1;
@@ -825,23 +826,40 @@  process_symtab (void *data, const char *
   s = strrchr (name, '.');
   if (s)
     sscanf (s, ".%" PRI_LL "x", &obj->out->id);
-  secdata = xmalloc (length);
+  secdata = secdatastart = xmalloc (length);
   offset += obj->file->offset;
-  if (offset != lseek (obj->file->fd, offset, SEEK_SET)
-	|| length != read (obj->file->fd, secdata, length))
+  if (offset != lseek (obj->file->fd, offset, SEEK_SET))
+    goto err;
+
+  do
     {
-      if (message)
-	message (LDPL_FATAL, "%s: corrupt object file", obj->file->name);
-      /* Force claim_file_handler to abandon this file.  */
-      obj->found = 0;
-      free (secdata);
-      return 0;
+      ssize_t got = read (obj->file->fd, secdata, length);
+      if (got == 0)
+	break;
+      else if (got > 0)
+	{
+	  secdata += got;
+	  length -= got;
+	}
+      else if (errno != EINTR)
+	goto err;
     }
+  while (length > 0);
+  if (length > 0)
+    goto err;
 
-  translate (secdata, secdata + length, obj->out);
+  translate (secdatastart, secdata, obj->out);
   obj->found++;
-  free (secdata);
+  free (secdatastart);
   return 1;
+
+err:
+  if (message)
+    message (LDPL_FATAL, "%s: corrupt object file", obj->file->name);
+  /* Force claim_file_handler to abandon this file.  */
+  obj->found = 0;
+  free (secdatastart);
+  return 0;
 }
 
 /* Callback used by gold to check if the plugin will claim FILE. Writes