Hi Colin,
> while working through a backlog of older static analysis reports from
> Coverity
So ... yeah. Every time I look at Coverity (not frequently, I must
admit) I see the same thing, and get confused.
> I found an interesting use of the ~ operator that looks
> incorrect to me in function ieee80211_set_bitrate_mask():
>
> for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
> if (~sdata->rc_rateidx_mcs_mask[i][j]) {
> sdata->rc_has_mcs_mask[i] = true;
> break;
> }
> }
>
> for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
> if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
> sdata->rc_has_vht_mcs_mask[i] = true;
> break;
> }
> }
>
> For the ~ operator in both if stanzas, Coverity reports:
>
> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
> logical_vs_bitwise:
>
> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
> values of its operand. This occurs as the logical operand of if.
> Did you intend to use ! rather than ~?
>
> I've checked the results of this and it does seem that ~ is incorrect
> and always returns true for the if expression. So it probably should be
> !, but I'm not sure if I'm missing something deeper here and wondering
> why this has always worked.
But is it really always true?
I _think_ it was intended to check that it's not 0xffffffff or
something?
https://lore.kernel.org/linux-wireless/[email protected]/
But maybe that isn't actually quite right due to integer promotion?
OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
to 0 also?
johannes
On 05/02/2021 18:05, Johannes Berg wrote:
> Hi Colin,
>
>> while working through a backlog of older static analysis reports from
>> Coverity
>
> So ... yeah. Every time I look at Coverity (not frequently, I must
> admit) I see the same thing, and get confused.
>
>> I found an interesting use of the ~ operator that looks
>> incorrect to me in function ieee80211_set_bitrate_mask():
>>
>> for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>> if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>> sdata->rc_has_mcs_mask[i] = true;
>> break;
>> }
>> }
>>
>> for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>> if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>> sdata->rc_has_vht_mcs_mask[i] = true;
>> break;
>> }
>> }
>>
>> For the ~ operator in both if stanzas, Coverity reports:
>>
>> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
>> logical_vs_bitwise:
>>
>> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
>> values of its operand. This occurs as the logical operand of if.
>> Did you intend to use ! rather than ~?
>>
>> I've checked the results of this and it does seem that ~ is incorrect
>> and always returns true for the if expression. So it probably should be
>> !, but I'm not sure if I'm missing something deeper here and wondering
>> why this has always worked.
>
> But is it really always true?
>
> I _think_ it was intended to check that it's not 0xffffffff or
> something?
>
> https://lore.kernel.org/linux-wireless/[email protected]/
>
> But maybe that isn't actually quite right due to integer promotion?
> OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
> to 0 also?
rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
expressed as:
if ((uint16_t)~sdata->rc_rateidx_mcs_mask[i][j]) ..
this is only true if all the 16 bits in the mask are 0xffff
>
> johannes
>
On 05/02/2021 18:19, Colin Ian King wrote:
> On 05/02/2021 18:05, Johannes Berg wrote:
>> Hi Colin,
>>
>>> while working through a backlog of older static analysis reports from
>>> Coverity
>>
>> So ... yeah. Every time I look at Coverity (not frequently, I must
>> admit) I see the same thing, and get confused.
>>
>>> I found an interesting use of the ~ operator that looks
>>> incorrect to me in function ieee80211_set_bitrate_mask():
>>>
>>> for (j = 0; j < IEEE80211_HT_MCS_MASK_LEN; j++) {
>>> if (~sdata->rc_rateidx_mcs_mask[i][j]) {
>>> sdata->rc_has_mcs_mask[i] = true;
>>> break;
>>> }
>>> }
>>>
>>> for (j = 0; j < NL80211_VHT_NSS_MAX; j++) {
>>> if (~sdata->rc_rateidx_vht_mcs_mask[i][j]) {
>>> sdata->rc_has_vht_mcs_mask[i] = true;
>>> break;
>>> }
>>> }
>>>
>>> For the ~ operator in both if stanzas, Coverity reports:
>>>
>>> Logical vs. bitwise operator (CONSTANT_EXPRESSION_RESULT)
>>> logical_vs_bitwise:
>>>
>>> ~sdata->rc_rateidx_mcs_mask[i][j] is always 1/true regardless of the
>>> values of its operand. This occurs as the logical operand of if.
>>> Did you intend to use ! rather than ~?
>>>
>>> I've checked the results of this and it does seem that ~ is incorrect
>>> and always returns true for the if expression. So it probably should be
>>> !, but I'm not sure if I'm missing something deeper here and wondering
>>> why this has always worked.
>>
>> But is it really always true?
>>
>> I _think_ it was intended to check that it's not 0xffffffff or
>> something?
>>
>> https://lore.kernel.org/linux-wireless/[email protected]/
>>
>> But maybe that isn't actually quite right due to integer promotion?
>> OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
>> to 0 also?
>
> rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
> expressed as:
oops, fat fingered that, it is a u16 not a u64
>
> if ((uint16_t)~sdata->rc_rateidx_mcs_mask[i][j]) ..
>
> this is only true if all the 16 bits in the mask are 0xffff
>
>>
>> johannes
>>
>
On Fri, 2021-02-05 at 18:20 +0000, Colin Ian King wrote:
>
> > > https://lore.kernel.org/linux-wireless/[email protected]/
> > >
> > > But maybe that isn't actually quite right due to integer promotion?
> > > OTOH, that's a u8, so it should do the ~ in u8 space, and then compare
> > > to 0 also?
> >
> > rc_rateidx_vht_mcs_mask is a u64, so I think the expression could be
> > expressed as:
>
> oops, fat fingered that, it is a u16 not a u64
Right, u16, I must've looked at some ancient version or something.
But no, I was obviously wrong with what I said above.
So of course the condition is always true, like you said.
However, what was intended doesn't look like !, but rather == 0xff and
== 0xffff respectively, I'll send a patch.
johannes