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

[Ingo Molnar <mingo@elte.hu>]

* 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