inline functions and context

DISCUSSION Christopher Li 2 participants 2 messages May 4, 2007 5 messages not in the archive Sparse checker

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Christopher Li

author maintainer

2007-05-04 18:52 UTC #5 lore raw

Adding sparse mailing list as Josh requested.

Quoted 6 lines · Josh Triplett

Quoted 5 lines · Christopher Li
Quoted 3 lines · Josh Triplett
Perhaps we should add inline functions to the symbols list. I personally like the idea of treating them as regular functions that just happen to have the "inline" attribute, at least until Sparse has a real need to perform inlining.

Quoted 6 lines

Quoted 3 lines
No. That is a bad idea. Inline function get include many times when checking on different source file. You will get tons duplicate warnings because different source file include the same header file. It also slow down the sparse checking.
The same thing happens for any other kind of warning that occurs on something in a header file; I don't consider that a problem.

If you do that, you will see tons of repeated warning on the spinlock header files. Sparse already has pretty high noise level, you really don't want more.

Quoted 15 lines

Quoted 9 lines
Quoted 5 lines
Not what I mean. I don't want to see multiple complaints about an unepected unlock in unlock_something, one per call to it; I want to see *one* complaint about an unexpected unlock in unlock_something, emitted when analyzing unlock_something. I don't think f should generate any warnings, unless you
That is pretty useless. Because we know that unlock_something is just a wrapper of unlocking. Telling me that this function has lock unbalance is not interesting.
How do you know? Why do we not assume that any function which unlocks without locking, like f, "is just a wrapper of unlocking"? I think we should stop at the first function that doesn't explicitly say "I intended to change the context", and not propogate the warning outward.

Quoted 5 lines

Quoted 1 line
I actually want to see the user of unlock_something() doing some thing wrong.
How do you know they did something wrong? Perhaps unlock_something did something wrong? Not every inline function that uses locks serves as a trivial lock wrapper.

Sparse can tell who's fault it is by just looking at the inline function. It also can't
tell if it is unlock_something() wrong or some thing wrong with f(). That is why it is better report on the f(). We can examine the f(), with ctags we can find out
what it really does, so if it is fault in unlock_something(), we still can tell.

The reverse it not true. If you only report error in unlock_something(). It is very
hard to find out which caller trigger the unbalance.

If there is some thing wrong with the inline function, sparse will complain when some one actually use it. You will not miss it.

Quoted 18 lines

Quoted 18 lines
Quoted 3 lines
add context annotations to lock_something and unlock_something and they don't match, or unless Sparse starts doing whole-program analysis and looks inside both lock_something and unlock_something and finds that they don't match.
In your case, you should just add context annotation to declaration of
lock_something.
Sparse will take into account that lock_something change context. It will also inline
unlock_something, which has some instruction will change context as well. In the end
sparse will find out context is balanced at exit. It is not as good as whole-program
analysis because you have to annotate lock_something manually. On the other hand,
annotation for lock_something is good for reading as well. There is very limited header
file need this kind of annotation.

Quoted 6 lines

Quoted 3 lines
The bottom line is, if you annotate the function correctly, sparse will do the right thing
on counting the balance..
It seems wrong to me that annotating lock_something and not annotating unlock_something will satisfy sparse.

That is because unlock_something has implementation detail get inlined while lock_something does not. The context annotation for lock_something is to make up for that.

Chris

Josh Triplett

2007-05-04 19:24 UTC #6 lore raw

Quoted 6 lines · Christopher Li

Quoted 5 lines · Josh Triplett
Quoted 4 lines · Christopher Li
Quoted 3 lines · Josh Triplett
Perhaps we should add inline functions to the symbols list. I personally like the idea of treating them as regular functions that just happen to have the "inline" attribute, at least until Sparse has a real need to perform inlining.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
No. That is a bad idea. Inline function get include many times when checking on different source file. You will get tons duplicate warnings because different source file include the same header file. It also slow down the sparse checking.
The same thing happens for any other kind of warning that occurs on something in a header file; I don't consider that a problem.
If you do that, you will see tons of repeated warning on the spinlock header files. Sparse already has pretty high noise level, you really don't want more.

You will only get such warnings until you annotate the inline spinlock functions.

Quoted 17 lines

Quoted 12 lines
Quoted 7 lines
Quoted 4 lines
Not what I mean. I don't want to see multiple complaints about an unepected unlock in unlock_something, one per call to it; I want to see *one* complaint about an unexpected unlock in unlock_something, emitted when analyzing unlock_something. I don't think f should generate any warnings, unless you
That is pretty useless. Because we know that unlock_something is just a wrapper of unlocking. Telling me that this function has lock unbalance is not interesting.
How do you know? Why do we not assume that any function which unlocks without locking, like f, "is just a wrapper of unlocking"? I think we should stop at the first function that doesn't explicitly say "I intended to change the context", and not propogate the warning outward.
Quoted 4 lines
Quoted 1 line
I actually want to see the user of unlock_something() doing some thing wrong.
How do you know they did something wrong? Perhaps unlock_something did something wrong? Not every inline function that uses locks serves as a trivial lock wrapper.

Quoted 6 lines

Sparse can tell who's fault it is by just looking at the inline function. It also can't
tell if it is unlock_something() wrong or some thing wrong with f(). That is why it is better report on the f(). We can examine the f(), with ctags we can find out
what it really does, so if it is fault in unlock_something(), we still can tell.

The moment you annotate unlock_something, the warning will show up in f instead.

Quoted 3 lines

The reverse it not true. If you only report error in unlock_something(). It is very
hard to find out which caller trigger the unbalance.

Once you look at unlock_something and decide that it does the right thing, you can add an annotation to it, and the warning will show up in f instead.

Quoted 2 lines

If there is some thing wrong with the inline function, sparse will complain when some one actually use it. You will not miss it.

But you first have to figure out that the warning comes from unlock_something, not from f. For a function with "unlock" in the name, that seems easy. For a random "do_something" inline function with broken locking, that might take some time. Pointing at the root cause of the warning, and letting the user tell you "no, that one does the right thing" with an annotation, seems like a better approach to me.

Quoted 20 lines

Quoted 20 lines
Quoted 20 lines
Quoted 3 lines
add context annotations to lock_something and unlock_something and they don't match, or unless Sparse starts doing whole-program analysis and looks inside both lock_something and unlock_something and finds that they don't match.
In your case, you should just add context annotation to declaration of
lock_something.
Sparse will take into account that lock_something change context. It will also inline
unlock_something, which has some instruction will change context as well. In the end
sparse will find out context is balanced at exit. It is not as good as whole-program
analysis because you have to annotate lock_something manually. On the other hand,
annotation for lock_something is good for reading as well. There is very limited header
file need this kind of annotation.
The bottom line is, if you annotate the function correctly, sparse will do the right thing
on counting the balance..

Quoted 6 lines

Quoted 2 lines
It seems wrong to me that annotating lock_something and not annotating unlock_something will satisfy sparse.
That is because unlock_something has implementation detail get inlined while lock_something does not. The context annotation for lock_something is to make up for that.

In the ideal future when Sparse has whole-program analysis, it could look inside both lock_something and unlock_something and find that the locking looks fine. The annotations would then only serve to localize warnings and simplify the analysis. However, I don't want to half-do interprocedural analysis, for inline functions only; that leads to the confusing scenario of having to annotate lock_something to prevent mismatch warnings. I'd prefer that if you haven't annotated either lock_something or unlock_something you see warnings on those two functions only.

Furthermore, as long as sparse does not distinguish between contexts, having many inlines that can change the context to unlocked may obscure real lock warnings.

- Josh Triplett