inet/net-internal.h: Fix uninitalised clntudp_call() variable

Message ID 20190916221536.18500-1-alistair.francis@wdc.com
State New, archived
Headers

Commit Message

Alistair Francis Sept. 16, 2019, 10:15 p.m. UTC
  The total_deadline variable inside the clntudp_call() function inside
sunrpc/clnt_udp.c can cause uninitalised variable warnings when building
with GCC 8.3 or 9.2 on a platform with a 64-bit tv_nsec on a 32-bit
architecture. To fix the warning let's use the DIAG_* macros to hide the
warning.

A GCC bug case has also been submitted:
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691

2019-09-16  Alistair Francis  <alistair.francis@wdc.com>

	* inet/net-internal.h: Fix uninitalised clntudp_call() variable
---
 inet/net-internal.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)
  

Comments

Alistair Francis Sept. 24, 2019, 5:04 p.m. UTC | #1
On Mon, Sep 16, 2019 at 3:19 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> The total_deadline variable inside the clntudp_call() function inside
> sunrpc/clnt_udp.c can cause uninitalised variable warnings when building
> with GCC 8.3 or 9.2 on a platform with a 64-bit tv_nsec on a 32-bit
> architecture. To fix the warning let's use the DIAG_* macros to hide the
> warning.
>
> A GCC bug case has also been submitted:
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
>
> 2019-09-16  Alistair Francis  <alistair.francis@wdc.com>
>
>         * inet/net-internal.h: Fix uninitalised clntudp_call() variable

Ping! Is this patch ok to go in?

Alistair

