Sparse errors

Sparse errors

[Pierre-Louis Bossart]

Hi Takashi,
Sparse reports a lot of new issues in our last checks with more options:

export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all -Wno-bitwise-pointer 
-Wno-pointer-arith -Wno-typesign -Wnoshadow -Wno-sizeof-bool"
make -k sound/ C=2

most are linked to the __user and pcm_format_t restricted types, but I 
found the simpler ones below which are useless comparisons. I can send a 
patch for the last but not sure how to address the first two.

Thanks for your feedback
-Pierre

sound/core/info.c:95:38: error: self-comparison always evaluates to false

	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
		return false;

not sure what the second comparison is meant to check?

sound/drivers/opl3/opl3_midi.c:183:60: error: self-comparison always 
evaluates to false

This indeed makes no sense. the voice_time and vp->time are not changed 
in the loop, the test is either redundant or something else is missing.

sound/pci/lx6464es/lx_core.c:677:34: error: self-comparison always 
evaluates to false

That seems like dead code indeed:

	u32 channels = runtime->channels;

	if (runtime->channels != channels)
		dev_err(chip->card->dev, "channel count mismatch: %d vs %d",
			   runtime->channels, channels);

Re: Sparse errors

[Dan Carpenter]

On Tue, May 25, 2021 at 02:32:27PM -0500, Pierre-Louis Bossart wrote:
> Hi Takashi,
> Sparse reports a lot of new issues in our last checks with more options:
> 
> export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all -Wno-bitwise-pointer
> -Wno-pointer-arith -Wno-typesign -Wnoshadow -Wno-sizeof-bool"
> make -k sound/ C=2
> 
> most are linked to the __user and pcm_format_t restricted types, but I found
> the simpler ones below which are useless comparisons. I can send a patch for
> the last but not sure how to address the first two.
> 
> Thanks for your feedback
> -Pierre
> 
> sound/core/info.c:95:38: error: self-comparison always evaluates to false
> 
> 	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
> 		return false;
> 
> not sure what the second comparison is meant to check?

It's checking for if a 32 bit system is using the upper 32 bits of a
u64.

This one is valid, the rest are nonsense code.

regards,
dan carpenter

Re: Sparse errors

[Takashi Iwai]

On Tue, 25 May 2021 21:32:27 +0200,
Pierre-Louis Bossart wrote:
> 
> Hi Takashi,
> Sparse reports a lot of new issues in our last checks with more options:
> 
> export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all
> -Wno-bitwise-pointer -Wno-pointer-arith -Wno-typesign -Wnoshadow
> -Wno-sizeof-bool"
> make -k sound/ C=2
> 
> most are linked to the __user and pcm_format_t restricted types, but I
> found the simpler ones below which are useless comparisons. I can send
> a patch for the last but not sure how to address the first two.
> 
> Thanks for your feedback
> -Pierre
> 
> sound/core/info.c:95:38: error: self-comparison always evaluates to false
> 
> 	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
> 		return false;
> 
> not sure what the second comparison is meant to check?

As Dan suggested, it's a check only for 32bit architecture for a 64bit
value.

> sound/drivers/opl3/opl3_midi.c:183:60: error: self-comparison always
> evaluates to false
> 
> This indeed makes no sense. the voice_time and vp->time are not
> changed in the loop, the test is either redundant or something else is
> missing.

The code doesn't look right, indeed.  It's likely meant to be vp2
instead of vp.

