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.
> 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.
> > 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.
>
Yeah. It's not beautiful. The main thing though is that it doesn't do
cross function analysis...
> > 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?--
Smatch wasn't seing that the if (S_ISREG(inode->i_mode)) checks were
related so it *should* have complained.
Your patch is doing a clone_sm(). The sm has the data about where a
state is from.
if (x) {
foo = 0; <-- foo is from here.
} else if (y) {
foo = 1; <-- or here.
} else {
foo = 2;
}
<-- foo is a merged state 1-3.
if (foo) { <-- we look at all the original places where foo was
set. If foo is non-zero then we then we take
the value of x in all those places and merge
them together to get the implied x for the true
side. So x is zero.
The clone_sm() copies over that data but really we just want to copy the
line number.
+ if (one->line == two->line)
+ result->line = one->line;
I have these patches done, but I'm hoping to fix some other things and
push soon.
Sorry if these emails are longer and include a lot of unrelated
information :P It feels great to have these sorts of discussion.
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
