On Fri, Feb 19, 2016 at 12:15:01PM -0500, Oleg Drokin wrote:
>
> On Feb 19, 2016, at 2:49 AM, Dan Carpenter wrote:
>
> > On Thu, Feb 18, 2016 at 05:35:47PM -0500, Oleg Drokin wrote:
> >>
> >> On Feb 18, 2016, at 5:04 PM, Dan Carpenter wrote:
> >>
> >>> On Wed, Feb 17, 2016 at 10:47:45AM -0500, Oleg Drokin wrote:
> >>>> 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.
> >>>>
> >>>
> >>> For me it works... I get a double unlock bug.
> >>
> >> I think we are talking about different things.
> >> I get double unlock bug too every time. Unless your patch is applied and then I don't.
> > I'm totally lost. What do you want it to do?
>
> I think the code with double up should report as a bug (double unlock).
> Your patch (the one about checking impossible state) silences it which makes me think
> some other true positives would be silenced too?
> The testcase I came up with does not work, but the original code still shows this, that's how I noticed it.
It does print the double unlock warning, though. Maybe send me your
exact test file.
>
> >>>> 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() )
> >>>
> >>> It's saying that that loop is a forever loop because we set
> >>> "lo->lo_state = LLOOP_BOUND;" at the start of the function and only
> >>> exit when it is set to LLOOP_RUNDOWN which I guess hapens in another
> >>> thread?
> >>>
> >>> Smatch doesn't understand about threads... I don't even have a roadmap
> >>> in my head how to handle this situation.
> >>
> >> I understand.
> >> That's why it should always be complaining about the double unlock, right? With your patch
> >> that tracks impossible states it no longer does this.
> >>
> >
> > No. It's working (if you ignore threads).
> >
> > // not locked.
> >
> > goto out;
> >
> > up();
> >
> > // state is unlocked
> >
> > for (;;) {
> >
> > if (impossible)
> > break;
>
> huh? state is "true" here, right? Because if we go by my original code, it's a function pointer so should not be NULL.
> if we convert it to a functional call, it's also possible, though not always.
> Seems there's an unrelated bug somewhere that assumes a pointer is always false or something like that
> (if we make the condition into "if (!some_random_stuff)" then the warning comes back and the merged state is suddenly unlocked
> where it should be impossible instead?)
> I guess my distilled test failed to live up to the expectations, but the original code in lloop.c is still affected.
Wow. Sorry. We're really talking at cross purposes. I'm describing
the flow analysis for drivers/staging/lustre/lustre/llite/lloop.c::loop_thread()
not the test file.
In loop_thread() we have a double unlock if "lo->lo_state ==
LLOOP_RUNDOWN", but that can only happen in another thread so Smatch
(incorrectly) says it is impossible and ignores it.
For the test file, whe the test is "if (some_random_stuff()) " then that
works, before and after the change. When it is "if (some_random_stuff) "
then that is evaulated as always true as you would expect so what
check_locking sees is:
void test1(void)
{
struct semaphore a_sem;
if (always_true)
goto out;
// impossible stuff snipped
out:
up(&a_sem);
}
So originally it would complain about the double unlock but now that
we're ignoring impossible stuff it doesn't.
regards,
dan carpenter
--
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
