Re: [PATCH 3/5] powerpc-pseries: Delete an unnecessary variable initialisation in iommu_pseries_alloc_group()

[Michal SuchÃnek]

On Thu, 19 Oct 2017 15:55:59 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:

> On Thu, Oct 19, 2017 at 01:37:18PM +0200, Michal SuchÃnek wrote:
> > Hello,
> > 
> > On Wed, 18 Oct 2017 21:24:25 +0200
> > SF Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > From: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > > Date: Wed, 18 Oct 2017 19:14:39 +0200
> > > 
> > > The variable "table_group" will be set to an appropriate pointer.
> > > Thus omit the explicit initialisation at the beginning.
> > > 
> > > Signed-off-by: Markus Elfring <elfring@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >  arch/powerpc/platforms/pseries/iommu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/powerpc/platforms/pseries/iommu.c
> > > b/arch/powerpc/platforms/pseries/iommu.c index
> > > b37d4fb20d1c..b6c12b8e3ace 100644 ---
> > > a/arch/powerpc/platforms/pseries/iommu.c +++
> > > b/arch/powerpc/platforms/pseries/iommu.c @@ -55,7 +55,7 @@
> > >  
> > >  static struct iommu_table_group *iommu_pseries_alloc_group(int
> > > node) {
> > > -	struct iommu_table_group *table_group = NULL;
> > > +	struct iommu_table_group *table_group;
> > >  	struct iommu_table *tbl = NULL;
> > >  	struct iommu_table_group_link *tgl = NULL;
> > >    
> > 
> > I think initializing pointers to NULL is generally a good idea.
> > 
> > If there is no use of the variable before it is reinitialized by
> > allocation gcc is free to optimize out the variable and its initial
> > value.
> > 
> > On the other hand, if the code is changed later and use of the
> > variable becomes possible you may crash (and get a gcc warning,
> > too).  
> 
> No, it's the opposite. GCC doesn't warn about potential NULL
> dereferences, 

However, kernel produces runtime errors on actual NULL dereference. So
you will learn about the issue quite fast.

> it warns about uninitialized variables.  By initializing
> it to a bogus value, you're deliberately disabling static analysis.
> We do see bugs where, if only people didn't initialize stuff to bogus
> values, then the bug would have been caught before it was merged.

There are recently merged changes that cause new warnings as well.

How that can be? Perhaps #ifdefs depending on kernel configuration and
0-day not testing all possible combinations? Perhaps the compiler
getting confused by the code and only later compiler update finding the
issue?

Whatever the reason you cannot rely on compiler warnings to correct
your code. They may help you to point out issues but writing the code
in such a way that the issues are less likely to happen in the first
place is better than fixing warnings after the fact.

> 
> You might imagine that static analysis tools would catch NULL
> dereferences but it's actually really really hard.  We used to have
> an __uninitialized_var() macro which was used to silence GCC false
> positives, but now we initialize the pointers to NULL instead.  So
> most of the code that you're dealing with is stuff that was marked as
> too hard for GCC to understand.  It's tricky.
> 

Then it should be made easy to understand and maintain for humans since
the compilers have failed at maintaining it for us.

Thanks

Michal