Questionable code in gcov-io.c

Questionable code in gcov-io.c

[Marek Polacek]

While implementing a warning I noticed this in gcov-io.c:

 187   else if (mode == 0)
 188     {
 189       struct stat st;
 190 
 191       if (fstat (fd, &st) < 0)
 192         {
 193           fclose (gcov_var.file);
 194           gcov_var.file = 0;
 195           return 0;
 196         }
 197       if (st.st_size != 0)
 198         gcov_var.mode = 1;
 199       else
 200         gcov_var.mode = mode * 2 + 1;
 201     }
 202   else
 203     gcov_var.mode = mode * 2 + 1;

It seems that lines 198 and 200 do the same thing, at line 200 we know that
mode == 0, so we just assign 1.  Should we just remove the condition on line 197?

This has been introduced in Jakub's r78281.

	Marek

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

On 10/12/16 08:10, Marek Polacek wrote:
> While implementing a warning I noticed this in gcov-io.c:
>
>  187   else if (mode == 0)
>  188     {
>  189       struct stat st;
>  190
>  191       if (fstat (fd, &st) < 0)
>  192         {
>  193           fclose (gcov_var.file);
>  194           gcov_var.file = 0;
>  195           return 0;
>  196         }
>  197       if (st.st_size != 0)
>  198         gcov_var.mode = 1;
>  199       else
>  200         gcov_var.mode = mode * 2 + 1;
>  201     }
>  202   else
>  203     gcov_var.mode = mode * 2 + 1;
>
> It seems that lines 198 and 200 do the same thing, at line 200 we know that
> mode == 0, so we just assign 1.  Should we just remove the condition on line 197?

ITYM lines 197 -> 203.  I.e. remove the entire if that;s inside the 'mode == 0' 
branch and make line 203 unconditional.

nathan

Re: Questionable code in gcov-io.c

[Bernd Schmidt]

On 10/12/2016 02:10 PM, Marek Polacek wrote:
> While implementing a warning I noticed this in gcov-io.c:
>
>  187   else if (mode == 0)
>  188     {
>  189       struct stat st;
>  190
>  191       if (fstat (fd, &st) < 0)
>  192         {
>  193           fclose (gcov_var.file);
>  194           gcov_var.file = 0;
>  195           return 0;
>  196         }
>  197       if (st.st_size != 0)
>  198         gcov_var.mode = 1;
>  199       else
>  200         gcov_var.mode = mode * 2 + 1;
>  201     }
>  202   else
>  203     gcov_var.mode = mode * 2 + 1;
>
> It seems that lines 198 and 200 do the same thing, at line 200 we know that
> mode == 0, so we just assign 1.  Should we just remove the condition on line 197?

The intention seems to be that a negative gcov_var.mode means we're 
writing, so for a size zero file maybe that's the expected result. But 
of course none of the existing code is going to expect that.

There are more oddities here...

Before the quoted piece of code, we test for mode > 0, so in line 203 we 
know that mode is < 0. I don't really see what the "mode * 2 + 1" 
expression is supposed to do anyway, given that the caller could pass in 
any positive/negative number and expect the same results as for 1 and 
-1. I think line 203 should just use -1. There's one further occurrence 
of that expression which should probably be modified.

The function comment also seems to have an issue:

/* Open a gcov file. NAME is the name of the file to open and MODE
    indicates whether a new file should be created, or an existing file
    opened. If MODE is >= 0 an existing file will be opened, if
    possible, and if MODE is <= 0, a new file will be created. Use
    MODE=0 to attempt to reopen an existing file and then fall back on
    creating a new one.  If MODE < 0, the file will be opened in
    read-only mode.  Otherwise it will be opened for modification.
    Return zero on failure, >0 on opening an existing file and <0 on
    creating a new one.  */

This suggests that with MODE < 0, we'll create a file in read-only mode, 
which is nonsensical. The code suggests that the comment should read " > 0".


Bernd

Re: Questionable code in gcov-io.c

[Jakub Jelinek]

On Wed, Oct 12, 2016 at 02:23:36PM +0200, Bernd Schmidt wrote:
> >It seems that lines 198 and 200 do the same thing, at line 200 we know that
> >mode == 0, so we just assign 1.  Should we just remove the condition on line 197?
> 
> The intention seems to be that a negative gcov_var.mode means we're writing,
> so for a size zero file maybe that's the expected result. But of course none
> of the existing code is going to expect that.
> 
> There are more oddities here...

