Message ID | 20180929104607.8121-1-Martin.Jansa@gmail.com |
---|---|
State | New |
Headers | show |
Series | [PATCHv2] sysdeps/ieee754: prevent maybe-uninitialized errors [BZ #19444] | expand |
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?
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 {
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(+)