On Feb 17, 2016, at 12:43 AM, Dan Carpenter wrote:
> Those warnings are disabled unless --spammy is turned on.
> ./smatch -p=kernel --spammy lock_test.c
Ah, indeed. Ok.
>> Ah, unchecked for state to override whatever the state was assumed for that path - sneaky ;)
>> Luckily there's no printing of where we locked as that merge would totally throw it off.
>>
>> Anyway, while this test case is fixed, I run it across our codebase and noticed that one
>> true positive has been silenced too.
>>
>> void up(struct semaphore *);
>> void down(struct semaphore *);
>>
>> int some_random_stuff(void);
>>
>> void test_func2(void)
>> {
>> struct semaphore a_sem;
>>
>> if (some_random_stuff)
>> goto out;
>
> This should be a function call:
>
> if (some_random_stuff())
> goto out;
>
> Or it marks the rest as &impossible.
Does not matter, I noticed the lack of the function call and changed it to be one and did not
see any difference in smatch output.
After all this is just a small attempt at distilling down the actual real world code
(see live example in drivers/staging/lustre/lustre/llite/lloop.c::loop_thread() )
>>
>> up(&a_sem);
> Did you mean down()?
No. down() is made by the caller there.
>> for (;;) {
>> if (some_random_stuff)
>> break;
>> }
>>
>> out:
>> up(&a_sem);
>> __smatch_states("check_locking");
>
> The state is obviously going to be unlocked because we unlocked on the
> line before. The state before the up() is &merged (&start_state +
> &impossible) If I change the condition then it's (&start_state +
> &unlocked) so it generates a double unlock warning.
But not with the patch from the other email.
> If you unlock a (&start_state + &unlocked) then it probably should
> generate a warning but it doesn't... Check_locking is one of the oldest
> remaining checks and it needs to be re-written.
I actually noticed it has several modes of operation, sometimes just warning about
inconsistent locking and sometimes actually elaborating that here, here and here
we return locking, and there, thee and there we return unlocked.
> Thanks for the help on getting lustre to compile. I'm looking at the
> warning:
> lustre/llite/llite_lib.c:1709 ll_setattr_raw() warn: 'mutex:&inode->i_mutex' is sometimes locked here and sometimes unlocked.
> I see a bug in the flow analysis handling:
>
> if ((((inode->i_mode) & S_IFMT) == S_IFREG))
>
> The smatch_stored_conditions.c file ignores comparisons because
> smatch_extra.c is supposed to handle those. But smatch_extra.c doesn't
> handle it either...
So you mean the the warning going away with my patch was just an unintended bug and some real world bugs would be hidden by it?--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