Unfortunately I really don't remember, and it seems we don't have the
posting in gcc-patches archive at all.
Found it in http://marc.info/?l=gcc-patches&m=107747608611324 though.

	Jakub

Re: Questionable code in gcov-io.c

[Marek Polacek]

On Wed, Oct 12, 2016 at 08:14:58AM -0400, Nathan Sidwell wrote:
> On 10/12/16 08:10, Marek Polacek wrote:
> > While implementing a warning I noticed this in gcov-io.c:
> > 
> >  187   else if (mode == 0)
> >  188     {
> >  189       struct stat st;
> >  190
> >  191       if (fstat (fd, &st) < 0)
> >  192         {
> >  193           fclose (gcov_var.file);
> >  194           gcov_var.file = 0;
> >  195           return 0;
> >  196         }
> >  197       if (st.st_size != 0)
> >  198         gcov_var.mode = 1;
> >  199       else
> >  200         gcov_var.mode = mode * 2 + 1;
> >  201     }
> >  202   else
> >  203     gcov_var.mode = mode * 2 + 1;
> > 
> > It seems that lines 198 and 200 do the same thing, at line 200 we know that
> > mode == 0, so we just assign 1.  Should we just remove the condition on line 197?
> 
> ITYM lines 197 -> 203.  I.e. remove the entire if that;s inside the 'mode ==
> 0' branch and make line 203 unconditional.

Yes, sorry for sloppy wording.  I'm testing a patch.

Thanks,

	Marek

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

On 10/12/16 09:43, Marek Polacek wrote:

>> ITYM lines 197 -> 203.  I.e. remove the entire if that;s inside the 'mode ==
>> 0' branch and make line 203 unconditional.
>
> Yes, sorry for sloppy wording.  I'm testing a patch.

heh, so am I :)

nathan

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

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

On 10/12/16 09:43, Marek Polacek wrote:

>> ITYM lines 197 -> 203.  I.e. remove the entire if that;s inside the 'mode ==
>> 0' branch and make line 203 unconditional.
>
> Yes, sorry for sloppy wording.  I'm testing a patch.

Here's the patch I've tested (but not bootstrapped).  As the incoming mode is 
-1, 0 or +1 we can make things considerably simpler.

nathan


[-- Attachment #2: gcov.patch --]
[-- Type: text/x-patch, Size: 2435 bytes --]

2016-10-12  Nathan Sidwell  <nathan@acm.org>

	* gcov-io.c (gcov_open): Fix documentation.  Simplify setting
	gcov_var.mode.

Index: gcov-io.c
===================================================================
--- gcov-io.c	(revision 241027)
+++ gcov-io.c	(working copy)
@@ -115,10 +115,9 @@ static inline gcov_unsigned_t from_file
    opened. If MODE is >= 0 an existing file will be opened, if
    possible, and if MODE is <= 0, a new file will be created. Use
    MODE=0 to attempt to reopen an existing file and then fall back on
-   creating a new one.  If MODE < 0, the file will be opened in
+   creating a new one.  If MODE > 0, the file will be opened in
    read-only mode.  Otherwise it will be opened for modification.
-   Return zero on failure, >0 on opening an existing file and <0 on
-   creating a new one.  */
+   Return zero on failure, non-zero on success.  */
 
 GCOV_LINKAGE int
 #if IN_LIBGCOV
@@ -156,17 +155,12 @@ gcov_open (const char *name, int mode)
       /* pass mode (ignored) for compatibility */
       fd = open (name, O_RDONLY, S_IRUSR | S_IWUSR);
     }
-  else if (mode < 0)
+  else
      {
        /* Write mode - acquire a write-lock.  */
        s_flock.l_type = F_WRLCK;
-      fd = open (name, O_RDWR | O_CREAT | O_TRUNC, 0666);
-    }
-  else /* mode == 0 */
-    {
-      /* Read-Write mode - acquire a write-lock.  */
-      s_flock.l_type = F_WRLCK;
-      fd = open (name, O_RDWR | O_CREAT, 0666);
+       /* Truncate if force new mode.  */
+       fd = open (name, O_RDWR | O_CREAT | (mode < 0 ? O_TRUNC : 0), 0666);
     }
   if (fd < 0)
     return 0;