> ---
>  inet/net-internal.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/inet/net-internal.h b/inet/net-internal.h
> index 2f522eef555..c774de2b78a 100644
> --- a/inet/net-internal.h
> +++ b/inet/net-internal.h
> @@ -23,6 +23,7 @@
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <sys/time.h>
> +#include <libc-diag.h>
>
>  int __inet6_scopeid_pton (const struct in6_addr *address,
>                            const char *scope, uint32_t *result);
> @@ -96,6 +97,16 @@ __deadline_is_infinite (struct deadline deadline)
>    return deadline.absolute.tv_nsec < 0;
>  }
>
> +/* GCC 8.3 and 9.2 both incorrectly report total_deadline
> + * (from sunrpc/clnt_udp.c) as maybe-uninitialized when tv_sec is 8 bytes
> + * (64-bits) wide on 32-bit systems. We have to set -Wmaybe-uninitialized
> + * here as it won't fix the error in sunrpc/clnt_udp.c.
> + * A GCC bug has been filed here:
> + *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
> + */
> +DIAG_PUSH_NEEDS_COMMENT;
> +DIAG_IGNORE_NEEDS_COMMENT (9, "-Wmaybe-uninitialized");
> +
>  /* Return true if the current time is at the deadline or past it.  */
>  static inline bool
>  __deadline_elapsed (struct deadline_current_time current,
> @@ -120,6 +131,8 @@ __deadline_first (struct deadline left, struct deadline right)
>      return right;
>  }
>
> +DIAG_POP_NEEDS_COMMENT;
> +
>  /* Add TV to the current time and return it.  Returns a special
>     infinite absolute deadline on overflow.  */
>  struct deadline __deadline_from_timeval (struct deadline_current_time,
> --
> 2.23.0
>
  
Carlos O'Donell Sept. 24, 2019, 8:21 p.m. UTC | #2
On 9/16/19 6:15 PM, Alistair Francis wrote:
> The total_deadline variable inside the clntudp_call() function inside
> sunrpc/clnt_udp.c can cause uninitalised variable warnings when building
> with GCC 8.3 or 9.2 on a platform with a 64-bit tv_nsec on a 32-bit
> architecture. To fix the warning let's use the DIAG_* macros to hide the
> warning.
> 
> A GCC bug case has also been submitted:
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
> 

This looks good to me. If you're seeing this using the released 8.3 or 9.2
then others will see it too. You also reference the compiler version that
is the latest with the problem e.g. 9. I don't think we should fix this
by zeroing out the entire field since this looks like an optimization
or compiler related issue.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 2019-09-16  Alistair Francis  <alistair.francis@wdc.com>
> 
> 	* inet/net-internal.h: Fix uninitalised clntudp_call() variable
> ---
>  inet/net-internal.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/inet/net-internal.h b/inet/net-internal.h
> index 2f522eef555..c774de2b78a 100644
> --- a/inet/net-internal.h
> +++ b/inet/net-internal.h
> @@ -23,6 +23,7 @@
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <sys/time.h>
> +#include <libc-diag.h>
>  
>  int __inet6_scopeid_pton (const struct in6_addr *address,
>                            const char *scope, uint32_t *result);
> @@ -96,6 +97,16 @@ __deadline_is_infinite (struct deadline deadline)
>    return deadline.absolute.tv_nsec < 0;
>  }
>  
> +/* GCC 8.3 and 9.2 both incorrectly report total_deadline
> + * (from sunrpc/clnt_udp.c) as maybe-uninitialized when tv_sec is 8 bytes
> + * (64-bits) wide on 32-bit systems. We have to set -Wmaybe-uninitialized
> + * here as it won't fix the error in sunrpc/clnt_udp.c.
> + * A GCC bug has been filed here:
> + *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
> + */
> +DIAG_PUSH_NEEDS_COMMENT;
> +DIAG_IGNORE_NEEDS_COMMENT (9, "-Wmaybe-uninitialized");
> +
>  /* Return true if the current time is at the deadline or past it.  */
>  static inline bool
>  __deadline_elapsed (struct deadline_current_time current,
> @@ -120,6 +131,8 @@ __deadline_first (struct deadline left, struct deadline right)
>      return right;
>  }
>  
> +DIAG_POP_NEEDS_COMMENT;
> +
>  /* Add TV to the current time and return it.  Returns a special
>     infinite absolute deadline on overflow.  */
>  struct deadline __deadline_from_timeval (struct deadline_current_time,
>
  
Alistair Francis Sept. 24, 2019, 8:30 p.m. UTC | #3
On Tue, Sep 24, 2019 at 1:21 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 9/16/19 6:15 PM, Alistair Francis wrote:
> > The total_deadline variable inside the clntudp_call() function inside
> > sunrpc/clnt_udp.c can cause uninitalised variable warnings when building
> > with GCC 8.3 or 9.2 on a platform with a 64-bit tv_nsec on a 32-bit
> > architecture. To fix the warning let's use the DIAG_* macros to hide the
> > warning.
> >
> > A GCC bug case has also been submitted:
> >     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
> >
>
> This looks good to me. If you're seeing this using the released 8.3 or 9.2
> then others will see it too. You also reference the compiler version that
> is the latest with the problem e.g. 9. I don't think we should fix this
> by zeroing out the entire field since this looks like an optimization
> or compiler related issue.
>
> OK for master.

Thanks!

I don't have commit access yet, I'm going to start the process of
requesting it. I named you as the person to approve my account
creation Carlos.

While that is happening if anyone else would like to apply this patch
please feel free to :)

Alistair

