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

  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(+)
  

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?