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

[Martin Jansa <martin dot jansa at gmail dot com>]

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

-- 
Martin 'JaMa' Jansa     jabber: Martin.Jansa@gmail.com

Attachment: signature.asc
Description: Digital signature