| Message ID | 9c71e3e0-8998-fd02-e3a3-ef219d82ee32@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | PowerPC-pSeries: Adjustments for seven function implementations | expand |
Hello, On Wed, 18 Oct 2017 21:24:25 +0200 SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > 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@users.sourceforge.net> > --- > 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). Removing these initializers adds no value, to the contrary. Thanks Michal
>> 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. This one would also not be needed if the call of the function “kzalloc_node” could be specified in the same statement. > Removing these initializers adds no value, to the contrary. This small update step is just a “preparation” for the subsequent two suggestions in this patch series. Regards, Markus
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@users.sourceforge.net> wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > 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@users.sourceforge.net> > > --- > > 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, 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. 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. regards, dan carpenter
On Thu, 19 Oct 2017 15:55:59 +0300 Dan Carpenter <dan.carpenter@oracle.com> 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@users.sourceforge.net> wrote: > > > > > From: Markus Elfring <elfring@users.sourceforge.net> > > > 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@users.sourceforge.net> > > > --- > > > 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
On Thu, Oct 19, 2017 at 03:55:59PM +0300, Dan Carpenter 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@users.sourceforge.net> wrote: > > > > > From: Markus Elfring <elfring@users.sourceforge.net> > > > 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@users.sourceforge.net> > > > --- > > > 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, 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. Seconded, I've seen this a number of times. I think this alone is a reason not to initiaize locals if they don't require it. > 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. > > regards, > dan carpenter > >
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;