R devel - Sep 2015 - Optimization bug when byte compiling with gcc 5.2.0 on windows

On 14/09/2015 9:36 AM, luke-tierney at uiowa.edu wrote:
> I believe the issue is that on Windows the sqrt function when called
> with a NaN does not return the same NaN, as it does on other platforms.
> We have
>
> #if (defined(_WIN32) || defined(_WIN64)) && defined(__GNUC__)
&& \
>       __GNUC__ <= 4
> # define R_sqrt(x) (ISNAN(x) ? x : sqrt(x))
> #else
> # define R_sqrt sqrt
> #endif
>
> for implementing the SQRT opcode. I suspect this came from Duncan
> Murdoch; I don't know the reason for restricting to __GNUC__ <= 4.
That was an update to keep the workaround for gcc 4.9.2.  The previous 
code restricted the change to 4.6.x or earlier.

I think the reason for the earlier restriction is the assumption that 
eventually gcc will be fixed and this workaround won't be necessary, but 
apparently 5.2.0 still has the same problem.

Duncan Murdoch
>   I
> seem to recall that there are other places in the code where we have
> similar workarounds but I can't find them at the moment.
>
> The builtin sqrt always pretests with ISNAN.
>
> Best,
>
> luke
>
>
>
> On Mon, 14 Sep 2015, Jeroen Ooms wrote:
>
> > When building R-devel with gcc 5.2.0 (mingw-w64 v4) on Windows, make
> > check fails reg-tests-1b.R at the following check:
> >
> >  x <- c(1:2, NA)
> >  sx <- sd(x)
> >  !is.nan(sx)
> >
> > Here 'sx' should be 'NA' but it is 'NaN'. It
turns out this problem
> > only appears when the function is byte compiled with optimization
> > level 3:
> >
> >  mysd <- function (x, na.rm = FALSE)
> >    sqrt(var(if (is.vector(x)) x else as.double(x), na.rm = na.rm))
> >
> >  mysd(x)
> >  # [1] NA
> >  compiler::cmpfun(mysd, list(optimize = 1L))(x)
> >  # [1] NA
> >  compiler::cmpfun(mysd, list(optimize = 2L))(x)
> >  # [1] NA
> >  compiler::cmpfun(mysd, list(optimize = 3L))(x)
> >  # [1] NaN
> >
> > The problem does not appear with gcc 5.2.0 on Debian, and also not
> > with gcc 4.9.3 on Windows. Where do I start debugging this? The
> > disassembled output from the compiled functions is here:
> > https://gist.github.com/jeroenooms/3206945a6db6680a9c5c
> >
> > ______________________________________________
> > R-devel at r-project.org mailing list
> > https://stat.ethz.ch/mailman/listinfo/r-devel
> >
>

On Mon, Sep 14, 2015 at 4:16 PM, Duncan Murdoch
<murdoch.duncan at gmail.com> wrote:
> I think the reason for the earlier restriction is the assumption that
> eventually gcc will be fixed and this workaround won't be necessary,
but
> apparently 5.2.0 still has the same problem.
I tested this hypothesis by replacing '__GNUC__ <= 4' with
'__GNUC__
<= 5' and rebuilding R, but this introduces a whole lot of problems
that were not there before. In particular R crashes when using the
graphics device, which it did not without this fix.

I first thought this was an unrelated random crash, but 'make check'
consistently fails in base-Ex.R at the first invocation of boxplot().
Same for 'check recommended', each package fails when making a plot.
None of this happens with the original source code, where everything
works fine, besides the optimized version of sqrt(NA) giving NaN. Did
something perhaps change in gcc 5 for the ISNAN macro that makes it
unsuitable to be used in this way?

FWIW it looks like the original fix was introduced by Luke about a
year ago in r66652:
https://github.com/wch/r-source/commit/e9aaf8fdeddf27c2a9078cd214a41475c8ff6f40

On Mon, Sep 14, 2015 at 8:49 PM, Jeroen Ooms <jeroen.ooms at
stat.ucla.edu> wrote:
> I tested this hypothesis by replacing '__GNUC__ <= 4' with
'__GNUC__
> <= 5' and rebuilding R, but this introduces a whole lot of problems
> that were not there before. In particular R crashes when using the
> graphics device, which it did not without this fix.
I found that the problem goes away when we use isnanl() instead of
isnan(). This is very similar to the problem we have for R_pow() where
we need to use powl() instead of pow() for recent versions mingw-w64
(this still needs to be patched in r-devel).

So the full solution is:

#if (defined(_WIN32) || defined(_WIN64)) && defined(__GNUC__)
# define R_sqrt(x) (isnanl(x) ? x : sqrt(x))
#else
# define R_sqrt sqrt
#endif

Below the relevant mingw-w64 sources:

https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-crt/math/isnan.c
https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-crt/math/isnanf.c
https://github.com/Alexpux/mingw-w64/blob/master/mingw-w64-crt/math/isnanl.c

I think mingw-w64 takes powl from glibc.