[v1] slirp: Gcc 9 -O3 fix

[Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Dr. David Alan Gilbert (git) 6 years, 10 months ago

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Gcc 9 needs some convincing that sopreprbuf really is going to fill
in iov in the call from soreadbuf, even though the failure case
shouldn't happen; so swing the check around initialising the fields.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 slirp/src/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/slirp/src/socket.c b/slirp/src/socket.c
index 4a3c935e25..4a2222a95f 100644
--- a/slirp/src/socket.c
+++ b/slirp/src/socket.c
@@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np)
 	DEBUG_CALL("sopreprbuf");
 	DEBUG_ARG("so = %p", so);
 
-	if (len <= 0)
-		return 0;
-
 	iov[0].iov_base = sb->sb_wptr;
+        iov[0].iov_len = 0;
         iov[1].iov_base = NULL;
         iov[1].iov_len = 0;
+
+	if (len <= 0)
+		return 0;
+
 	if (sb->sb_wptr < sb->sb_rptr) {
 		iov[0].iov_len = sb->sb_rptr - sb->sb_wptr;
 		/* Should never succeed, but... */
-- 
2.21.0

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Samuel Thibault 6 years, 10 months ago

Hello,

Dr. David Alan Gilbert (git), le ven. 05 avril 2019 19:46:48 +0100, a ecrit:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Gcc 9 needs some convincing that sopreprbuf really is going to fill
> in iov in the call from soreadbuf, even though the failure case
> shouldn't happen; so swing the check around initialising the fields.

While I can understand that setting iov[0].iov_len may help a compiler,
I don't see why moving if (len <= 0) return 0; down?

> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  slirp/src/socket.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/slirp/src/socket.c b/slirp/src/socket.c
> index 4a3c935e25..4a2222a95f 100644
> --- a/slirp/src/socket.c
> +++ b/slirp/src/socket.c
> @@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np)
>  	DEBUG_CALL("sopreprbuf");
>  	DEBUG_ARG("so = %p", so);
>  
> -	if (len <= 0)
> -		return 0;
> -
>  	iov[0].iov_base = sb->sb_wptr;
> +        iov[0].iov_len = 0;
>          iov[1].iov_base = NULL;
>          iov[1].iov_len = 0;
> +
> +	if (len <= 0)
> +		return 0;
> +
>  	if (sb->sb_wptr < sb->sb_rptr) {
>  		iov[0].iov_len = sb->sb_rptr - sb->sb_wptr;
>  		/* Should never succeed, but... */
> -- 
> 2.21.0
> 

-- 
Samuel
 FYLG> Tiens, vlà une URL qui va bien :
 FYLG> ftp://127.0.0.1/WaReZ/NiouZeS/WinDoZe/NeWSMoNGeR/SuPeR
 c'est gentil sauf que l'adresse ne fonctionne pas sa me fais une erreur
 -+- Furtif in Guide du Neuneu Usenet : <MODE CERVEAU OFF> -+-

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Dr. David Alan Gilbert 6 years, 10 months ago

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Hello,
> 
> Dr. David Alan Gilbert (git), le ven. 05 avril 2019 19:46:48 +0100, a ecrit:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Gcc 9 needs some convincing that sopreprbuf really is going to fill
> > in iov in the call from soreadbuf, even though the failure case
> > shouldn't happen; so swing the check around initialising the fields.
> 
> While I can understand that setting iov[0].iov_len may help a compiler,
> I don't see why moving if (len <= 0) return 0; down?

The original errors are:
/home/dgilbert/git/qemu/slirp/src/socket.c: In function ‘soread’:
/home/dgilbert/git/qemu/slirp/src/socket.c:188:7: error: ‘iov.iov_base’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  188 |  nn = recv(so->s, iov[0].iov_base, iov[0].iov_len,0);
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/dgilbert/git/qemu/slirp/src/socket.c:188:7: error: ‘iov.iov_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
/home/dgilbert/git/qemu/slirp/src/socket.c:232:5: error: ‘n’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  232 |  if (n == 2 && nn == iov[0].iov_len) {
      |     ^
/home/dgilbert/git/qemu/slirp/src/socket.c:234:19: error: ‘*((void *)&iov+16).iov_len’ may be used uninitialized in this function [-Werror=maybe-uninitialized]

(and a few more along the same idea).
So the problem is actually in soread not sopreprbuf itself.

'soread' has the comment:

        /*
         * No need to check if there's enough room to read.
         * soread wouldn't have been called if there weren't
         */
        sopreprbuf(so, iov, &n);

the compiler doesn't realise that, and is moaning about the case
where the if (len <=0) return happens and the following 
code tries to use iov.

Dave

> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  slirp/src/socket.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/slirp/src/socket.c b/slirp/src/socket.c
> > index 4a3c935e25..4a2222a95f 100644
> > --- a/slirp/src/socket.c
> > +++ b/slirp/src/socket.c
> > @@ -113,12 +113,14 @@ size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np)
> >  	DEBUG_CALL("sopreprbuf");
> >  	DEBUG_ARG("so = %p", so);
> >  
> > -	if (len <= 0)
> > -		return 0;
> > -
> >  	iov[0].iov_base = sb->sb_wptr;
> > +        iov[0].iov_len = 0;
> >          iov[1].iov_base = NULL;
> >          iov[1].iov_len = 0;
> > +
> > +	if (len <= 0)
> > +		return 0;
> > +
> >  	if (sb->sb_wptr < sb->sb_rptr) {
> >  		iov[0].iov_len = sb->sb_rptr - sb->sb_wptr;
> >  		/* Should never succeed, but... */
> > -- 
> > 2.21.0
> > 
> 
> -- 
> Samuel
>  FYLG> Tiens, vlà une URL qui va bien :
>  FYLG> ftp://127.0.0.1/WaReZ/NiouZeS/WinDoZe/NeWSMoNGeR/SuPeR
>  c'est gentil sauf que l'adresse ne fonctionne pas sa me fais une erreur
>  -+- Furtif in Guide du Neuneu Usenet : <MODE CERVEAU OFF> -+-
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Samuel Thibault 6 years, 10 months ago

Hello,

Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit:
> 'soread' has the comment:
> 
>         /*
>          * No need to check if there's enough room to read.
>          * soread wouldn't have been called if there weren't
>          */
>         sopreprbuf(so, iov, &n);
> 
> the compiler doesn't realise that, and is moaning about the case
> where the if (len <=0) return happens and the following 
> code tries to use iov.

I see. Perhaps we should make this an assert then? In case this isn't
true, i.e. soread() is called even if no room is available, returning 0
would probably just let the caller just try again, and we should rather
just plainly crash than hang?

Samuel

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Dr. David Alan Gilbert 6 years, 10 months ago

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Hello,
> 
> Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit:
> > 'soread' has the comment:
> > 
> >         /*
> >          * No need to check if there's enough room to read.
> >          * soread wouldn't have been called if there weren't
> >          */
> >         sopreprbuf(so, iov, &n);
> > 
> > the compiler doesn't realise that, and is moaning about the case
> > where the if (len <=0) return happens and the following 
> > code tries to use iov.
> 
> I see. Perhaps we should make this an assert then? In case this isn't
> true, i.e. soread() is called even if no room is available, returning 0
> would probably just let the caller just try again, and we should rather
> just plainly crash than hang?

Adding the assert in soread sorts that case out:
  assert(sopreprbuf(so, iov, &n) != 0);

however, I also need to fix soreadbuf;  is it legal to call that with
a 0 size?

Dave

> Samuel
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Samuel Thibault 6 years, 10 months ago

Dr. David Alan Gilbert, le ven. 12 avril 2019 16:49:42 +0100, a ecrit:
> * Samuel Thibault (samuel.thibault@gnu.org) wrote:
> > Hello,
> > 
> > Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit:
> > > 'soread' has the comment:
> > > 
> > >         /*
> > >          * No need to check if there's enough room to read.
> > >          * soread wouldn't have been called if there weren't
> > >          */
> > >         sopreprbuf(so, iov, &n);
> > > 
> > > the compiler doesn't realise that, and is moaning about the case
> > > where the if (len <=0) return happens and the following 
> > > code tries to use iov.
> > 
> > I see. Perhaps we should make this an assert then? In case this isn't
> > true, i.e. soread() is called even if no room is available, returning 0
> > would probably just let the caller just try again, and we should rather
> > just plainly crash than hang?
> 
> Adding the assert in soread sorts that case out:
>   assert(sopreprbuf(so, iov, &n) != 0);
> 
> however, I also need to fix soreadbuf;  is it legal to call that with
> a 0 size?

It does not really make sense to, so an assert >0 should be fine.

Samuel

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by Dr. David Alan Gilbert 6 years, 9 months ago

* Samuel Thibault (samuel.thibault@gnu.org) wrote:
> Dr. David Alan Gilbert, le ven. 12 avril 2019 16:49:42 +0100, a ecrit:
> > * Samuel Thibault (samuel.thibault@gnu.org) wrote:
> > > Hello,
> > > 
> > > Dr. David Alan Gilbert, le lun. 08 avril 2019 09:46:53 +0100, a ecrit:
> > > > 'soread' has the comment:
> > > > 
> > > >         /*
> > > >          * No need to check if there's enough room to read.
> > > >          * soread wouldn't have been called if there weren't
> > > >          */
> > > >         sopreprbuf(so, iov, &n);
> > > > 
> > > > the compiler doesn't realise that, and is moaning about the case
> > > > where the if (len <=0) return happens and the following 
> > > > code tries to use iov.
> > > 
> > > I see. Perhaps we should make this an assert then? In case this isn't
> > > true, i.e. soread() is called even if no room is available, returning 0
> > > would probably just let the caller just try again, and we should rather
> > > just plainly crash than hang?
> > 
> > Adding the assert in soread sorts that case out:
> >   assert(sopreprbuf(so, iov, &n) != 0);
> > 
> > however, I also need to fix soreadbuf;  is it legal to call that with
> > a 0 size?
> 
> It does not really make sense to, so an assert >0 should be fine.

OK, replacement patch with just a couple of asserts sent.

Dave

> Samuel
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix

Posted by no-reply@patchew.org 6 years, 10 months ago

Patchew URL: https://patchew.org/QEMU/20190405184648.17029-1-dgilbert@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190405184648.17029-1-dgilbert@redhat.com
Subject: [Qemu-devel] [PATCH] slirp: Gcc 9 -O3 fix
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190405184648.17029-1-dgilbert@redhat.com -> patchew/20190405184648.17029-1-dgilbert@redhat.com
Switched to a new branch 'test'
c02eccdeb3 slirp: Gcc 9 -O3 fix

=== OUTPUT BEGIN ===
ERROR: code indent should never use tabs
#22: FILE: slirp/src/socket.c:116:
+^Iiov[0].iov_base = sb->sb_wptr;$

total: 1 errors, 0 warnings, 17 lines checked

Commit c02eccdeb3e6 (slirp: Gcc 9 -O3 fix) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190405184648.17029-1-dgilbert@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com