[PATCHv2] sysdeps/ieee754: prevent maybe-uninitialized errors [BZ #19444]

Message ID 20180929104607.8121-1-Martin.Jansa@gmail.com
State New
Headers show
Series
  • [PATCHv2] sysdeps/ieee754: prevent maybe-uninitialized errors [BZ #19444]
Related show

Commit Message

Martin Jansa Sept. 29, 2018, 10:46 a.m.
With -O included in CFLAGS it fails to build with:

../sysdeps/ieee754/ldbl-96/e_jnl.c: In function '__ieee754_jnl':
../sysdeps/ieee754/ldbl-96/e_jnl.c:146:20: error: 'temp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
      b = invsqrtpi * temp / sqrtl (x);
          ~~~~~~~~~~^~~~~~
../sysdeps/ieee754/ldbl-96/e_jnl.c: In function '__ieee754_ynl':
../sysdeps/ieee754/ldbl-96/e_jnl.c:375:16: error: 'temp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  b = invsqrtpi * temp / sqrtl (x);
      ~~~~~~~~~~^~~~~~

        [BZ #23716]
        * sysdeps/ieee754/ldbl-96/e_jnl.c: Fix build with -O

Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
---
 ChangeLog                       |  4 ++++
 sysdeps/ieee754/ldbl-96/e_jnl.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+)

Comments

Joseph Myers Sept. 30, 2018, 1:25 p.m. | #1
On Sat, 29 Sep 2018, Martin Jansa wrote:

> +/* With GCC 8 when compiling with -O the compiler
> +   warns that the variable 'temp',  may be used uninitialized.
> +   The switch above should cover all possible values of n & 3
> +   so I belive it's false possitive. */
> +DIAG_PUSH_NEEDS_COMMENT;
> +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
>  	    b = invsqrtpi * temp / sqrtl (x);
> +DIAG_POP_NEEDS_COMMENT;

DIAG_* and associated comments should be indented to match the associated 
code rather than at the left margin.  Any alphabetic characters at the 
left margin in the middle of a function are a bad idea because they break 
things trying to determine the function name for diff context etc.

As per <https://sourceware.org/ml/libc-alpha/2018-09/msg00311.html> I 
think it would be best to add DIAG_IGNORE_O1_NEEDS_COMMENT to libc-diag.h, 
and use it for cases such as here that only need the diagnostics disabled 
for -O1.

glibc code is written and maintained by many people over time, so there 
shouldn't be an "I" in comments; they should reflect a current collective 
understanding rather than one person's view, and read as such.  Also, the 
point isn't that the switch *should* cover all possible value of n & 3, 
it's that it *does* cover all possible values (and sets temp in every 
case) and so the warning *is* a false positive.
Martin Jansa Sept. 30, 2018, 1:50 p.m. | #2
On Sun, Sep 30, 2018 at 01:25:08PM +0000, Joseph Myers wrote:
> On Sat, 29 Sep 2018, Martin Jansa wrote:
> 
> > +/* With GCC 8 when compiling with -O the compiler
> > +   warns that the variable 'temp',  may be used uninitialized.
> > +   The switch above should cover all possible values of n & 3
> > +   so I belive it's false possitive. */
> > +DIAG_PUSH_NEEDS_COMMENT;
> > +DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
> >  	    b = invsqrtpi * temp / sqrtl (x);
> > +DIAG_POP_NEEDS_COMMENT;
> 
> DIAG_* and associated comments should be indented to match the associated 
> code rather than at the left margin.  Any alphabetic characters at the 
> left margin in the middle of a function are a bad idea because they break 
> things trying to determine the function name for diff context etc.

OK, will update in v3.

> As per <https://sourceware.org/ml/libc-alpha/2018-09/msg00311.html> I 
> think it would be best to add DIAG_IGNORE_O1_NEEDS_COMMENT to libc-diag.h, 
> and use it for cases such as here that only need the diagnostics disabled 
> for -O1.

How should I distinguish between -O1 and -O2? For -Os it's simple,
because there is __OPTIMIZE_SIZE__ defined only when -Os, but for -O1 as
well as -O2 and -O3 there is __OPTIMIZE__, but I don't know how to find
out which level is being used. And it's still not completely accurate
-O1 + -ftree-vrp should be working fine and -O2 + -fno-tree-vrp will
probably trigger the maybe-uninitialized warning. And I don't see any
macro defined only when VRP is enabled in gcc.

> glibc code is written and maintained by many people over time, so there 
> shouldn't be an "I" in comments; they should reflect a current collective 
> understanding rather than one person's view, and read as such.  Also, the 
> point isn't that the switch *should* cover all possible value of n & 3, 
> it's that it *does* cover all possible values (and sets temp in every 
> case) and so the warning *is* a false positive.

