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

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]

On Wed, 2008-09-10 at 12:21 -0700, Christopher Li wrote:

> 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?

Can you explain?

> 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.

Well, yeah, it's very simple, and that doesn't help. And it's not about
cross-function checking either:

extern spinlock_t a;

spin_lock(&a);
rcu_read_unlock()

will not result in an error which is a pain.

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

I don't care, I don't have any more cycles to burn on this. For all I'm
concerned it works pretty well and is a HUGE improvement over the
current sparse which
 * doesn't tell you what line the error really is in, it only warns
   about the last line of the function
 * isn't able to check different contexts despite documenting it
   (see above)

> 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.

See above.

> 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.

Actually, I kinda disagree. Annotating *each and every function* with
the locks it requires is _very_ useful, not just for sparse but also for
human readers of the code. Hence, I don't just want sparse to check
across functions, I want to be able to tell people what to do for
calling a given function as well. The fact that sparse can check my
annotations is great, but having them would be useful without that as
well.

> 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.

It used to be like that. But I'm arguing (and others backed me up on
this) that inlines are really just functions and as such should be
annotated, not magically inlined into the code and then checked as
sparse behaves right now.

> 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.

Please don't focus on cross-function checking so much. Having different
contexts is way more desirable, and cross-function checking probably is
_not_ desirable in that you _want_ the annotations.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]