* [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc @ 2014-10-11 19:57 Ramsay Jones 2014-10-15 14:23 ` Christopher Li 0 siblings, 1 reply; 9+ messages in thread From: Ramsay Jones @ 2014-10-11 19:57 UTC (permalink / raw) To: Christopher Li; +Cc: Sparse Mailing-list Passing the '-Wsparse-error' to cgcc can cause that option to be passed to the C compiler (usually gcc), if the given source file does not provoke any sparse warnings, which in turn results in a failure to compile that file. In order to avoid passing this sparse option to the compiler, we add the '-Wsparse-error' option to the regular expression check in the 'check_only_option' function. In addition, we replace the plain call to 'die' when sparse exits with non-zero status (maybe due to -Wsparse-error), with a simple 'exit 1'. This suppresses an 'Died at ./cgcc line 86.' message on exit from cgcc. Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk> --- Hi Chris, The following shows the error/fix: $ pwd /home/ramsay/sparse $ cat -n hello.c 1 #include <stdio.h> 2 3 int main(void) 4 { 5 printf("Hello, world!\n"); 6 return 0; 7 } $ CHECK=./sparse ./cgcc hello.c $ echo $? 0 $ CHECK=./sparse ./cgcc -Wsparse-error hello.c cc: error: unrecognized command line option ‘-Wsparse-error’ $ echo $? 1 $ Note gcc complaining about the unknown option. Now edit hello.c so that we generate a sparse warning ... $ cat -n hello.c 1 #include <stdio.h> 2 3 int f(void) 4 { 5 return 42; 6 } 7 8 int main(void) 9 { 10 printf("Hello, world!\n"); 11 return 0; 12 } $ CHECK=./sparse ./cgcc hello.c hello.c:3:5: warning: symbol 'f' was not declared. Should it be static? $ echo $? 0 $ CHECK=./sparse ./cgcc -Wsparse-error hello.c hello.c:3:5: error: symbol 'f' was not declared. Should it be static? Died at ./cgcc line 86. $ echo $? 1 $ After this patch: with the original hello.c ... $ CHECK=./sparse ./cgcc hello.c $ echo $? 0 $ CHECK=./sparse ./cgcc -Wsparse-error hello.c $ echo $? 0 $ with the modified hello.c ... $ CHECK=./sparse ./cgcc hello.c hello.c:3:5: warning: symbol 'f' was not declared. Should it be static? $ echo $? 0 $ CHECK=./sparse ./cgcc -Wsparse-error hello.c hello.c:3:5: error: symbol 'f' was not declared. Should it be static? $ echo $? 1 $ ATB, Ramsay Jones cgcc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cgcc b/cgcc index 8ee8da1..8e38174 100755 --- a/cgcc +++ b/cgcc @@ -83,7 +83,7 @@ if ($do_check) { print "$check\n" if $verbose; if ($do_compile) { - system ($check) == 0 or die; + system ($check) == 0 or exit 1; } else { exec ($check); } @@ -101,7 +101,7 @@ exit 0; sub check_only_option { my ($arg) = @_; - return 1 if $arg =~ /^-W(no-?)?(default-bitfield-sign|one-bit-signed-bitfield|cast-truncate|bitwise|typesign|context|undef|ptr-subtraction-blows|cast-to-as|decl|transparent-union|address-space|enum-mismatch|do-while|old-initializer|non-pointer-null|paren-string|return-void|designated-init|sparse-all)$/; + return 1 if $arg =~ /^-W(no-?)?(default-bitfield-sign|one-bit-signed-bitfield|cast-truncate|bitwise|typesign|context|undef|ptr-subtraction-blows|cast-to-as|decl|transparent-union|address-space|enum-mismatch|do-while|old-initializer|non-pointer-null|paren-string|return-void|designated-init|sparse-all|sparse-error)$/; return 1 if $arg =~ /^-v(no-?)?(entry|dead)$/; return 0; } -- 2.1.0 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-11 19:57 [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc Ramsay Jones @ 2014-10-15 14:23 ` Christopher Li 2014-10-15 17:33 ` Ramsay Jones 0 siblings, 1 reply; 9+ messages in thread From: Christopher Li @ 2014-10-15 14:23 UTC (permalink / raw) To: Ramsay Jones; +Cc: Sparse Mailing-list On Sun, Oct 12, 2014 at 3:57 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > > > - system ($check) == 0 or die; > + system ($check) == 0 or exit 1; Why not exit with the error code from sparse here? Sparse might exit with other error code than "1" in the future. Chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-15 14:23 ` Christopher Li @ 2014-10-15 17:33 ` Ramsay Jones 2014-10-16 1:45 ` Christopher Li 0 siblings, 1 reply; 9+ messages in thread From: Ramsay Jones @ 2014-10-15 17:33 UTC (permalink / raw) To: Christopher Li; +Cc: Sparse Mailing-list On 15/10/14 15:23, Christopher Li wrote: > On Sun, Oct 12, 2014 at 3:57 AM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> >> >> - system ($check) == 0 or die; >> + system ($check) == 0 or exit 1; > > Why not exit with the error code from sparse here? > Sparse might exit with other error code than "1" in the future. The meaning of the return value from system() is not portable. If you read the perl documentation (e.g. perldoc -f system and perldoc perlport [look for system() specific comments near the end]), you will see that there is some variability. (This is particularly true if it executes a shell to run the command). However, I have found over the years that the situation is much worse than even the perl documentation claims! (e.g. On 'platforms' like cygwin/MinGW/Win32-gnu/Win32-ActiveState and even some unix systems). About the only thing they all agree on is that a 0 return means success. So, that is all I rely on, in general. This is helped by the fact that most programs return either 0 or 1. For those programs that do return multiple failure codes, I often don't care why it failed - just that it did. In the very few cases that I do care, then I do a 'best effort' knowing that it may return the wrong code on some platforms. Hmm, off the top of my head, something like: sub exit_code { my ($code) = @_; if ($code == -1) { # failed to execute $code = 1; } elsif ($code & 127) { # died with a signal (maybe) $code = 1; } elsif ($code != 0) { # non-zero exit code (maybe) my $t = ($code >> 8) & 0xff; $code = 1; if ($t > 0) { $code = $t; } } return $code; } system ($check) or exit exit_code($?); However, I think the above is way overkill in this case. ;-) ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-15 17:33 ` Ramsay Jones @ 2014-10-16 1:45 ` Christopher Li 2014-10-19 14:21 ` Ramsay Jones 0 siblings, 1 reply; 9+ messages in thread From: Christopher Li @ 2014-10-16 1:45 UTC (permalink / raw) To: Ramsay Jones; +Cc: Sparse Mailing-list Hi, I create a branch in the chrisl repo call "review-ramsay" for your new follow up patches. I plan to do incremental fix up if any on that branch. Then when we both happy about it, I will do rebase and smash the commit before push to master branch. On Thu, Oct 16, 2014 at 1:33 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > On 15/10/14 15:23, Christopher Li wrote: >> On Sun, Oct 12, 2014 at 3:57 AM, Ramsay Jones >> <ramsay@ramsay1.demon.co.uk> wrote: >>> >>> >>> - system ($check) == 0 or die; >>> + system ($check) == 0 or exit 1; >> >> Why not exit with the error code from sparse here? >> Sparse might exit with other error code than "1" in the future. > > The meaning of the return value from system() is not portable. I see. Does it mean we should use some thing other than "system". or maybe some thing like: $retcode = system($check); if $retcode !=0 { exit $retcode; } I don't use Perl so that might not be valid Perl. > Hmm, off the top of my head, something like: > > sub exit_code { > my ($code) = @_; > if ($code == -1) { # failed to execute > $code = 1; > } > elsif ($code & 127) { # died with a signal (maybe) > $code = 1; > } > elsif ($code != 0) { # non-zero exit code (maybe) > my $t = ($code >> 8) & 0xff; > $code = 1; > if ($t > 0) { > $code = $t; > } > } > return $code; > } > > system ($check) or exit exit_code($?); > > However, I think the above is way overkill in this case. ;-) Yes, I agree, that is way overkill. Chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-16 1:45 ` Christopher Li @ 2014-10-19 14:21 ` Ramsay Jones 2014-10-22 0:23 ` Christopher Li 0 siblings, 1 reply; 9+ messages in thread From: Ramsay Jones @ 2014-10-19 14:21 UTC (permalink / raw) To: Christopher Li; +Cc: Sparse Mailing-list, Thomas Graf Sorry for the late reply, it's been a busy few days. ;-) On 16/10/14 02:45, Christopher Li wrote: > Hi, > > I create a branch in the chrisl repo call "review-ramsay" for your new > follow up patches. > I plan to do incremental fix up if any on that branch. Then when we > both happy about it, > I will do rebase and smash the commit before push to master branch. Yes, I noticed the new branch - thanks! Did you have any thoughts regarding Josh Triplett's comments on the "compile-i386.c: don't ignore return value of write(2)" patch? > > > > On Thu, Oct 16, 2014 at 1:33 AM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> On 15/10/14 15:23, Christopher Li wrote: >>> On Sun, Oct 12, 2014 at 3:57 AM, Ramsay Jones >>> <ramsay@ramsay1.demon.co.uk> wrote: >>>> >>>> >>>> - system ($check) == 0 or die; >>>> + system ($check) == 0 or exit 1; >>> >>> Why not exit with the error code from sparse here? >>> Sparse might exit with other error code than "1" in the future. >> >> The meaning of the return value from system() is not portable. > > I see. Does it mean we should use some thing other than "system". > or maybe some thing like: > > $retcode = system($check); > if $retcode !=0 { exit $retcode; } No, no, you don't want to do that even on Linux (let alone more exotic platforms like Windows, MinGW and cygwin). $ vim cgcc $ git diff diff --git a/cgcc b/cgcc index 8e38174..364c77c 100755 --- a/cgcc +++ b/cgcc @@ -83,7 +83,9 @@ if ($do_check) { print "$check\n" if $verbose; if ($do_compile) { - system ($check) == 0 or exit 1; + my $code = system ($check); + print "code is: $code\n" ; + exit $code if $code != 0; } else { exec ($check); } $ $ CHECK=./sparse ./cgcc hello.c hello.c:3:5: warning: symbol 'f' was not declared. Should it be static? code is: 0 $ echo $? 0 $ CHECK=./sparse ./cgcc -Wsparse-error hello.c hello.c:3:5: error: symbol 'f' was not declared. Should it be static? code is: 256 $ echo $? 0 $ Note that the 'return code' from ./sparse (1) is encoded in the 2nd byte of $code. i.e. you have to 'interpret' the return from system in a similar manner to a C program inspecting the status returned by the wait() system call. (see the exit_code sub from the last email, which included a right shift by 8 bits). Hmm, well, maybe! It is often difficult, on many platforms, to determine if system() will run the program directly using execvp or indirectly using a system shell (and which one). There are various rules about this issue which depends on the *form* of the call to system (is the argument a simple scalar or an array) and if it is a scalar variable does it contain any 'shell metacharacters'. (And even if it doesn't, it still may use a shell anyway!) $ CHECK=./junk ./cgcc -Wsparse-error hello.c sh: 1: ./junk: not found code is: 32512 $ echo $? 0 $ Note that 32512 == 0x7f00 which means the return code from the shell was 127 which, according to the bash manpage means the command execution failed due to 'command not found'. (This was actually an execution of the dash shell, but I couldn't find the information in the dash manpage, and it seems to follow bash, which in turn is following the original bourne shell). ['man bash', then /EXIT<ret>]. It simply isn't worth the trouble ... :-D [This all presupposes that we are happy to accept the change in behaviour introduced by commit 4d881187 in the first place; i.e. not calling gcc if 'sparse' returns a non-zero status. ;-) ] ATB, Ramsay Jones ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-19 14:21 ` Ramsay Jones @ 2014-10-22 0:23 ` Christopher Li 2014-10-23 0:38 ` Ramsay Jones 0 siblings, 1 reply; 9+ messages in thread From: Christopher Li @ 2014-10-22 0:23 UTC (permalink / raw) To: Ramsay Jones; +Cc: Sparse Mailing-list, Thomas Graf On Sun, Oct 19, 2014 at 10:21 PM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > Sorry for the late reply, it's been a busy few days. ;-) > > On 16/10/14 02:45, Christopher Li wrote: >> Hi, >> >> I create a branch in the chrisl repo call "review-ramsay" for your new >> follow up patches. >> I plan to do incremental fix up if any on that branch. Then when we >> both happy about it, >> I will do rebase and smash the commit before push to master branch. > > Yes, I noticed the new branch - thanks! > > Did you have any thoughts regarding Josh Triplett's comments on the > "compile-i386.c: don't ignore return value of write(2)" patch? I am fine with either way. The current fix is fine. If you are up for a new xwrite function, that is of course fine as well. Using printf() is not much an improvement because printf() can still return bytes less than the amount you requested. > > No, no, you don't want to do that even on Linux (let alone more > exotic platforms like Windows, MinGW and cygwin). In that case, I will leave the patch as it is. Thanks for the explain. Chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-22 0:23 ` Christopher Li @ 2014-10-23 0:38 ` Ramsay Jones 2014-10-25 4:14 ` Christopher Li 0 siblings, 1 reply; 9+ messages in thread From: Ramsay Jones @ 2014-10-23 0:38 UTC (permalink / raw) To: Christopher Li; +Cc: Sparse Mailing-list, Thomas Graf On 22/10/14 01:23, Christopher Li wrote: > On Sun, Oct 19, 2014 at 10:21 PM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> Sorry for the late reply, it's been a busy few days. ;-) >> >> On 16/10/14 02:45, Christopher Li wrote: >>> Hi, >>> >>> I create a branch in the chrisl repo call "review-ramsay" for your new >>> follow up patches. >>> I plan to do incremental fix up if any on that branch. Then when we >>> both happy about it, >>> I will do rebase and smash the commit before push to master branch. >> >> Yes, I noticed the new branch - thanks! >> >> Did you have any thoughts regarding Josh Triplett's comments on the >> "compile-i386.c: don't ignore return value of write(2)" patch? > > I am fine with either way. The current fix is fine. I'm fine with this too. ;-) > If you are up for > a new xwrite function, that is of course fine as well. Without putting too much thought into it, such a beast would probably look something like this: #include <stddef.h> #include <unistd.h> #include <errno.h> ssize_t xwrite(int fd, const void *buf, size_t len) { const char *p = buf; ssize_t count = 0; while (len > 0) { ssize_t nw = write(fd, p, len); if (nw < 0) { if (errno == EAGAIN || errno == EINTR) continue; /* recoverable error */ return -1; } len -= nw; p += nw; count += nw; } return count; } Actually, I would need to think about what to do if write() returns zero. Such a return is not an error, it simply didn't write anything. Are we guaranteed that some progress will eventually be made, or should there be another check for non-progress in the loop. dunno. :( > Using printf() > is not much an improvement because printf() can still return > bytes less than the amount you requested. Hmm, I don't understand this. _In general_, you don't know how many bytes you are going to write before you call printf() and you don't explicitly request any amount. Assuming no errors, which are indicated with a negative return value, printf() returns the number of bytes actually 'written'. The standard I/O routines, including all the internal buffering, are much easier to use than the raw system calls (you don't have to cater for short writes etc., like the code above, because the standard library takes care of all of that for you). My preference for the first patch (replacing calls to write with printf), has little to do with this issue. I simply see no advantage in mixing calls to the standard I/O library with write() system calls. (The call to fflush(stdout) on line 889 has nothing to do with paranoia; it has to do with correctness and is very much needed!) Using printf() is more portable than write(). The emit_insn_atom() could be greatly simplified by calling printf() directly rather than using a sprintf/write combo. (not implemented in my patch). I would be very surprised if using write() was any more efficient than printf(). I find the code easier to comprehend without mixing I/O styles. So, once again, what advantage does write() bestow? Having said that, although I think the latest patch is perfectly fine, if you would like me to try expanding on the above xwrite() idea, just let me know. ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-23 0:38 ` Ramsay Jones @ 2014-10-25 4:14 ` Christopher Li 2014-10-25 12:32 ` Ramsay Jones 0 siblings, 1 reply; 9+ messages in thread From: Christopher Li @ 2014-10-25 4:14 UTC (permalink / raw) To: Ramsay Jones; +Cc: Sparse Mailing-list, Thomas Graf On Thu, Oct 23, 2014 at 8:38 AM, Ramsay Jones <ramsay@ramsay1.demon.co.uk> wrote: > > Without putting too much thought into it, such a beast would probably > look something like this: > > #include <stddef.h> > #include <unistd.h> > #include <errno.h> > > ssize_t xwrite(int fd, const void *buf, size_t len) > { > const char *p = buf; > ssize_t count = 0; > > while (len > 0) { > ssize_t nw = write(fd, p, len); > if (nw < 0) { > if (errno == EAGAIN || errno == EINTR) > continue; /* recoverable error */ > return -1; > } > len -= nw; > p += nw; > count += nw; > } > return count; > } > > Actually, I would need to think about what to do if write() returns > zero. Such a return is not an error, it simply didn't write anything. > Are we guaranteed that some progress will eventually be made, or > should there be another check for non-progress in the loop. dunno. :( The while loop need to have some ending conditions otherwise it can result in no progress make on a loop. e.g. calling on a non blocking file descriptor and the write will block. I think for the compile-i386.c. It is fine just die on short write. It is just an example program. > Hmm, I don't understand this. _In general_, you don't know how many > bytes you are going to write before you call printf() and you don't > explicitly request any amount. Assuming no errors, which are indicated > with a negative return value, printf() returns the number of bytes > actually 'written'. The standard I/O routines, including all the internal > buffering, are much easier to use than the raw system calls (you don't > have to cater for short writes etc., like the code above, because the > standard library takes care of all of that for you). My feed back is base on the reason of the change. If your intend is to get rid of the warning of not checking the error condition. Then the right thing to do is to just check and handle it. Die on error is also one kind of handle. Might be a bit harsh, but fine for the example program. In this respect, printf is not better. I am actually not sure how printf take care care of the short write. "printf" is very complex. I image if there is real error on the file system level. e.g. disk full. "printf" will have to return shorter than you expected. So printf will not get rid of the short write completely. > > My preference for the first patch (replacing calls to write with printf), > has little to do with this issue. I simply see no advantage in mixing > calls to the standard I/O library with write() system calls. (The call > to fflush(stdout) on line 889 has nothing to do with paranoia; it has to > do with correctness and is very much needed!) Then your intend is different than what I previously understand. If you call that change to unify the writing style of printf vs write. Converting write to use printf is acceptable. BTW, if there is no formatting string, why not use some thing like "puts()" instead? However, using printf without checking is not a right fix for the reason to address error condition not check. It still have unchecked error conditions. > Using printf() is more portable than write(). The emit_insn_atom() could > be greatly simplified by calling printf() directly rather than using > a sprintf/write combo. (not implemented in my patch). I would be very > surprised if using write() was any more efficient than printf(). I find Actually write is more efficient than printf. "write" is just a wrapper to the system call "write". "printf" is another strong. It is very complex. But that is not relevant here. I vote for left the patch as it is. Thanks Chris ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc 2014-10-25 4:14 ` Christopher Li @ 2014-10-25 12:32 ` Ramsay Jones 0 siblings, 0 replies; 9+ messages in thread From: Ramsay Jones @ 2014-10-25 12:32 UTC (permalink / raw) To: Christopher Li; +Cc: Sparse Mailing-list, Thomas Graf On 25/10/14 05:14, Christopher Li wrote: > On Thu, Oct 23, 2014 at 8:38 AM, Ramsay Jones > <ramsay@ramsay1.demon.co.uk> wrote: >> >> Without putting too much thought into it, such a beast would probably >> look something like this: >> >> #include <stddef.h> >> #include <unistd.h> >> #include <errno.h> >> >> ssize_t xwrite(int fd, const void *buf, size_t len) >> { >> const char *p = buf; >> ssize_t count = 0; >> >> while (len > 0) { >> ssize_t nw = write(fd, p, len); >> if (nw < 0) { >> if (errno == EAGAIN || errno == EINTR) >> continue; /* recoverable error */ >> return -1; >> } >> len -= nw; >> p += nw; >> count += nw; >> } >> return count; >> } >> >> Actually, I would need to think about what to do if write() returns >> zero. Such a return is not an error, it simply didn't write anything. >> Are we guaranteed that some progress will eventually be made, or >> should there be another check for non-progress in the loop. dunno. :( > > The while loop need to have some ending conditions otherwise > it can result in no progress make on a loop. e.g. calling on a non > blocking file descriptor and the write will block. Yeah, I just typed that directly into my mail client and the first version had an 'if (nw == 0) return -1;' in the loop. However, I couldn't justify doing that, because it is not an error. I briefly thought about adding a 'non-progress' count to use to exit, but at this point I felt I would have to go away and start reading ... so I just punted. It was only a quick 'something like this ...' anyway! ;-) > I think for the compile-i386.c. It is fine just die on short write. > It is just an example program. Again, I'm happy to go with this too! >> Hmm, I don't understand this. _In general_, you don't know how many >> bytes you are going to write before you call printf() and you don't >> explicitly request any amount. Assuming no errors, which are indicated >> with a negative return value, printf() returns the number of bytes >> actually 'written'. The standard I/O routines, including all the internal >> buffering, are much easier to use than the raw system calls (you don't >> have to cater for short writes etc., like the code above, because the >> standard library takes care of all of that for you). > > My feed back is base on the reason of the change. If your intend is > to get rid of the warning of not checking the error condition. Then the > right thing to do is to just check and handle it. Die on error is also one > kind of handle. Might be a bit harsh, but fine for the example program. > In this respect, printf is not better. OK, point taken. [snip] > I vote for left the patch as it is. Me too. Thanks! ATB, Ramsay Jones ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-10-25 20:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-10-11 19:57 [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc Ramsay Jones 2014-10-15 14:23 ` Christopher Li 2014-10-15 17:33 ` Ramsay Jones 2014-10-16 1:45 ` Christopher Li 2014-10-19 14:21 ` Ramsay Jones 2014-10-22 0:23 ` Christopher Li 2014-10-23 0:38 ` Ramsay Jones 2014-10-25 4:14 ` Christopher Li 2014-10-25 12:32 ` Ramsay Jones
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 NNTP newsgroup(s).