Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1358275 > unrolled thread

[PATCH] qla2xxx: avoid maybe_uninitialized warning

Back to article view | Back to linux.kernel

Contents

  [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann <arnd@arndb.de> - 2016-03-15 22:50 +0100
    Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning James Bottomley <jejb@linux.vnet.ibm.com> - 2016-03-15 22:50 +0100
      Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning Arnd Bergmann <arnd@arndb.de> - 2016-03-16 14:10 +0100
        Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning Himanshu Madhani <himanshu.madhani@qlogic.com> - 2016-03-16 15:10 +0100
          Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning James Bottomley <jejb@linux.vnet.ibm.com> - 2016-03-16 15:30 +0100
    Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning Tomas Henzl <thenzl@redhat.com> - 2016-03-16 16:10 +0100
      Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning Tomas Henzl <thenzl@redhat.com> - 2016-03-16 16:20 +0100
      Re: [PATCH] qla2xxx: avoid maybe_uninitialized warning "Ewan D. Milne" <emilne@redhat.com> - 2016-03-16 16:50 +0100

#1358275 — [PATCH] qla2xxx: avoid maybe_uninitialized warning

The qlt_check_reserve_free_req() function produces an incorrect warning
when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:

drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   ql_dbg(ql_dbg_io, vha, 0x305a,
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       vha->vp_idx, vha->req->ring_index,
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]

The problem is that gcc fails to track the state of the condition across
an annotated branch.

This slightly rearranges the code to move the second if() block
into the first one, to avoid the warning while retaining the
behavior of the code.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index 985231900aca..8a44d1541eb4 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
 		else
 			vha->req->cnt = vha->req->length -
 			    (vha->req->ring_index - cnt);
-	}
 
-	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
-		ql_dbg(ql_dbg_io, vha, 0x305a,
-		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
-		    vha->vp_idx, vha->req->ring_index,
-		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
-		return -EAGAIN;
+		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
+			ql_dbg(ql_dbg_io, vha, 0x305a,
+			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
+			    vha->vp_idx, vha->req->ring_index,
+			    vha->req->cnt, req_cnt, cnt, cnt_in,
+			    vha->req->length);
+			return -EAGAIN;
+		}
 	}
+
 	vha->req->cnt -= req_cnt;
 
 	return 0;
-- 
2.7.0

[toc] | [next] | [standalone]

#1358278