OK, I've already improved it a bit in local v3 (still waiting for build tests
across 7 architectures x 3 optimization levels), I've also one more fix
for aarch64 when -Os is being used.

Thanks
Joseph Myers Sept. 30, 2018, 4:49 p.m. | #3
On Sun, 30 Sep 2018, Martin Jansa wrote:

> > As per <https://sourceware.org/ml/libc-alpha/2018-09/msg00311.html> I 
> > think it would be best to add DIAG_IGNORE_O1_NEEDS_COMMENT to libc-diag.h, 
> > and use it for cases such as here that only need the diagnostics disabled 
> > for -O1.
> 
> How should I distinguish between -O1 and -O2? For -Os it's simple,
> because there is __OPTIMIZE_SIZE__ defined only when -Os, but for -O1 as
> well as -O2 and -O3 there is __OPTIMIZE__, but I don't know how to find
> out which level is being used. And it's still not completely accurate
> -O1 + -ftree-vrp should be working fine and -O2 + -fno-tree-vrp will
> probably trigger the maybe-uninitialized warning. And I don't see any
> macro defined only when VRP is enabled in gcc.

It looks like that would require a configure test, so maybe a separate 
macro there is overkill.  Does anyone else have thoughts on this question 
of how narrowly to disable warnings for cases that only warn for -O1?

Patch

diff --git a/ChangeLog b/ChangeLog
index 07760299e6..5066a4840d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@ 
+2018-09-29  Martin Jansa  <Martin.Jansa@gmail.com>
+	Partial fix for [BZ #23716]
+	* sysdeps/ieee754/ldbl-96/e_jnl.c: Fix build with -O
+
 2018-09-28  Joseph Myers  <joseph@codesourcery.com>
 
 	* math/fromfp.h: Do not include <math_private.h>.
diff --git a/sysdeps/ieee754/ldbl-96/e_jnl.c b/sysdeps/ieee754/ldbl-96/e_jnl.c
index 855190841b..c5b27e7fcf 100644
--- a/sysdeps/ieee754/ldbl-96/e_jnl.c
+++ b/sysdeps/ieee754/ldbl-96/e_jnl.c
@@ -62,6 +62,7 @@ 
 #include <math_private.h>
 #include <fenv_private.h>
 #include <math-underflow.h>
+#include <libc-diag.h>
 
 static const long double
   invsqrtpi = 5.64189583547756286948079e-1L, two = 2.0e0L, one = 1.0e0L;
@@ -144,7 +145,14 @@  __ieee754_jnl (int n, long double x)
 		temp = c - s;
 		break;
 	      }
+/* With GCC 8 when compiling with -O the compiler
+   warns that the variable 'temp',  may be used uninitialized.
+   The switch above should cover all possible values of n & 3
+   so I belive it's false possitive. */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
 	    b = invsqrtpi * temp / sqrtl (x);
+DIAG_POP_NEEDS_COMMENT;
 	  }
 	else
 	  {
@@ -373,7 +381,14 @@  __ieee754_ynl (int n, long double x)
 	    temp = s + c;
 	    break;
 	  }
+/* With GCC 8 when compiling with -O the compiler
+   warns that the variable 'temp',  may be used uninitialized.
+   The switch above should cover all possible values of n & 3
+   so I belive it's false possitive. */
+DIAG_PUSH_NEEDS_COMMENT;
+DIAG_IGNORE_NEEDS_COMMENT (8, "-Wmaybe-uninitialized");
 	b = invsqrtpi * temp / sqrtl (x);
+DIAG_POP_NEEDS_COMMENT;
       }
     else
       {