@@ -182,9 +176,7 @@ gcov_open (const char *name, int mode)
       return 0;
     }
 
-  if (mode > 0)
-    gcov_var.mode = 1;
-  else if (mode == 0)
+  if (mode == 0)
     {
       struct stat st;
 
@@ -194,21 +186,20 @@ gcov_open (const char *name, int mode)
 	  gcov_var.file = 0;
 	  return 0;
 	}
-      if (st.st_size != 0)
-	gcov_var.mode = 1;
-      else
-	gcov_var.mode = mode * 2 + 1;
+      gcov_var.mode = 1;
     }
   else
-    gcov_var.mode = mode * 2 + 1;
+    gcov_var.mode = mode;
 #else
   if (mode >= 0)
+    /* Open an existing file.  */
     gcov_var.file = fopen (name, (mode > 0) ? "rb" : "r+b");
 
   if (gcov_var.file)
     gcov_var.mode = 1;
   else if (mode <= 0)
     {
+      /* Create a new file.  */
       gcov_var.file = fopen (name, "w+b");
       if (gcov_var.file)
 	gcov_var.mode = mode * 2 + 1;

Re: Questionable code in gcov-io.c

[Marek Polacek]

On Wed, Oct 12, 2016 at 10:47:07AM -0400, Nathan Sidwell wrote:
> On 10/12/16 09:43, Marek Polacek wrote:
> 
> > > ITYM lines 197 -> 203.  I.e. remove the entire if that;s inside the 'mode ==
> > > 0' branch and make line 203 unconditional.
> > 
> > Yes, sorry for sloppy wording.  I'm testing a patch.
> 
> Here's the patch I've tested (but not bootstrapped).  As the incoming mode
> is -1, 0 or +1 we can make things considerably simpler.

Great, thanks.

	Marek

Re: Questionable code in gcov-io.c

[Andreas Schwab]

On Okt 12 2016, Nathan Sidwell <nathan@acm.org> wrote:

> @@ -182,9 +176,7 @@ gcov_open (const char *name, int mode)
>        return 0;
>      }
>  
> -  if (mode > 0)
> -    gcov_var.mode = 1;
> -  else if (mode == 0)
> +  if (mode == 0)
>      {
>        struct stat st;
>  
> @@ -194,21 +186,20 @@ gcov_open (const char *name, int mode)
>  	  gcov_var.file = 0;
>  	  return 0;
>  	}
> -      if (st.st_size != 0)
> -	gcov_var.mode = 1;
> -      else
> -	gcov_var.mode = mode * 2 + 1;
> +      gcov_var.mode = 1;

Do we still need to call fstat?  I don't think it can ever fail here.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

On 10/12/16 11:04, Andreas Schwab wrote:

> Do we still need to call fstat?  I don't think it can ever fail here.

You appear to be correct.

nathan

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

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

On 10/12/16 11:04, Andreas Schwab wrote:

> Do we still need to call fstat?  I don't think it can ever fail here.

Update removing the fstat.  Survived a profiled bootstrap, so I'll commit 
tomorrow, unless there are further comments.  Thanks for spotting this!

nathan



[-- Attachment #2: gcov.patch --]
[-- Type: text/x-patch, Size: 2618 bytes --]

2016-10-12  Nathan Sidwell  <nathan@acm.org>

	* gcov-io.c (gcov_open): Fix documentation.  Simplify setting
	gcov_var.mode.  Remove unnecessary fstat.

Index: gcov-io.c
===================================================================
--- gcov-io.c	(revision 241027)
+++ gcov-io.c	(working copy)
@@ -115,10 +115,9 @@ static inline gcov_unsigned_t from_file
    opened. If MODE is >= 0 an existing file will be opened, if
    possible, and if MODE is <= 0, a new file will be created. Use
    MODE=0 to attempt to reopen an existing file and then fall back on
-   creating a new one.  If MODE < 0, the file will be opened in
+   creating a new one.  If MODE > 0, the file will be opened in
    read-only mode.  Otherwise it will be opened for modification.
-   Return zero on failure, >0 on opening an existing file and <0 on
-   creating a new one.  */
+   Return zero on failure, non-zero on success.  */
 
 GCOV_LINKAGE int
 #if IN_LIBGCOV
@@ -156,17 +155,12 @@ gcov_open (const char *name, int mode)
       /* pass mode (ignored) for compatibility */
       fd = open (name, O_RDONLY, S_IRUSR | S_IWUSR);
     }