On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> The qlt_check_reserve_free_req() function produces an incorrect 
> warning when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> 
> drivers/scsi/qla2xxx/qla_target.c: In function
> 'qlt_check_reserve_free_req':
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>    ql_dbg(ql_dbg_io, vha, 0x305a,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        "qla_target(%d): There is no room in the request ring: vha
> ->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req
> -in=%d Req-Length=%d\n",
>       
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ~~~~~~~~~~
>        vha->vp_idx, vha->req->ring_index,
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
> 
> The problem is that gcc fails to track the state of the condition 
> across an annotated branch.
> 
> This slightly rearranges the code to move the second if() block
> into the first one, to avoid the warning while retaining the
> behavior of the code.

I thought our usual policy was to ask someone to fix the compiler when
it emitted a spurious warning.

James

[toc] | [prev] | [next] | [standalone]

#1358970

On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
> On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> > 
> > This slightly rearranges the code to move the second if() block
> > into the first one, to avoid the warning while retaining the
> > behavior of the code.
> 
> I thought our usual policy was to ask someone to fix the compiler when
> it emitted a spurious warning.

No, the rule is that we shouldn't blindly add initializations to
the variables when the compiler should have figured it out.

In this case, I wouldn't expect the compiler to ever see through
the unlikely() macro, and I'm not adding a potentially counterproductive
initialization, so I see no reason not to apply the patch.

Making it easier for the compiler to figure out what is going
on should also lead to slightly better object code. If you think
my patch makes it less readable, an alternative would be to remove
the 'unlikely', which also gets rid of the warning.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1359028

On 3/16/16, 5:59 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:

>On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
>> On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
>> > 
>> > This slightly rearranges the code to move the second if() block
>> > into the first one, to avoid the warning while retaining the
>> > behavior of the code.
>> 
>> I thought our usual policy was to ask someone to fix the compiler when
>> it emitted a spurious warning.
>
>No, the rule is that we shouldn't blindly add initializations to
>the variables when the compiler should have figured it out.
>
>In this case, I wouldn't expect the compiler to ever see through
>the unlikely() macro, and I'm not adding a potentially counterproductive
>initialization, so I see no reason not to apply the patch.

I would like to keep unlikely() macro in the code. This patch looks good.

Acked-By: Himanshu Madhani <himanshu.madhani@qlogic.com>

>
>Making it easier for the compiler to figure out what is going
>on should also lead to slightly better object code. If you think
>my patch makes it less readable, an alternative would be to remove
>the 'unlikely', which also gets rid of the warning.
>
>	Arnd

[toc] | [prev] | [next] | [standalone]

#1359040

On Wed, 2016-03-16 at 14:05 +0000, Himanshu Madhani wrote:
> 
> On 3/16/16, 5:59 AM, "Arnd Bergmann" <arnd@arndb.de> wrote:
> 
> > On Tuesday 15 March 2016 14:49:14 James Bottomley wrote:
> > > On Tue, 2016-03-15 at 22:40 +0100, Arnd Bergmann wrote:
> > > > 
> > > > This slightly rearranges the code to move the second if() block
> > > > into the first one, to avoid the warning while retaining the
> > > > behavior of the code.
> > > 
> > > I thought our usual policy was to ask someone to fix the compiler 
> > > when it emitted a spurious warning.
> > 
> > No, the rule is that we shouldn't blindly add initializations to
> > the variables when the compiler should have figured it out.
> > 
> > In this case, I wouldn't expect the compiler to ever see through
> > the unlikely() macro, and I'm not adding a potentially 
> > counterproductive initialization, so I see no reason not to apply
> > the patch.

OK, as long as there's a good reason why the compiler can never be
fixed to sort out this case.

> I would like to keep unlikely() macro in the code. This patch looks
> good.
> 
> Acked-By: Himanshu Madhani <himanshu.madhani@qlogic.com>

Well, OK that's good enough for me.

James

[toc] | [prev] | [next] | [standalone]

#1359066

On 15.3.2016 22:40, Arnd Bergmann wrote:
> The qlt_check_reserve_free_req() function produces an incorrect warning
> when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
>
> drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>    ql_dbg(ql_dbg_io, vha, 0x305a,
>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->vp_idx, vha->req->ring_index,
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> The problem is that gcc fails to track the state of the condition across
> an annotated branch.
>
> This slightly rearranges the code to move the second if() block
> into the first one, to avoid the warning while retaining the
> behavior of the code.

When the first 'if' is true the vha->req->ring_index gets a new value 
assigned - so it could be possible that the second 'if' wont be true any more.
The code should not be merged into that single 'if', or am I missing something?

tomash

>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 985231900aca..8a44d1541eb4 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
>  		else
>  			vha->req->cnt = vha->req->length -
>  			    (vha->req->ring_index - cnt);
> -	}
>  
> -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> -		ql_dbg(ql_dbg_io, vha, 0x305a,
> -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> -		    vha->vp_idx, vha->req->ring_index,
> -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> -		return -EAGAIN;
> +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> +			ql_dbg(ql_dbg_io, vha, 0x305a,
> +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> +			    vha->vp_idx, vha->req->ring_index,
> +			    vha->req->cnt, req_cnt, cnt, cnt_in,
> +			    vha->req->length);
> +			return -EAGAIN;
> +		}
>  	}
> +
>  	vha->req->cnt -= req_cnt;
>  
>  	return 0;

[toc] | [prev] | [next] | [standalone]

#1359084

