Re: [PATCH 6/9 v2] check context expressions as expressions

["Christopher Li" <sparse@chrisli.org>]

On Wed, Sep 10, 2008 at 12:33 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:

> +static int ident_equal(struct ident *ident1, struct ident *ident2)
> +{
> +       if (ident1 == ident2)
> +               return 1;
> +       if (!ident1 || !ident2)
> +               return 0;
> +
> +       return ident1->len == ident2->len &&
> +               !strncmp(ident1->name, ident2->name, ident1->len);
> +}

Nah, you pretty should never need to do that.
Ident equal should just need to compare ident1 == ident2.

All ident has been hashed. Same ident, should show up
as same pointer. Doing strncmp again is unnecessary.

I feel that this patch  is adding too much hack to the
sparse front end. At the very least, can you
just change __context__ parsing to accept symbol expression
and the let the checking back end to do the code motions
stuff?

I haven't study the new context checking very carefully.
I actually prefer the Linus's  old simple context checking.
Yes, that does not distinguish which symbols taking the lock.
But other than that it is working and counting the number
correctly. And, it is very simple.

The new context checking seems able to do more features.
But it has too many sore spots. I vote for back it out.

Instead of keep adding more hacks to fix up the problem. I
think we should step back and ask ourselves what do we really
want to achieve.

The fundamental problem I saw here is that, sparse does not
support cross function checking. There is no good way to save
some analyzed result for some function and used it later by other
function. That is why we actually have to put __context__
around so many functions. The __context__ describe what
these functions in forms of source code annotation. There is
only so much we can do with source code annotations.
I am not saying that annotation is not useful. I agree source
code annotation helps on the source code reading. But it
shouldn't limit checker only use the annotations. The checker
should be able to draw intelligent conclusions, by looking the
the function source code itself.

e.g. Why do we have to annotate foo_lock(&bar->lock) will take a lock
on &bar->lock? The checker should be able to find out foo_lock() just
callers some lower level locking functions. For example,  let say if we
can force foo to be inlined into the caller even foo is not declared as
inline. Then there is no need to annotate foo itself. The caller see
exactly what foo does.

So, I think we should implement cross functions checking capability
systematically rather than putting more hacks on source code
annotation. The writer patch I send out earlier is one step towards it.
I will write up more detail proposal.

Chris