On Aug 20, 2015, at 5:51 AM, Dan Carpenter wrote:
> On Wed, Aug 19, 2015 at 09:26:30PM -0400, Oleg Drokin wrote:
>> Hello!
>>
>> I pulled in the smatch update today (I was at 73de935ee75c55e09be67a995cd9c8a88aea5451 before) and
>> immediately I got a lot of extra warnings where random integer comparisons were marked as e.g.:
>> lustre/mdt/mdt_reint.c:668 mdt_reint_setattr() warn: we tested 'ma->ma_attr.la_valid' before and it was 'false'
>> if ((ma->ma_attr.la_valid & LA_SIZE) ||
>> ...
>> } else if ((ma->ma_valid & MA_LMV) && (ma->ma_valid & MA_INODE)) {
>> struct lu_buf *buf = &info->mti_buf;
>> struct mdt_lock_handle *lh;
>>
>> 668 if (ma->ma_attr.la_valid != 0)
>>
>>
>> Well, that's not really a boolean, could it be smart enough that when we use bitwise logical operands on the value,
>> or compare it to stuff other than 0 and 1, the variable becomes "not boolean" and does not generate those warnings
>> unless we compare with the exact same value (and then that value is printed)?
>
> It's not really a false positive, it's just that the warning is sucky
> and perhaps not very useful.
Well, usefulness is in the eye of the beholder as usual.
I am one of the bigger believers in "static analysis tools highlight suspicious code,
you then apply your brains to find what the real problems are", so in this
regard such warnings are useful.
I failed to anticipate that it's going to look into this much of the details.
I wonder if the warning text could be expanded to print the condition set more fully?
> It's not looking at boolean variables, it's looking at "simplified"
> conditions, and saying if they are known to be true or false. Normally
> checks deal with only simplified conditions. So instead of seeing:
>
> if (!!foo || (baz & FLAG) == 0) {
>
> The checks only see "foo" and "baz & FLAG". The plumbing of negation,
> zero compares and logical OR and AND are removed and handled
> automatically.
Yes, I understand that.
But sometimes what it thinks is a simplified condition is really not, I think?
Though I now see it is interpreted more narrowly.
>> I guess ability to look at struct members now also enables another class of false positives best seen as:
>> if (lqe->lqe_qunit == 0) {
>> /* lqe was read from disk, but neither qunit, nor edquot flag
>> * were initialized */
>> qmt_adjust_qunit(env, lqe);
>> if (lqe->lqe_qunit != 0)
>> qmt_adjust_edquot(lqe, cfs_time_current_sec());
>>
>> Which now generates:
>> lustre/quota/qmt_entry.c:695 qmt_revalidate() warn: we tested 'lqe->lqe_qunit' before and it was 'false'
>>
>> I guess it has no knowledge that lqe might be modified in that call and I'd just have to live with this one (to add
>> insult here, the field is not a boolean thing anyway and holds size).
>>
>
> It should have seen the modify. What is the output from:
>
> `~/path/to/smatch/smatch_data/db/smdb.py return_states qmt_adjust_qunit`
>
> I will look at ways to silence this if the cross function db is not
> set up.
Here's the output (note I only built the db using previous version of smatch, so that might be in play):
file | function | return_id | return_value | type | param | key | value |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 225 | | INTERNAL | -1 | | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 225 | | UNTRACKED_PARAM | 1 | $ | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 225 | | PARAM_LIMIT | 1 | $ | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 225 | | PARAM_LIMIT | 1 | $->lqe_site | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | INTERNAL | -1 | | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | UNTRACKED_PARAM | 1 | $ | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | PARAM_LIMIT | 1 | $ | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | PARAM_LIMIT | 1 | $->lqe_enforced | 1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | PARAM_LIMIT | 1 | $->lqe_id.qid_uid | 1-u64max |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | PARAM_LIMIT | 1 | $->lqe_site | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 228 | | PARAM_LIMIT | 1 | $->lqe_site->lqs_qtype | 0-1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | INTERNAL | -1 | | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | UNTRACKED_PARAM | 1 | $ | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $ | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->lqe_enforced | 1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->lqe_id.qid_uid | 1-u64max |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->lqe_site | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->lqe_site->lqs_qtype | 0-1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->u.me.lme_hardlimit | 0 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 237 | | PARAM_LIMIT | 1 | $->u.me.lme_softlimit | 0 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | INTERNAL | -1 | | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | UNTRACKED_PARAM | 1 | $ | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | PARAM_LIMIT | 1 | $ | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | PARAM_LIMIT | 1 | $->lqe_enforced | 1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | PARAM_LIMIT | 1 | $->lqe_id.qid_uid | 1-u64max |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | PARAM_LIMIT | 1 | $->lqe_site | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 240 | | PARAM_LIMIT | 1 | $->lqe_site->lqs_qtype | 0-1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | INTERNAL | -1 | | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | UNTRACKED_PARAM | 1 | $ | |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | PARAM_LIMIT | 1 | $ | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | PARAM_LIMIT | 1 | $->lqe_enforced | 1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | PARAM_LIMIT | 1 | $->lqe_id.qid_uid | 1-u64max |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | PARAM_LIMIT | 1 | $->lqe_site | 4096-2117777777777777777 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | PARAM_LIMIT | 1 | $->lqe_site->lqs_qtype | 0-1 |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | 1025 | 1 | $->lqe_qunit | 0-u64max |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 244 | | 1025 | 1 | $->u.me.lme_revoke_time | 0-u64max |
================================================
file | function | type | param | key | value |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 0 | $ |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $ |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_enforced |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_granted |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_id.qid_uid |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_qunit |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_site |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->lqe_site->lqs_qtype |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->u.me.lme_hardlimit |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->u.me.lme_revoke_time |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | 1026 | 1 | $->u.me.lme_softlimit |
/home/green/smt/git/lustre-release/lustre/quota/qmt_entry.c | qmt_adjust_qunit | DEREFERENCE | 1 | $ |
I'll try to build the db using the fully new version and see what would happen to these then.
>> Also a couple of really bizzare warnings that I have no explanation for:
>> lustre/lfsck/lfsck_layout.c:2388 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2384)
>> lustre/lfsck/lfsck_layout.c:2413 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2409)
>>
>> The code here is (full file: http://git.whamcloud.com/fs/lustre-release.git/blob/refs/heads/master:/lustre/lfsck/lfsck_layout.c):
>> 2409 if (bk->lb_param & LPF_DRYRUN)
>> GOTO(unlock_parent, rc = 1);
>>
>> dt_write_unlock(env, parent);
>> 2413 if (handle != NULL)
>> dt_trans_stop(env, dt, handle);
>>
>> huh?!
>
> Yeah. The line numbers are off... We originally dereference "handle"
> on line 2236, but only if the LPF_DRYRUN flag is not set. The line 2409
> that is mentioned is the test to say that the the flag is not set.
We don't really "dereference" on line 2236, we use handle there as an argument
to an (inline) function that passes it on to a handler from a nonstatic table.
So then those other functions do their own handle != NULL checks as desired.
> I think the warning seems sort of correct (the NULL check on line 2413
> could be removed). But it still shouldn't be printing this warning, I
What if the function checked for handle == NULL and returned some error value,
then yes, it's redundant.
Can we really assume that, though?
> think, because dt_trans_create() doesn't return NULL. What does the
> output of
> `~/path/to/smatch/smatch_data/db/smdb.py return_states dt_trans_create`
> look like?
The output is empty (it's an inline func, though).
It again calls into the method table.
Hm… cross-referencing the methods, it returns PTR_ERR, not NULL on error,
but we correctly check for it.
The whole thing revolves around "bk->lb_param & LPF_DRYRUN" condition.
If it is true - we do not create the transaction and the handle is always NULL.
Ah! That's must be why there is a warning, it sees the initial assignment
and then
if (bk->lb_param & LPF_DRYRUN)
GOTO(unlock_parent, rc = 1);
dt_write_unlock(env, parent);
if (handle != NULL)
dt_trans_stop(env, dt, handle);
so since we already checked for lb_param - we know handle could not be NULL
anymore.
Wow, this thing is smart! ;)
Thanks!
Bye,
Oleg--
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