On 16.3.2016 16:03, Tomas Henzl wrote:
> On 15.3.2016 22:40, Arnd Bergmann wrote:
>> The qlt_check_reserve_free_req() function produces an incorrect warning
>> when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
>>
>> drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
>> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>    ql_dbg(ql_dbg_io, vha, 0x305a,
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        vha->vp_idx, vha->req->ring_index,
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>>        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>>
>> The problem is that gcc fails to track the state of the condition across
>> an annotated branch.
>>
>> This slightly rearranges the code to move the second if() block
>> into the first one, to avoid the warning while retaining the
>> behavior of the code.
> When the first 'if' is true the vha->req->ring_index gets a new value 
> assigned - so it could be possible that the second 'if' wont be true any more.
> The code should not be merged into that single 'if', or am I missing something?

Oh, I haven't noticed that the second if actually remained in place
- please ignore my previous comments.


>
> tomash
>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
>> index 985231900aca..8a44d1541eb4 100644
>> --- a/drivers/scsi/qla2xxx/qla_target.c
>> +++ b/drivers/scsi/qla2xxx/qla_target.c
>> @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
>>  		else
>>  			vha->req->cnt = vha->req->length -
>>  			    (vha->req->ring_index - cnt);
>> -	}
>>  
>> -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
>> -		ql_dbg(ql_dbg_io, vha, 0x305a,
>> -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>> -		    vha->vp_idx, vha->req->ring_index,
>> -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
>> -		return -EAGAIN;
>> +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
>> +			ql_dbg(ql_dbg_io, vha, 0x305a,
>> +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
>> +			    vha->vp_idx, vha->req->ring_index,
>> +			    vha->req->cnt, req_cnt, cnt, cnt_in,
>> +			    vha->req->length);
>> +			return -EAGAIN;
>> +		}
>>  	}
>> +
>>  	vha->req->cnt -= req_cnt;
>>  
>>  	return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1359106

On Wed, 2016-03-16 at 16:03 +0100, Tomas Henzl wrote:
> On 15.3.2016 22:40, Arnd Bergmann wrote:
> > The qlt_check_reserve_free_req() function produces an incorrect warning
> > when CONFIG_PROFILE_ANNOTATED_BRANCHES is set:
> >
> > drivers/scsi/qla2xxx/qla_target.c: In function 'qlt_check_reserve_free_req':
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt_in' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >    ql_dbg(ql_dbg_io, vha, 0x305a,
> >    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        vha->vp_idx, vha->req->ring_index,
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >        vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> >        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > drivers/scsi/qla2xxx/qla_target.c:1887:3: error: 'cnt' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> >
> > The problem is that gcc fails to track the state of the condition across
> > an annotated branch.
> >
> > This slightly rearranges the code to move the second if() block
> > into the first one, to avoid the warning while retaining the
> > behavior of the code.
> 
> When the first 'if' is true the vha->req->ring_index gets a new value 
> assigned - so it could be possible that the second 'if' wont be true any more.
> The code should not be merged into that single 'if', or am I missing something?
> 
> tomash

If the first "if" is false, the second "if" will be false also, because
the vha->req->cnt value has not changed.  If the first "if" is true, the
nested second "if" will retest the condition.

The compiler is not at fault, because vha->req->cnt can't be tracked as
it could be modified by another thread/process.  It isn't, it's protected
by the ->hardware_lock, but the compiler doesn't know that.

-Ewan

> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> >  drivers/scsi/qla2xxx/qla_target.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> > index 985231900aca..8a44d1541eb4 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -1881,15 +1881,17 @@ static int qlt_check_reserve_free_req(struct scsi_qla_host *vha,
> >  		else
> >  			vha->req->cnt = vha->req->length -
> >  			    (vha->req->ring_index - cnt);
> > -	}
> >  
> > -	if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > -		ql_dbg(ql_dbg_io, vha, 0x305a,
> > -		    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> > -		    vha->vp_idx, vha->req->ring_index,
> > -		    vha->req->cnt, req_cnt, cnt, cnt_in, vha->req->length);
> > -		return -EAGAIN;
> > +		if (unlikely(vha->req->cnt < (req_cnt + 2))) {
> > +			ql_dbg(ql_dbg_io, vha, 0x305a,
> > +			    "qla_target(%d): There is no room in the request ring: vha->req->ring_index=%d, vha->req->cnt=%d, req_cnt=%d Req-out=%d Req-in=%d Req-Length=%d\n",
> > +			    vha->vp_idx, vha->req->ring_index,
> > +			    vha->req->cnt, req_cnt, cnt, cnt_in,
> > +			    vha->req->length);
> > +			return -EAGAIN;
> > +		}
> >  	}
> > +
> >  	vha->req->cnt -= req_cnt;
> >  
> >  	return 0;
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web