Re: [PATCH] staging: rtl8192u: initialize array in C compliant way
On Tue, May 6, 2014 at 1:47 AM, Dan Carpenter wrote:
>> - boolsearch_dir[4] = {0, 0, 0, 0};
>> + boolsearch_dir[4] = {0};
>
> That's weird. The original code is valid but it generates a sparse
> warning.
>
> drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:244:58: warning:
> Initializer entry defined twice
> drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c:244:61: also defined
> here
>
> It has something to do with "_Bool" types. Changing it to "int" will
> also make the warning disappear. I've CC'd the sparse list to see if
> anyone knows what's happening.
This is easy reproducible with a small test case. It is a sparse bug for sure.
Let me take a look.
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: rtl8192u: initialize array in C compliant way
On Tue, May 6, 2014 at 1:47 AM, Dan Carpenter wrote:
>> diff --git a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> index 426f223..c96dbab 100644
>> --- a/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> +++ b/drivers/staging/rtl8192u/ieee80211/rtl819x_TSProc.c
>> @@ -241,7 +241,7 @@ static PTS_COMMON_INFO SearchAdmitTRStream(struct
>> ieee80211_device *ieee,
>> {
>> //DIRECTION_VALUE dir;
>> u8 dir;
>> - boolsearch_dir[4] = {0, 0, 0, 0};
>> + boolsearch_dir[4] = {0};
>
> That's weird. The original code is valid but it generates a sparse
> warning.
Sorry for the late reply. I see what is going on already. The bits_to_bytes()
function is buggy for bool type. Bool is the only type has bits less than 8.
It truncates the bool byte size to 0. As a result, the array layout function
convert_index() always set the expr->init_offset to 0 for bool type:
"e->init_offset = from * bits_to_bytes(e->ctype->bit_size);"
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
Sorry for the late reply.
On Wed, Jun 11, 2014 at 2:45 PM, wrote:
> On Thu, Jun 12, 2014 at 12:24:25AM +0300, Dan Carpenter wrote:
>> Let's forward this to the Sparse mailing list.
>>
>> We're seeing a Sparse false positive testing
No, this is a valid complain. See my point follows.
>> drivers/staging/comedi/drivers/ni_pcimio.c.
>>
>> CHECK drivers/staging/comedi/drivers/ni_pcimio.c
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
>> (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
>> (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
>> (4294967295) for type int
>> drivers/staging/comedi/drivers/ni_stc.h:720:26: warning: shift too big
>> (4294967295) for type int
>>
>> I have created some test code to demonstrate the problem (attached).
>>
>> The check_shift_count() warning is only supposed to be printed for
>> number literals but because of the way inline functions are expanded it
>> still complains even though channel is a variable.
>
> Thanks for the test case; this definitely makes no sense. I don't think
> Sparse will suddenly develop enough range analysis or reachability
> analysis to handle this case; I think the right answer is to avoid
> giving such warnings for shifts with a non-constant RHS.
Sparse can handle inline function expand and some constant
propagate. In this case, sparse seems doing the right thing.
Sparse actually sees the channel value being 4294967295 (-1).
>> static inline unsigned ni_stc_dma_channel_select_bitfield(unsigned channel)
This is the bug. See this channel is *unsigned*. When -1 pass into
channel, it become a really large number 4294967295.
The code does request C compiler to perform left shift 4294967295 bits.
Which did not make sense.
>> {
>> if (channel < 4)
>> return 1 << channel;
>> return 0;
>> }
>>
>> static inline void filter(int channel)
>> {
>> if (channel < 0)
>> return;
>> ni_stc_dma_channel_select_bitfield(channel);
See this channel is *signed*, with -1 convert to 4294967295.
This is a bug in the kernel source not sparse.
Chris
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
On Sat, Jun 28, 2014 at 12:20 PM, Josh Triplett wrote: > Except that "filter" has an "int channel" (signed), so it can > successfully test "channel < 0" and return early; it'll never call > ni_stc_dma_channel_select_bitfield(channel) with a negative number. > > I do agree that this code should sort out the signedness of its types, > but in this case I don't think the bad shift can actually happen, making > this a false positive. Ah, I see. I agree that the warning happen on the dead code. However, the root cause for the warning is not because sparse can not figure out the value of "channel". Sparse actually correctly does the inline and figure out "channel" is 4294967295. The root cause of your complain against sparse is that it happen on the dead code. I don' think the patch is the right fix for this problem though. This patch try to silence a warning purely because it can happen on dead code, otherwise perfectly valid warning. My point is that, it can happen on dead code is not a valid reason to weaken the warning. Sparse can give any possible warning on dead code. e.g. On the dead code path, you can construct an address space warning. Do we weakling the address space warning purely because it can happen on the dead code as well? Of course not. The same apply to any other sparse warnings. So if there is a bug, It is sparse did not know exactly which part of the code is dead on AST level. If you enable the "#if 0" inside expand_if_statement(), sparse can actually correctly expand "if (channel < 0)" into "return". Of course, it still can't handle jump into dead code situation, which is the reason that piece of code is not enabled in the first place. It needs more work. I think that is a better approach than just random weakening some warnings on dead code path. Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
On Sat, Jun 28, 2014 at 8:09 PM, Christopher Li wrote: > So if there is a bug, It is sparse did not know exactly which part > of the code is dead on AST level. If you enable the "#if 0" inside I am thinking about how to fix the dead code issue. Try to do more than trivial dead code analyses is really not the right place for AST. Actually, there is a simple way out. We can just move the warning to the linearize instruction level. $ ./test-linearize /tmp/test_shift.c /tmp/test_shift.c:8:26: warning: shift too big (4294967295) for type int main: .L0x7f8f490e5010: # call %r1 <- filter, $0x ret.32 $0 You see, once drop into the instruction level, sparse already know that code is dead. Since the sparse is already linearizing instruction to perform the context check. We just need to move the warning from AST to instruction level. The warning should be trivial in instruction level. We are looking for an instruction has larger than type size constant shift value. Comments and feed backs, even patches? Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging/comedi: Fixes static analysis warning raised by sparse
On Mon, Jun 30, 2014 at 10:49 AM, Christopher Li wrote: > The warning should be trivial in instruction level. We are looking > for an instruction has larger than type size constant shift > value. Actually, just try it. Not as trivial as I thought. The problem is that, in the instruction level, sparse will optimize away the shift instruction when it see a larger than type size shift. The finial instruction level don't see the big constant shift any way. Need to catch it before the instruction get optimized away. That is unfortunate. If we check it too early, we can't tell it is in the dead code. If we check it too late, the instruction itself get optimized away. Chris ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