--- a/sound/drivers/opl3/opl3_midi.c
+++ b/sound/drivers/opl3/opl3_midi.c
@@ -180,8 +180,7 @@ static int opl3_get_voice(struct snd_opl3 *opl3, int instr_4op,
 			if (vp2->state == SNDRV_OPL3_ST_ON_2OP) {
 				/* kill two voices, EXPENSIVE */
 				bp++;
-				voice_time = (voice_time > vp->time) ?
-					voice_time : vp->time;
+				voice_time = max(voice_time, vp2->time);
 			}
 		} else {
 			/* allocate 2op voice */


> sound/pci/lx6464es/lx_core.c:677:34: error: self-comparison always
> evaluates to false
> 
> That seems like dead code indeed:
> 
> 	u32 channels = runtime->channels;
> 
> 	if (runtime->channels != channels)
> 		dev_err(chip->card->dev, "channel count mismatch: %d vs %d",
> 			   runtime->channels, channels);

Yes, this can be deleted.


thanks,

Takashi

Re: Sparse errors

[Pierre-Louis Bossart]

On 5/26/21 2:40 AM, Takashi Iwai wrote:
> On Tue, 25 May 2021 21:32:27 +0200,
> Pierre-Louis Bossart wrote:
>>
>> Hi Takashi,
>> Sparse reports a lot of new issues in our last checks with more options:
>>
>> export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all
>> -Wno-bitwise-pointer -Wno-pointer-arith -Wno-typesign -Wnoshadow
>> -Wno-sizeof-bool"
>> make -k sound/ C=2
>>
>> most are linked to the __user and pcm_format_t restricted types, but I
>> found the simpler ones below which are useless comparisons. I can send
>> a patch for the last but not sure how to address the first two.
>>
>> Thanks for your feedback
>> -Pierre
>>
>> sound/core/info.c:95:38: error: self-comparison always evaluates to false
>>
>> 	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
>> 		return false;
>>
>> not sure what the second comparison is meant to check?
> 
> As Dan suggested, it's a check only for 32bit architecture for a 64bit
> value.

Isn't there a better way to check this?

>> sound/drivers/opl3/opl3_midi.c:183:60: error: self-comparison always
>> evaluates to false
>>
>> This indeed makes no sense. the voice_time and vp->time are not
>> changed in the loop, the test is either redundant or something else is
>> missing.
> 
> The code doesn't look right, indeed.  It's likely meant to be vp2
> instead of vp.
> 
> --- a/sound/drivers/opl3/opl3_midi.c
> +++ b/sound/drivers/opl3/opl3_midi.c
> @@ -180,8 +180,7 @@ static int opl3_get_voice(struct snd_opl3 *opl3, int instr_4op,
>   			if (vp2->state == SNDRV_OPL3_ST_ON_2OP) {
>   				/* kill two voices, EXPENSIVE */
>   				bp++;
> -				voice_time = (voice_time > vp->time) ?
> -					voice_time : vp->time;
> +				voice_time = max(voice_time, vp2->time);
>   			}
>   		} else {
>   			/* allocate 2op voice */

It's really old code, unchanged since the first git commit in 2005... 
Are you comfortable changing this code? One of those cases where if 
people didn't notice an issue in 16+ years maybe no one cares or even 
uses this driver...

>> sound/pci/lx6464es/lx_core.c:677:34: error: self-comparison always
>> evaluates to false
>>
>> That seems like dead code indeed:
>>
>> 	u32 channels = runtime->channels;
>>
>> 	if (runtime->channels != channels)
>> 		dev_err(chip->card->dev, "channel count mismatch: %d vs %d",
>> 			   runtime->channels, channels);
> 
> Yes, this can be deleted.

I'll send a patch for this, thanks for the feedback.

Re: Sparse errors

[Takashi Iwai]

On Wed, 26 May 2021 16:17:18 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 5/26/21 2:40 AM, Takashi Iwai wrote:
> > On Tue, 25 May 2021 21:32:27 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >> Hi Takashi,
> >> Sparse reports a lot of new issues in our last checks with more options:
> >>
> >> export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all
> >> -Wno-bitwise-pointer -Wno-pointer-arith -Wno-typesign -Wnoshadow
> >> -Wno-sizeof-bool"
> >> make -k sound/ C=2
> >>
> >> most are linked to the __user and pcm_format_t restricted types, but I
> >> found the simpler ones below which are useless comparisons. I can send
> >> a patch for the last but not sure how to address the first two.
> >>
> >> Thanks for your feedback
> >> -Pierre
> >>
> >> sound/core/info.c:95:38: error: self-comparison always evaluates to false
> >>
> >> 	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
> >> 		return false;
> >>
> >> not sure what the second comparison is meant to check?
> >
> > As Dan suggested, it's a check only for 32bit architecture for a 64bit
> > value.
> 
> Isn't there a better way to check this?

Sure, there are hundreds other ways, but they might be longer :)
If any, a comment would be deserved.

> >> sound/drivers/opl3/opl3_midi.c:183:60: error: self-comparison always
> >> evaluates to false
> >>
> >> This indeed makes no sense. the voice_time and vp->time are not
> >> changed in the loop, the test is either redundant or something else is
> >> missing.
> >
> > The code doesn't look right, indeed.  It's likely meant to be vp2
> > instead of vp.
> >
> > --- a/sound/drivers/opl3/opl3_midi.c
> > +++ b/sound/drivers/opl3/opl3_midi.c
> > @@ -180,8 +180,7 @@ static int opl3_get_voice(struct snd_opl3 *opl3, int instr_4op,
> >   			if (vp2->state == SNDRV_OPL3_ST_ON_2OP) {
> >   				/* kill two voices, EXPENSIVE */
> >   				bp++;
> > -				voice_time = (voice_time > vp->time) ?
> > -					voice_time : vp->time;
> > +				voice_time = max(voice_time, vp2->time);
> >   			}
> >   		} else {
> >   			/* allocate 2op voice */
> 
> It's really old code, unchanged since the first git commit in
> 2005... Are you comfortable changing this code? One of those cases
> where if people didn't notice an issue in 16+ years maybe no one cares
> or even uses this driver...

It'll be a safe change, so I'd apply the fix.

It's about the choice of an OPL3 voice, and the function tries to find
the best one.  The code seems to want to align with the timestamp of
the coupled voice, and even if it's not right, the impact is very
limited.


thanks,

Takashi

Re: Sparse errors

[Dan Carpenter]

On Wed, May 26, 2021 at 09:17:18AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 5/26/21 2:40 AM, Takashi Iwai wrote:
> > On Tue, 25 May 2021 21:32:27 +0200,
> > Pierre-Louis Bossart wrote:
> > > 
> > > Hi Takashi,
> > > Sparse reports a lot of new issues in our last checks with more options:
> > > 
> > > export ARCH=x86_64 CF="-Wsparse-error -Wsparse-all
> > > -Wno-bitwise-pointer -Wno-pointer-arith -Wno-typesign -Wnoshadow
> > > -Wno-sizeof-bool"
> > > make -k sound/ C=2
> > > 
> > > most are linked to the __user and pcm_format_t restricted types, but I
> > > found the simpler ones below which are useless comparisons. I can send
> > > a patch for the last but not sure how to address the first two.
> > > 
> > > Thanks for your feedback
> > > -Pierre
> > > 
> > > sound/core/info.c:95:38: error: self-comparison always evaluates to false
> > > 
> > > 	if (pos < 0 || (long) pos != pos || (ssize_t) count < 0)
> > > 		return false;
> > > 
> > > not sure what the second comparison is meant to check?
> > 
> > As Dan suggested, it's a check only for 32bit architecture for a 64bit
> > value.
> 
> Isn't there a better way to check this?
> 

I suppose you could do:

	if (pos > ULONG_MAX) {

I think Smatch used to complain about code like that but it doesn't
now?

Really the correct thing is to fix the static checker because the code
is fine.  To me as a human reader it was pretty obvious that the code
was deliberately checking that the value was still the same after a
cast.

regards,
dan carpenter