Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Win32] Inclusion of '-std=c99' flag in 32-bit builds of perl leads to anomalies in XS arithmetic. #22199

Closed
sisyphus opened this issue May 8, 2024 · 4 comments

Comments

@sisyphus
Copy link
Contributor

sisyphus commented May 8, 2024

4 things to note:

  1. that 64-bit builds are unaffected;
  2. that pure perl code is unaffected - the issue is evident only in XS code;
  3. that the issue begins with release 5.39.7, which was the first release in which win32/GNUmakefile added the -std=c99 flag to $Config{ccflags};
  4. that I don't regard this as a release blocker (but only because it affects only 32-bit perls).

My suggested fix is to have win32/GNUmakefile add the -ffast-math flag to perl's $Config{ccflags}.
See information provide by Liu Hao at:
https://sourceforge.net/p/mingw-w64/mailman/mingw-w64-public/thread/CADZSBj04E%2BYaCYDiKRSX6gqNEqLW9u99T2zCFszPeLZbhDjCCw%40mail.gmail.com/#msg58769159

Does that suggested fix seem reasonable ?
I envisage that the addition of the -ffast-math flag can be made to both 64-bit and 32-bit builds ..... or would we want to limit it to the 32-bit build only ?

Description

Inside XS code, a decimal literal floating point value will (for many values) be considered unequal to the same value expressed as a hex string literal.

Steps to Reproduce

A simple C demo program to build (with/without -std=c99/-ffast-math):

#include <stdio.h>

int main(void) {
  if(1.3999999999999999 == 0x1.6666666666666p+0) printf("OK1\n");
  else printf("%a\n%a\n", 0x1.6666666666666p+0, 1.3999999999999999);
  return 0;
}

An Inline::C script:

use strict;
use warnings;
use Config;

die "Wrong OS and/or architecture" unless $Config{archname} =~ /MSWin32\-x86/;
die "Wrong perl version ($])" unless $] >5.039006;

use Inline C => Config =>
  BUILD_NOISY => 1,
  USING => 'ParseRegExp',
  FORCE_BUILD => 1,
 # CCFLAGSEX => ' -ffast-math',
;

use Inline C =><<'EOC';
void foo(void) {
  if(1.7976931348623157e+308 == 0x1.fffffffffffffp+1023) printf("OK2\n");
  else printf("%a\n%a\n\n", 1.7976931348623157e+308, 0x1.fffffffffffffp+1023);

  if(1.4142135623730951 == 0x1.6a09e667f3bcdp+0) printf("OK3\n");
  else printf("%a\n%a\n\n", 1.4142135623730951, 0x1.6a09e667f3bcdp+0);

  if(1.3999999999999999 == 0x1.6666666666666p+0) printf("OK4\n");
  else printf("%a\n%a\n\n", 1.3999999999999999, 0x1.6666666666666p+0);
}

EOC
print "OK1\n" if 1.7976931348623157e+308 == 0x1.fffffffffffffp+1023
              && 1.4142135623730951      == 0x1.6a09e667f3bcdp+0
              && 1.3999999999999999      == 0x1.6666666666666p+0;
foo();

Expected behavior

Expected output of the Inline::C script:

OK1
OK2
OK3
OK4

Actual output of the Inline::C script:

OK1
0x1.fffffffffffffp+1023
0x1.fffffffffffffp+1023

0x1.6a09e667f3bcdp+0
0x1.6a09e667f3bcdp+0

0x1.6666666666666p+0
0x1.6666666666666p+0

(Uncomment the "CCFLAGSEX..." line in the Inline::C script to obtain expected output.)

Perl configuration