-  else if (mode < 0)
+  else
      {
        /* Write mode - acquire a write-lock.  */
        s_flock.l_type = F_WRLCK;
-      fd = open (name, O_RDWR | O_CREAT | O_TRUNC, 0666);
-    }
-  else /* mode == 0 */
-    {
-      /* Read-Write mode - acquire a write-lock.  */
-      s_flock.l_type = F_WRLCK;
-      fd = open (name, O_RDWR | O_CREAT, 0666);
+       /* Truncate if force new mode.  */
+       fd = open (name, O_RDWR | O_CREAT | (mode < 0 ? O_TRUNC : 0), 0666);
     }
   if (fd < 0)
     return 0;
@@ -181,42 +175,23 @@ gcov_open (const char *name, int mode)
       close (fd);
       return 0;
     }
-
-  if (mode > 0)
-    gcov_var.mode = 1;
-  else if (mode == 0)
-    {
-      struct stat st;
-
-      if (fstat (fd, &st) < 0)
-	{
-	  fclose (gcov_var.file);
-	  gcov_var.file = 0;
-	  return 0;
-	}
-      if (st.st_size != 0)
-	gcov_var.mode = 1;
-      else
-	gcov_var.mode = mode * 2 + 1;
-    }
-  else
-    gcov_var.mode = mode * 2 + 1;
 #else
   if (mode >= 0)
+    /* Open an existing file.  */
     gcov_var.file = fopen (name, (mode > 0) ? "rb" : "r+b");
 
   if (gcov_var.file)
-    gcov_var.mode = 1;
+    mode = 1;
   else if (mode <= 0)
-    {
-      gcov_var.file = fopen (name, "w+b");
-      if (gcov_var.file)
-	gcov_var.mode = mode * 2 + 1;
-    }
+    /* Create a new file.  */
+    gcov_var.file = fopen (name, "w+b");
+
   if (!gcov_var.file)
     return 0;
 #endif
 
+  gcov_var.mode = mode ? mode : 1;
+
   setbuf (gcov_var.file, (char *)0);
 
   return 1;

Re: Questionable code in gcov-io.c

[Andrew Pinski]

On Wed, Oct 12, 2016 at 11:24 AM, Nathan Sidwell <nathan@acm.org> wrote:
> On 10/12/16 11:04, Andreas Schwab wrote:
>
>> Do we still need to call fstat?  I don't think it can ever fail here.
>
>
> Update removing the fstat.  Survived a profiled bootstrap, so I'll commit
> tomorrow, unless there are further comments.  Thanks for spotting this!


This breaks the build for aarch64-elf:

In file included from
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/libgcov-driver.c:53:0:
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/../gcc/gcov-io.c:
In function ‘__gcov_open’:
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/../gcc/gcov-io.c:184:10:
error: assignment of read-only variable ‘mode’
     mode = 1;
          ^

Thanks,
Andrew


>
> nathan
>
>

Re: Questionable code in gcov-io.c

[Nathan Sidwell]

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

On 10/13/16 18:10, Andrew Pinski wrote:

> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/libgcov-driver.c:53:0:
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/../gcc/gcov-io.c:
> In function ‘__gcov_open’:
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/libgcc/../gcc/gcov-io.c:184:10:
> error: assignment of read-only variable ‘mode’
>      mode = 1;

Darn, even when I'm paranoid I still mess up :(

fixed.

nathan

[-- Attachment #2: gcov-fix.patch --]
[-- Type: text/x-patch, Size: 414 bytes --]

2016-10-14  Nathan Sidwell  <nathan@acm.org>

	* gcov-io.c (gcov_open): Deconstify 'mode'.

Index: gcov-io.c
===================================================================
--- gcov-io.c	(revision 241153)
+++ gcov-io.c	(working copy)
@@ -127,7 +127,7 @@ gcov_open (const char *name, int mode)
 #endif
 {
 #if IN_LIBGCOV
-  const int mode = 0;
+  int mode = 0;
 #endif
 #if GCOV_LOCKED
   struct flock s_flock;