Re: Questionable code in gcov-io.c

[Nathan Sidwell]

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
2016-10-14  Nathan Sidwell  

	* 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;

Re: Questionable code in gcov-io.c

[Andrew Pinski]

On Wed, Oct 12, 2016 at 11:24 AM, Nathan Sidwell  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]

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


2016-10-12  Nathan Sidwell  

	* 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, ) < 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

[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

[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  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, sch...@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 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

2016-10-12  Nathan Sidwell  

	* 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

[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

[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, ) < 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

[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=107747608611324 though.

Jakub

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, ) < 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

[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, ) < 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

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, ) < 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