* Questionable code in gcov-io.c @ 2016-10-12 12:10 Marek Polacek 2016-10-12 12:15 ` Nathan Sidwell 2016-10-12 12:23 ` Bernd Schmidt 0 siblings, 2 replies; 13+ messages in thread From: Marek Polacek @ 2016-10-12 12:10 UTC (permalink / raw) To: GCC Patches; +Cc: Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 12:10 Questionable code in gcov-io.c Marek Polacek @ 2016-10-12 12:15 ` Nathan Sidwell 2016-10-12 13:43 ` Marek Polacek 2016-10-12 12:23 ` Bernd Schmidt 1 sibling, 1 reply; 13+ messages in thread From: Nathan Sidwell @ 2016-10-12 12:15 UTC (permalink / raw) To: Marek Polacek, GCC Patches; +Cc: Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 12:15 ` Nathan Sidwell @ 2016-10-12 13:43 ` Marek Polacek 2016-10-12 13:45 ` Nathan Sidwell 2016-10-12 14:47 ` Nathan Sidwell 0 siblings, 2 replies; 13+ messages in thread From: Marek Polacek @ 2016-10-12 13:43 UTC (permalink / raw) To: Nathan Sidwell; +Cc: GCC Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 13:43 ` Marek Polacek @ 2016-10-12 13:45 ` Nathan Sidwell 2016-10-12 14:47 ` Nathan Sidwell 1 sibling, 0 replies; 13+ messages in thread From: Nathan Sidwell @ 2016-10-12 13:45 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 13:43 ` Marek Polacek 2016-10-12 13:45 ` Nathan Sidwell @ 2016-10-12 14:47 ` Nathan Sidwell 2016-10-12 14:50 ` Marek Polacek 2016-10-12 15:05 ` Andreas Schwab 1 sibling, 2 replies; 13+ messages in thread From: Nathan Sidwell @ 2016-10-12 14:47 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches, Jakub Jelinek [-- 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 14:47 ` Nathan Sidwell @ 2016-10-12 14:50 ` Marek Polacek 2016-10-12 15:05 ` Andreas Schwab 1 sibling, 0 replies; 13+ messages in thread From: Marek Polacek @ 2016-10-12 14:50 UTC (permalink / raw) To: Nathan Sidwell; +Cc: GCC Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 14:47 ` Nathan Sidwell 2016-10-12 14:50 ` Marek Polacek @ 2016-10-12 15:05 ` Andreas Schwab 2016-10-12 15:12 ` Nathan Sidwell 2016-10-12 18:25 ` Nathan Sidwell 1 sibling, 2 replies; 13+ messages in thread From: Andreas Schwab @ 2016-10-12 15:05 UTC (permalink / raw) To: Nathan Sidwell; +Cc: Marek Polacek, GCC Patches, Jakub Jelinek 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 15:05 ` Andreas Schwab @ 2016-10-12 15:12 ` Nathan Sidwell 2016-10-12 18:25 ` Nathan Sidwell 1 sibling, 0 replies; 13+ messages in thread From: Nathan Sidwell @ 2016-10-12 15:12 UTC (permalink / raw) To: Andreas Schwab; +Cc: Marek Polacek, GCC Patches, Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 15:05 ` Andreas Schwab 2016-10-12 15:12 ` Nathan Sidwell @ 2016-10-12 18:25 ` Nathan Sidwell 2016-10-13 22:10 ` Andrew Pinski 1 sibling, 1 reply; 13+ messages in thread From: Nathan Sidwell @ 2016-10-12 18:25 UTC (permalink / raw) To: Andreas Schwab; +Cc: Marek Polacek, GCC Patches, Jakub Jelinek [-- 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 18:25 ` Nathan Sidwell @ 2016-10-13 22:10 ` Andrew Pinski 2016-10-14 10:53 ` Nathan Sidwell 0 siblings, 1 reply; 13+ messages in thread From: Andrew Pinski @ 2016-10-13 22:10 UTC (permalink / raw) To: Nathan Sidwell; +Cc: Andreas Schwab, Marek Polacek, GCC Patches, Jakub Jelinek 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 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-13 22:10 ` Andrew Pinski @ 2016-10-14 10:53 ` Nathan Sidwell 0 siblings, 0 replies; 13+ messages in thread From: Nathan Sidwell @ 2016-10-14 10:53 UTC (permalink / raw) To: Andrew Pinski; +Cc: Andreas Schwab, Marek Polacek, GCC Patches, Jakub Jelinek [-- 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; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 12:10 Questionable code in gcov-io.c Marek Polacek 2016-10-12 12:15 ` Nathan Sidwell @ 2016-10-12 12:23 ` Bernd Schmidt 2016-10-12 12:54 ` Jakub Jelinek 1 sibling, 1 reply; 13+ messages in thread From: Bernd Schmidt @ 2016-10-12 12:23 UTC (permalink / raw) To: Marek Polacek, GCC Patches; +Cc: Jakub Jelinek 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Questionable code in gcov-io.c 2016-10-12 12:23 ` Bernd Schmidt @ 2016-10-12 12:54 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2016-10-12 12:54 UTC (permalink / raw) To: Bernd Schmidt; +Cc: Marek Polacek, GCC Patches 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-10-14 10:53 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-12 12:10 Questionable code in gcov-io.c Marek Polacek 2016-10-12 12:15 ` Nathan Sidwell 2016-10-12 13:43 ` Marek Polacek 2016-10-12 13:45 ` Nathan Sidwell 2016-10-12 14:47 ` Nathan Sidwell 2016-10-12 14:50 ` Marek Polacek 2016-10-12 15:05 ` Andreas Schwab 2016-10-12 15:12 ` Nathan Sidwell 2016-10-12 18:25 ` Nathan Sidwell 2016-10-13 22:10 ` Andrew Pinski 2016-10-14 10:53 ` Nathan Sidwell 2016-10-12 12:23 ` Bernd Schmidt 2016-10-12 12:54 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).