Unreachable code diagnostic

Unreachable code diagnostic

[Matthew Wilcox]

I was recently sent some code that looked like this:

int foo()
{
	lock();
	return bar();
	unlock();
}

When you're restructuring code that contains locks, this is a
*really* easy mistake to make.  I've done it myself.  But there's no
compiler warning for it!  gcc doesn't have it, sparse doesn't have it.
I've mentioned it to the gcc developers and they don't seem terribly
enthusiastic (they had a -Wunreachable-code at one point, but it
got disabled, probably due to too many false positives like their
-Wmaybe-uninitialized).

Maybe sparse could warn about code after an unconditional return
statement?  I wouldn't like to see it warn about code after a conditional
return statement where the condition is always true; I think that would
have a lot of false positives due to macros.  For example, something
like this:

int foo()
{
	int i = 0;
	for (;;) {
		if (i == FOO_MAX)
			return i;
		bar(i++);
	}
}

if FOO_MAX happens to be 0 should silently optimise to 'return 0' and
not emit a warning.

I would like to see it warn in this case:

int foo()
{
	return bar();
	do { } while (0);
}

As that can be generated by the preprocessor in the case of optimising
away locks for the !SMP case, for example.

Any takers for this idea?  :-)

Re: Unreachable code diagnostic

[Linus Torvalds]

On Fri, Feb 24, 2017 at 10:07 AM, Matthew Wilcox <willy@infradead.org> wrote:
>
> Maybe sparse could warn about code after an unconditional return
> statement?

This is not likely to be a very successful model, I suspect.

You already partly see the problem:

> I wouldn't like to see it warn about code after a conditional
> return statement where the condition is always true; I think that would
> have a lot of false positives due to macros.

