Re: [PATCH] x86/xen: silence smatch warning in pmu_msr_chk_emulated()

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Thu, Oct 20, 2022 at 10:22:17AM -0400, Boris Ostrovsky wrote:
> 
> On 10/20/22 9:34 AM, Juergen Gross wrote:
> > On 20.10.22 15:16, Jan Beulich wrote:
> > > On 20.10.2022 13:37, Juergen Gross wrote:
> > > > Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr
> > > > accesses") introduced code resulting in a warning issued by the smatch
> > > > static checker, claiming to use an uninitialized variable.
> > > > 
> > > > This is a false positive, but work around the warning nevertheless.
> > > 
> > > The risk of introducing a problem might be quite low here, but in general
> > > it exists: With the adjustment you remove any chance of the compiler
> > > spotting a missing initialization before use. And I'm not convinced using
> > > 0 in such a case would actually be ending up sufficiently benign.
> > 
> > Hmm, an alternative would be to initialize it to -1 and add a test for the
> > index to be >= 0 before using it.
> > 
> > Or to live with the smash warning with the chance, that a compiler might be
> > warning for the same reason in the future.
> 
> 
> Is smatch complaining about both variables or just index?

Just "index".

> There are two cases in is_intel_pmu_msr() where it returns true but
> index is not set so perhaps that's what bothers smatch?

Yep.  The "index" variable *is* undefined when it's passed so Smatch
is correct in what it's saying.  But it's is not used on that path
inside the function so it's harmless.

> It shold not complain if is_intel_pmu_msr() returns false.

Correct.

I kind of like the patch.  We generally say "fix the checker and don't
silence the warning" but in this case I feel like the checker is doing
the best possible thing and I'm not going to fix it.  Trying to silence
this warning in Smatch would come with some real downsides.

regards,
dan carpenter