| Message ID | 54398BA5.6050005@ramsay1.demon.co.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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; }
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(-)