>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > 2019-09-16  Alistair Francis  <alistair.francis@wdc.com>
> >
> >       * inet/net-internal.h: Fix uninitalised clntudp_call() variable
> > ---
> >  inet/net-internal.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/inet/net-internal.h b/inet/net-internal.h
> > index 2f522eef555..c774de2b78a 100644
> > --- a/inet/net-internal.h
> > +++ b/inet/net-internal.h
> > @@ -23,6 +23,7 @@
> >  #include <stdbool.h>
> >  #include <stdint.h>
> >  #include <sys/time.h>
> > +#include <libc-diag.h>
> >
> >  int __inet6_scopeid_pton (const struct in6_addr *address,
> >                            const char *scope, uint32_t *result);
> > @@ -96,6 +97,16 @@ __deadline_is_infinite (struct deadline deadline)
> >    return deadline.absolute.tv_nsec < 0;
> >  }
> >
> > +/* GCC 8.3 and 9.2 both incorrectly report total_deadline
> > + * (from sunrpc/clnt_udp.c) as maybe-uninitialized when tv_sec is 8 bytes
> > + * (64-bits) wide on 32-bit systems. We have to set -Wmaybe-uninitialized
> > + * here as it won't fix the error in sunrpc/clnt_udp.c.
> > + * A GCC bug has been filed here:
> > + *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
> > + */
> > +DIAG_PUSH_NEEDS_COMMENT;
> > +DIAG_IGNORE_NEEDS_COMMENT (9, "-Wmaybe-uninitialized");
> > +
> >  /* Return true if the current time is at the deadline or past it.  */
> >  static inline bool
> >  __deadline_elapsed (struct deadline_current_time current,
> > @@ -120,6 +131,8 @@ __deadline_first (struct deadline left, struct deadline right)
> >      return right;
> >  }
> >
> > +DIAG_POP_NEEDS_COMMENT;
> > +
> >  /* Add TV to the current time and return it.  Returns a special
> >     infinite absolute deadline on overflow.  */
> >  struct deadline __deadline_from_timeval (struct deadline_current_time,
> >
>
>
> --
> Cheers,
> Carlos.
  
Maciej W. Rozycki Sept. 24, 2019, 9 p.m. UTC | #4
On Tue, 24 Sep 2019, Alistair Francis wrote:

> > This looks good to me. If you're seeing this using the released 8.3 or 9.2
> > then others will see it too. You also reference the compiler version that
> > is the latest with the problem e.g. 9. I don't think we should fix this
> > by zeroing out the entire field since this looks like an optimization
> > or compiler related issue.
> >
> > OK for master.
> 
> Thanks!
> 
> I don't have commit access yet, I'm going to start the process of
> requesting it. I named you as the person to approve my account
> creation Carlos.
> 
> While that is happening if anyone else would like to apply this patch
> please feel free to :)

 I have committed it on your behalf, with a couple of minor formatting 
nits fixed in the commit message (double space after a full stop) and the 
ChangeLog entry (full stop at the end of a sentence).  Thanks for your 
contribution.

  Maciej
  

Patch

diff --git a/inet/net-internal.h b/inet/net-internal.h
index 2f522eef555..c774de2b78a 100644
--- a/inet/net-internal.h
+++ b/inet/net-internal.h
@@ -23,6 +23,7 @@ 
 #include <stdbool.h>
 #include <stdint.h>
 #include <sys/time.h>
+#include <libc-diag.h>
 
 int __inet6_scopeid_pton (const struct in6_addr *address,
                           const char *scope, uint32_t *result);
@@ -96,6 +97,16 @@  __deadline_is_infinite (struct deadline deadline)
   return deadline.absolute.tv_nsec < 0;
 }
 
+/* GCC 8.3 and 9.2 both incorrectly report total_deadline
+ * (from sunrpc/clnt_udp.c) as maybe-uninitialized when tv_sec is 8 bytes
+ * (64-bits) wide on 32-bit systems. We have to set -Wmaybe-uninitialized
+ * here as it won't fix the error in sunrpc/clnt_udp.c.
+ * A GCC bug has been filed here:
+ *    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91691
+ */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_NEEDS_COMMENT (9, "-Wmaybe-uninitialized");
+
 /* Return true if the current time is at the deadline or past it.  */
 static inline bool
 __deadline_elapsed (struct deadline_current_time current,
@@ -120,6 +131,8 @@  __deadline_first (struct deadline left, struct deadline right)
     return right;
 }
 
+DIAG_POP_NEEDS_COMMENT;
+
 /* Add TV to the current time and return it.  Returns a special
    infinite absolute deadline on overflow.  */
 struct deadline __deadline_from_timeval (struct deadline_current_time,