because the thing is, we actually have tons and tons of unreachable
code due to things like

   if (IS_ENABLED(CONFIG_XYZ)) {...

which allows us much cleaner code than using things like #ifdef's.

So yes, unreachable code in general is actually very common.

And it's not just in Linux - think of all the code you've ever seen
that has used "ASSERT()", which often ends up doing exactly the same
things. Even if you'd think that "when debugging isn't enabled, it
just goes away entirely", you often end up having things that
basically end up expanding to things like

    do { if (0) {..} } while (0)

inside macros very consciously in order to avoid compiler warnings
about unused variables etc when the variable is only used inside that
debug statement.

I realize that on the face of it, such a "if (0)" sounds insane, but
you can seriously just grep for that pattern in Linux:

    [torvalds@i7 linux]$ git grep 'if (0)' | wc -l
    156

now, getting back to your "limit it _only_ to code after an
unconditional 'return' statement" suggestion. The reason I don't
believe that will be all that useful either, is that a reasonable C
compiler (or something like sparse) simply doesn't even see many
conditionals.

That comes largely from how the C pre-processor is such a separate
phase and not actually integrated with the C syntax itself. So if any
of the conditionals above end up being done as cpp macros, it's
basically pretty much impossible to see them.

You'd actually likely be better off with something that doesn't
actually really parse the C code, but parses the code _without_ doing
preprocessor expansion, and basically look at it without doing the
full code analysis. More like what tools like checkpatch etc do -
lookign for the superficial patterns, rather than the patterns that
you see when you actually expand everything.

I'm not disputing that you can always find particular cases where a
warning would make sense, I just have a very strong suspicion that you
end up having to limit the condition you search for _so_ much that it
ends up being basically pointless for anything but the one or two
cases you already knew about and that triggered it.

              Linus

Re: Unreachable code diagnostic

[Josh Triplett]

On Fri, Feb 24, 2017 at 10:07:59AM -0800, Matthew Wilcox wrote:
> I was recently sent some code that looked like this:
> 
> int foo()
> {
> 	lock();
> 	return bar();
> 	unlock();
> }
> 
> When you're restructuring code that contains locks, this is a
> *really* easy mistake to make.  I've done it myself.  But there's no
> compiler warning for it!  gcc doesn't have it, sparse doesn't have it.

Sparse does have a warning (via -Wcontext) for this, if you annotate
lock() and unlock() with __acquires(somelock) and __releases(somelock),
which expand to __attribute__((context(somelock,0,1))) and
__attribute__((context(somelock,0,1))) respectively.  You'll get a
warning that foo() returns with the lock held.

Not at all perfect, but it does have reasonable handling of
conditionals, including a way to handle cond_lock().

Re: Unreachable code diagnostic

[Dan Carpenter]

Smatch works pretty well for finding unreachable code.

regards,
dan carpenter

Re: Unreachable code diagnostic

[Luc Van Oostenryck]

On Fri, Feb 24, 2017 at 10:57:07AM -0800, Josh Triplett wrote:
> On Fri, Feb 24, 2017 at 10:07:59AM -0800, Matthew Wilcox wrote:
> > I was recently sent some code that looked like this:
> > 
> > int foo()
> > {
> > 	lock();
> > 	return bar();
> > 	unlock();
> > }
> > 
> > When you're restructuring code that contains locks, this is a
> > *really* easy mistake to make.  I've done it myself.  But there's no
> > compiler warning for it!  gcc doesn't have it, sparse doesn't have it.
> 
> Sparse does have a warning (via -Wcontext) for this, if you annotate
> lock() and unlock() with __acquires(somelock) and __releases(somelock),
> which expand to __attribute__((context(somelock,0,1))) and
> __attribute__((context(somelock,0,1))) respectively.  You'll get a
> warning that foo() returns with the lock held.
> 
> Not at all perfect, but it does have reasonable handling of
> conditionals, including a way to handle cond_lock().

Absolutely, -Wcontext is even enabled by default.
So with what is done in the kernel, you have something like:
	#define __acquires(x)  __attribute__((context(x,0,1)))
	#define __releases(x)  __attribute__((context(x,1,0)))
	
	void lock(void) __acquires(lock);
	void unlock(void) __releases(lock);
	
	int bar(void);
	
	static int foo(void)
	{
	        lock();
	        return bar();
	        unlock();
	}

For which sparse returns the following warning:
	zz.c:9:5: warning: context imbalance in 'foo' - wrong count at exit

But of course, that's just for code properly lock/context annotated and I'm
not sure if what you're asking, which is much more general, is only motivated
by lock/unlock problems or by others problems too.

Luc Van Oostenryck

Re: Unreachable code diagnostic

[Luc Van Oostenryck]

On Fri, Feb 24, 2017 at 10:33:37AM -0800, Linus Torvalds wrote:
> On Fri, Feb 24, 2017 at 10:07 AM, Matthew Wilcox <willy@infradead.org> wrote:
> >
> > Maybe sparse could warn about code after an unconditional return
> > statement?
> 
> This is not likely to be a very successful model, I suspect.
> 
> You already partly see the problem:
> 
> > I wouldn't like to see it warn about code after a conditional
> > return statement where the condition is always true; I think that would
> > have a lot of false positives due to macros.
> 
> because the thing is, we actually have tons and tons of unreachable
> code due to things like
> 
>    if (IS_ENABLED(CONFIG_XYZ)) {...
> 
> which allows us much cleaner code than using things like #ifdef's.
> 
> So yes, unreachable code in general is actually very common.
> 

snip snip 

> 
> now, getting back to your "limit it _only_ to code after an
> unconditional 'return' statement" suggestion. The reason I don't
> believe that will be all that useful either, is that a reasonable C
> compiler (or something like sparse) simply doesn't even see many
> conditionals.
> 
> That comes largely from how the C pre-processor is such a separate
> phase and not actually integrated with the C syntax itself. So if any
> of the conditionals above end up being done as cpp macros, it's
> basically pretty much impossible to see them.
> 
> You'd actually likely be better off with something that doesn't
> actually really parse the C code, but parses the code _without_ doing
> preprocessor expansion, and basically look at it without doing the
> full code analysis. More like what tools like checkpatch etc do -
> lookign for the superficial patterns, rather than the patterns that
> you see when you actually expand everything.
> 
> I'm not disputing that you can always find particular cases where a
> warning would make sense, I just have a very strong suspicion that you
> end up having to limit the condition you search for _so_ much that it
> ends up being basically pointless for anything but the one or two
> cases you already knew about and that triggered it.
> 
>               Linus


I'm not very sure what are the cases in wich Matthew is really
interested but I suppose that, even after preprocessing and
elimination of if (0) {...}, a return statement in the middle
of a compound statement is very often unintentional. 
That should be easy to check.


Luc Van Oostenryck

Re: Unreachable code diagnostic

[Josh Triplett]

On Fri, Feb 24, 2017 at 09:11:53PM +0100, Luc Van Oostenryck wrote:
> I'm not very sure what are the cases in wich Matthew is really
> interested but I suppose that, even after preprocessing and
> elimination of if (0) {...}, a return statement in the middle
> of a compound statement is very often unintentional. 
> That should be easy to check.

By "middle of a compound statement", you mean an unconditional return
followed by more code?  Yes, that seems like something reasonable to
statically check.

- Josh Triplett

Re: Unreachable code diagnostic

[Luc Van Oostenryck]

On Fri, Feb 24, 2017 at 12:43:56PM -0800, Josh Triplett wrote:
> On Fri, Feb 24, 2017 at 09:11:53PM +0100, Luc Van Oostenryck wrote:
> > I'm not very sure what are the cases in wich Matthew is really
> > interested but I suppose that, even after preprocessing and
> > elimination of if (0) {...}, a return statement in the middle
> > of a compound statement is very often unintentional. 
> > That should be easy to check.
> 
> By "middle of a compound statement", you mean an unconditional return
> followed by more code?  Yes, that seems like something reasonable to
> statically check.
> 
> - Josh Triplett

Yes, it's what I meant.

Luc

Re: Unreachable code diagnostic

[Matthew Wilcox]

On Fri, Feb 24, 2017 at 10:57:07AM -0800, Josh Triplett wrote:
> On Fri, Feb 24, 2017 at 10:07:59AM -0800, Matthew Wilcox wrote:
> > I was recently sent some code that looked like this:
> > 
> > int foo()
> > {
> > 	lock();
> > 	return bar();
> > 	unlock();
> > }
> > 
> > When you're restructuring code that contains locks, this is a
> > *really* easy mistake to make.  I've done it myself.  But there's no
> > compiler warning for it!  gcc doesn't have it, sparse doesn't have it.
> 
> Sparse does have a warning (via -Wcontext) for this, if you annotate
> lock() and unlock() with __acquires(somelock) and __releases(somelock),
> which expand to __attribute__((context(somelock,0,1))) and
> __attribute__((context(somelock,0,1))) respectively.  You'll get a
> warning that foo() returns with the lock held.
> 
> Not at all perfect, but it does have reasonable handling of
> conditionals, including a way to handle cond_lock().

Ah, yes, thanks.  I didn't actually try to compile the patch I was
sent ... I was just bemused that the compiler didn't warn about this
"obvious" wrongness.  So I wrote a test-case, which of course didn't have
any lock annotations.  rcu_read_lock()/unlock() are correctly annotated
and applying the patch I sent produces a sparse (and not gcc) warning.
So I've asked the submitter to run sparse in future.

(of course, this means they have to ignore all the *other* pre-existing
sparse warnings, but that's not the fault of anyone on this mailing list)