cgcc: avoid passing a sparse-only option to cc

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.

---

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

index 8ee8da1..8e38174 100755
@@ -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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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.

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.

Yes, I agree, that is way overkill.

Chris

Sorry for the late reply, it's been a busy few days. ;-)

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?

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

index 8e38174..364c77c 100755
@@ -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

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.

In that case, I will leave the patch as it is. Thanks for the explain.

Chris

I'm fine with this too. ;-)

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. :(

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

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.

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.

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.

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

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! ;-)

Again, I'm happy to go with this too!

OK, point taken.

[snip]

Me too.

Thanks!

ATB,
Ramsay Jones