Summary of my perl5 (revision 5 version 39 subversion 10) configuration:

  Platform:
    osname=MSWin32
    osvers=10.0.22631.3447
    archname=MSWin32-x86-multi-thread
    uname=''
    config_args='undef'
    hint=recommended
    useposix=true
    d_sigaction=undef
    useithreads=define
    usemultiplicity=define
    use64bitint=undef
    use64bitall=undef
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='gcc'
    ccflags ='-std=c99 -DWIN32 -fdiagnostics-color=never -DPERL_TEXTMODE_SCRIPTS -DMULTIPLICITY -DPERL_IMPLICIT_SYS -DUSE_PERLIO -D__USE_MINGW_ANSI_STDIO -fwrapv -fno-strict-aliasing -mms-bitfields'
    optimize='-O2'
    cppflags='-DWIN32'
    ccversion=''
    gccversion='13.2.0'
    gccosandvers=''
    intsize=4
    longsize=4
    ptrsize=4
    doublesize=8
    byteorder=1234
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=12
    longdblkind=3
    ivtype='long'
    ivsize=4
    nvtype='double'
    nvsize=8
    Off_t='long long'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='g++'
    ldflags ='-s -L"D:\perl-5.39.10-1320-ucrt-mcf\lib\MSWin32-x86-multi-thread\CORE" -L"C:\winlibs-ucrt-1320\mingw32\lib" -L"C:\winlibs-ucrt-1320\mingw32\i686-w64-mingw32\lib" -L"C:\winlibs-ucrt-1320\mingw32\lib\gcc\i686-w64-mingw32\13.2.0"'
    libpth=C:\winlibs-ucrt-1320\mingw32\lib C:\winlibs-ucrt-1320\mingw32\i686-w64-mingw32\lib C:\winlibs-ucrt-1320\mingw32\lib\gcc\i686-w64-mingw32\13.2.0 D:\_32\msys_1320\1.0\local\lib
    libs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    perllibs= -lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
    libc=-lucrt
    so=dll
    useshrplib=true
    libperl=libperl539.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_win32.xs
    dlext=dll
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags=' '
    lddlflags='-shared -s -L"D:\perl-5.39.10-1320-ucrt-mcf\lib\MSWin32-x86-multi-thread\CORE" -L"C:\winlibs-ucrt-1320\mingw32\lib" -L"C:\winlibs-ucrt-1320\mingw32\i686-w64-mingw32\lib" -L"C:\winlibs-ucrt-1320\mingw32\lib\gcc\i686-w64-mingw32\13.2.0"'


Characteristics of this binary (from libperl):
  Compile-time options:
    HAS_LONG_DOUBLE
    HAS_TIMES
    HAVE_INTERP_INTERN
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_HASH_FUNC_ZAPHOD32
    PERL_HASH_USE_SBOX32
    PERL_IMPLICIT_SYS
    PERL_MALLOC_WRAP
    PERL_OP_PARENT
    PERL_PRESERVE_IVUV
    PERL_USE_SAFE_PUTENV
    USE_ITHREADS
    USE_LARGE_FILES
    USE_LOCALE
    USE_LOCALE_COLLATE
    USE_LOCALE_CTYPE
    USE_LOCALE_NUMERIC
    USE_LOCALE_TIME
    USE_PERLIO
    USE_PERL_ATOF
    USE_THREAD_SAFE_LOCALE
  Built under MSWin32
  Compiled at Apr 28 2024 13:28:40
  @INC:
    D:/perl-5.39.10-1320-ucrt-mcf/site/lib/MSWin32-x86-multi-thread
    D:/perl-5.39.10-1320-ucrt-mcf/site/lib
    D:/perl-5.39.10-1320-ucrt-mcf/lib/MSWin32-x86-multi-thread
    D:/perl-5.39.10-1320-ucrt-mcf/lib
@sisyphus
Copy link
Contributor Author

sisyphus commented May 13, 2024

Although adding -ffast-math to perl's ccflags fixes the above issue, it creates another one in that the condition ($nan == 0) (where $nan is a NaN) evaluates as true.
Adding -fexcess-precision=fast instead of -ffast-math to $Config{ccflags} fixes the original problem, without causing this second issue.

@tonycoz
Copy link
Contributor

tonycoz commented May 16, 2024

After looking over this...

First, I think your example is non-portable, the decimal value isn't precisely equal to the hex version, and the standard allows the compiler to take advantage of any extra precision it has access to, from c99 6.3.8.1 paragraph 2:

The values of floating operands and of the results of floating expressions may be represented in greater precision and range than that required by the type; the types are not changed thereby.

I suspect this is why the compiler treats the comparison as false under -std=c99. Adding a (double) cast to the decimal side of the comparison make it true, since now both sides have some (and the same) exact representation.

Note in both cases, with or without -std=c99, the optimizer optimized away the comparison, even with -O0.

We could remove the -std=c99, or make it -std=gnu99, or either of those and hide the std flag from ccflags, but it won't make your code portable.

Adding -ffast-math would be wrong.

@sisyphus
Copy link
Contributor Author

I sense that we might be able to classify the problematic C code as badly written code - or, perhaps we could argue that the assumption that the equivalence will be true is without basis.

That would mean that we can just close this Issue and do nothing else (because there really is nothing else to do).
I'm amenable to that, if you think that would be a reasonable response.

As you point out, it's not hard to alter the code so that it DWIMs.

@tonycoz
Copy link
Contributor

tonycoz commented May 20, 2024

I think it's reasonable to close it.

@tonycoz tonycoz closed this as completed May 20, 2024
sisyphus pushed a commit to sisyphus/math-mpfr that referenced this issue May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants