Powered by Linux
Re: smatch usage? — Semantic Matching Tool

Re: smatch usage?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Apr 27, 2015, at 1:38 PM, Dan Carpenter wrote:

>> For example - the annoying false positives from functions that do:
>> do_stuff(int lock)
>> {
>> 	if (lock)
>> 		spin_lock(somelock);
>> 
>> 	stuff;
>> 
>> 	if (lock)
>> 		spin_unlock(somelock);
>> }
>> I hope there's a way to get an idea of the preceding branch conditions and compare notes on lock/unlock
>> for example?
> 
> This example code shouldn't cause a false positive.  smatch_implied.c
> knows that the "if (lock)" condition implies that lock is set.

I'd like to enter into evidence drivers/staging/lustre/lustre/lov/lov_io.c ;)
that currently produces:
drivers/staging/lustre/lustre/lov/lov_io.c:675 lov_io_submit() warn: 'mutex:&ld->ld_mutex' is sometimes locked here and sometimes unlocked.

The code itself is:
        if (alloc) {
…
	} else {
        	...
                mutex_lock(&ld->ld_mutex);
                lio->lis_mem_frozen = 1;
        }
….
        if (alloc) {
                OBD_FREE_LARGE(stripes_qin,
                         sizeof(*stripes_qin) * lio->lis_nr_subios);
        } else {

…
                lio->lis_mem_frozen = 0;
                mutex_unlock(&ld->ld_mutex);

	}


> I also recently made it so smatch_comparisons.c can set implications.
> smatch_comparison.c is used for when we are comparing:
> 
> 	if (unknown_variable <= other_unknown)
> 		lock();
> 
> What doesn't work is function calls:
> 
> 	if (needs_lock(foo))
> 
> That's tricky to add because of side effects.  (Not impossibly tricky).

I imagine we don't want to always ignore this anyway except for some well known
functions.

> Also "if (foo & NEEDS_LOCK)" doesn't work but it's similar to
> smatch_comparison.c so I should be able to add that.

More tricky is
if (something->bitfield & mask)
	spin_lock();

do_stuff(foo, something, somethingelse);

if (something->bitfield & mask)
	spin_unlock();

Here, we don't really know if the intermediate call happened to change the bitfield or not
(unless we do know, due to a very simple call path or something).

>> Any way to get the current function name to avoid false positives with aliased variable names?
> 
> "cur_func" has the function name a string.
> "cur_func_sym" has the function a symbol struct pointer.
> 
> I'm not sure what "aliased variable names" means.

it's stuff like this:
/home/green/smt/git/lustre-release/lnet/klnds/o2iblnd/o2iblnd_cb.c:3448 kiblnd_scheduler() error: double lock 'irqsave:flags'

                                spin_lock_irqsave(&sched->ibs_lock, flags);
                        }

                        kiblnd_conn_decref(conn); /* ...drop my ref from above */

Where kiblnd_conn_decref is a macro that also does spin_lock_irqsave(…, flags), but despite the same name, it's a different one:
#define kiblnd_conn_decref(conn)                                        \
do {                                                                    \
        unsigned long flags;                                            \
...
                spin_lock_irqsave(&kiblnd_data.kib_connd_lock, flags);  \

…

or could be a similar problem with inlined functions too?

>> Additionally the current failure to print filename where the error is if we are dealing with
>> a macro expansion from a header (it prints the correct line number, but wrong file).
> 
> I'm not certain what you mean here.  If it sees a macro() then it should
> use the file and line of the .c file.  I think you are talking about
> macros which generate functions?
> 
> Ah.  I think you are still looking at --info output and that is not
> meant for human consumption.  The filename for static inline functions
> needs to be the .c file for --info output but you shouldn't be looking
> at that.  For normal output it will use the .h filename as you would
> expect.

Hm, I ave not wrapped my head around this fully.
Right now I runs this build_kernel_data.sh which builds the whole kernel
(or external lustre tree) and gives me all the output I need.
It did not occur to me that I need to then do make clean and rebuild
once more.

Hm… So I just did that and that completely removed all the "sometimes locked here and sometimes unlocked"
warnings, certainly this should not be correct?
Some other stuff disappeared too, like some of the "could be null",
and all of the "potentially dereferencing uninitialized" messages - I know that at least one
of them was valid:
somefunc()
{
        struct ptlrpc_request *req;
…
        rc = ll_get_default_mdsize(sbi, &lmmsize);
        if (rc == 0)
                rc = md_getxattr(sbi->ll_md_exp, ll_inode2fid(inode), oc,
                                OBD_MD_FLXATTR, XATTR_NAME_LOV, NULL, 0,
                                lmmsize, 0, &req);
        capa_put(oc);
        if (rc < 0)
                RETURN(rc);

        body = req_capsule_server_get(&req->rq_pill, &RMF_MDT_BODY);
…

This one certainly shows in the build_kernel_data.sh build, but not in the test_kernel.sh output.

>> Oh, also the SQL statements - what are those for? just debug?
> 
> If you cat all the SQL statements into a 17 GB warns.txt file and then
> run `~/smatch/smatch_data/db/create_db.sh -p=kernel warns.txt` then it
> creates a smatch_db.sqlite with call and return path information so you
> can do cross function analysis.

Is there an example of this cross function analysis, as in does it
actually happen now anywhere? (I see the database is generated).

Thanks! This is very helpful.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux