[PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Ramsay Jones]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Christopher Li]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Ramsay Jones]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Christopher Li]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Ramsay Jones]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Christopher Li]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Ramsay Jones]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Christopher Li]

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

Re: [PATCH 2/4] cgcc: avoid passing a sparse-only option to cc

[Ramsay Jones]

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