[PATCHv2] sysdeps/ieee754: prevent maybe-uninitialized errors [BZ #19444]
Commit Message
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
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.
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
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?
@@ -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>.
@@ -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
{