Re: [PATCH] x86: !x & y typo in mtrr code

Ingo Molnar
2008-04-30T09:32:58Z

* Bj?rn Steinbrink <B.Steinbrink@gmx.de> wrote:

> > firstly, it _was_ caught via Sparse, ...
>
> But that is exactly Al's point. Had you run sparse over your tree
> _before_ it got pulled by Linus, that bug should have never made it
> into mainline.

It is clear from your argument that you have never attempted to do this
yourself for any significant size of code. Sparse output is way too
verbose, the false positive rate is well above 50% and way too few
subsystems care about it.

What you have also missed is the plain fact that x86 _is_ amongst the
very few subsystems that _do_ regular Sparse runs. The person who does
this regular (and not trivial) manual Sparse sweep work for arch/x86 is
... Harvey, the person who submitted this fix! Most of the time Harvey's
Sparse fixes hit x86.git before we push it upstream. Sometimes, as in
this case, after. As per the analysis of this specific bug no puppies
were hurt - and that's typical of most Sparse fixes.

Sparse analysis is not just a matter of typing "make C=1" and fixing
whatever it prints... FYI, today's -git has the following Sparse
warnings histogram (sparse-0.4.1-2.fc9):

2 error: incompatible types in comparison expression (different address spaces)
3 error: dubious one-bit signed bitfield
5 error: subtraction of different types can't work (different address spaces)
10 error: incompatible types for operation (*)
10 error: invalid assignment
24 error: cannot size expression
97 error: bad constant expression
157 error: bad integer constant expression
86817 error: attribute '__cold__': unknown attribute

even ignoring those nearly one hundred thousand '__cold__' warnings
(which came upstream many months ago via commit v2.6.24-2245-gf3fe866),
that's still more than 300 warnings to sift through.

The reality is that nearly nobody runs Sparse for every patch and nearly
nobody cares about making the default C=1 output meaningful and
interesting to newbies. It's a frustrating experience at the moment, and
its (non-) uptake is reflecting that reality.

And as anyone can see it from the verbosity of the histogram above,
Sparse validation is a deeply manual work today, for something as
complex as general x86. Al has delta analysis automation around it but
those tools are not part of the upstream kernel yet and it's not usable
for daily patch-management.

Automating Sparse runs so that it can become part of our daily
patch-flow is _not_ trivial and it's not just a matter of typing make
C=1. Thomas is working on integrating Sparse delta analysis into our
daily patch workflow nevertheless.

But to suggest that we somehow should have done this before shows deep
ignorance of the problem space, at best.

Ingo