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

POSIX shouldn't test signbit(NAN) #21533

Closed
rkennedy opened this issue Sep 28, 2023 · 4 comments · Fixed by #21610
Closed

POSIX shouldn't test signbit(NAN) #21533

rkennedy opened this issue Sep 28, 2023 · 4 comments · Fixed by #21610

Comments

@rkennedy
Copy link

Module: POSIX

Description
ext/POSIX/t/math.t includes an assertion about signbit(NAN), and it fails while I'm trying to build v5.38.0:

ext/POSIX/t/math ................................................. #   Failed test 'signbit(NAN)'
#   at t/math.t line 296.
# Looks like you failed 1 test of 151.

It's this assertion:

ok(!signbit(NAN), "signbit(NAN)");

The assertion fails on my Solaris x86 system. The underlying C signbit function exhibits the same behavior.

#include <stdio.h>
#include <math.h>
int main(void) {
    printf("NaN sign bit: %d\n", signbit(NAN));
    return 0;
}

That prints NaN sign bit: 1. (On my Solaris Sparc system, and on my RHEL x86_64 host, it reports "0" instead, and Perl's signbit test passes.)

Steps to Reproduce

./Configure -Dcc=/bin/cc -des -Dusethreads -Duseshrplib=false -Dnoextensions=ODBM_File
make
make test TEST_FILES=../ext/POSIX/t/math.t

Expected behavior
This particular assertion shouldn't be present. POSIX does not specify the precise value of NAN. Although NAN values may have sign bits, they do not have signs, per se, so it's wrong to assert that the value of NAN's sign bit should have any particular value.

This is somewhat related to issue #18473, and especially to PR #18590, where the failing assertion was added.

Perl configuration

Summary of my perl5 (revision 5 version 38 subversion 0) configuration:
  Commit id: 76298ae68aa7796f0ffc05095b127d23f4b2de8f
  Platform:
    osname=solaris
    osvers=2.10
    archname=i86pc-solaris-thread-multi
    uname='sunos solx8610u11 5.10 generic_147148-26 i86pc i386 i86pc '
    config_args='-Dcc=/bin/cc -des -Dusethreads -Duseshrplib=false -Dnoextensions=ODBM_File'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=define
    usemultiplicity=define
    use64bitint=undef
    use64bitall=undef
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
  Compiler:
    cc='/bin/cc'
    ccflags ='-D_REENTRANT -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64'
    optimize='-O'
    cppflags='-D_REENTRANT'
    ccversion='Studio 12.6 Sun C 5.15 SunOS_i386 2017/05/30'
    gccversion=''
    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='off_t'
    lseeksize=8
    alignbytes=4
    prototype=define
  Linker and Libraries:
    ld='/bin/cc'
    ldflags =' -L/usr/lib -L/usr/ccs/lib -L/opt/developerstudio12.6/lib/compilers/sse2 -L/opt/developerstudio12.6/lib/compilers -L/lib '
    libpth=/usr/lib /usr/ccs/lib /opt/developerstudio12.6/lib/compilers/sse2 /opt/developerstudio12.6/lib/compilers /lib
    libs=-lpthread -lsocket -lnsl -ldl -lm -lc
    perllibs=-lpthread -lsocket -lnsl -ldl -lm -lc
    libc=/lib/libc.so
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags='-KPIC'
    lddlflags='-G -L/usr/lib -L/usr/ccs/lib -L/opt/developerstudio12.6/lib/compilers/sse2 -L/opt/developerstudio12.6/lib/compilers -L/lib'


Characteristics of this binary (from libperl):
  Compile-time options:
    HAS_LONG_DOUBLE
    HAS_STRTOLD
    HAS_TIMES
    MULTIPLICITY
    PERLIO_LAYERS
    PERL_COPY_ON_WRITE
    PERL_DONT_CREATE_GVSV
    PERL_HASH_FUNC_ZAPHOD32
    PERL_HASH_USE_SBOX32
    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_REENTRANT_API
  Built under solaris
  Compiled at Sep 28 2023 14:42:16
  @INC:
    lib
    /opt/lib/perl5/site_perl/5.38.0/i86pc-solaris-thread-multi
    /opt/lib/perl5/site_perl/5.38.0
    /opt/lib/perl5/5.38.0/i86pc-solaris-thread-multi
    /opt/lib/perl5/5.38.0
@sisyphus
Copy link
Contributor

I think that some people would prefer to know that signbit(POSIX::NAN) returns the value it should, or (alternatively) to know that it returns the value it shouldn't.
I'm personally happy enough to live without such a test, but I wouldn't like to decree that everyone should be happy to do so.

It seems to me that in this instance that @rkennedy has reported, signbit(POSIX::NAN) is returning the value that it ought, but the test script is behaving erroneously.

@rkennedy, could you confirm that when the perl-5.38.0 you've quoted runs the following script:

use warnings;
use POSIX ':math_h_c99';
use POSIX ':nan_payload';
use Config;

print unpack "H*", pack "F>", NAN;
print "\n";
print unpack "H*", pack "F>", POSIX::NAN;

it outputs:

fff8000000000000
fff8000000000000

Cheers,
Rob

@rkennedy
Copy link
Author

Yes, Perl 5.38.0 prints both NAN and POSIX::NAN as fff8000000000000 on my Solaris i86pc system.

Based on my understanding of how the POSIX standard defines NAN, I think signbit(NAN) == 1 is valid behavior. (Weird, perhaps, but still correct.) The value of the sign bit of NAN is unspecified, so it follows that the result from checking is sign bit is also unspecified. There shouldn't be tests where the result is unspecified.

Unless math.t has some additional knowledge of how the underlying signbit and NAN implementations are supposed to be on the current platform, then I think it's wrong to assert that only one behavior is correct.

Maybe there's some way for the test to determine the underlying behaviors, without using the POSIX module? Then it can test that they're consistent. Without a comparison to something else, though, I don't think any result from signbit(NAN) can really be considered "wrong."

@sisyphus
Copy link
Contributor

Yes, I see the same on 32-bit MSVC-built perl-5.38.0 on Windows:

D:\>perl -MPOSIX -wle "print unpack 'H*', pack 'F>', NAN;"
fff8000000000000

But perl itself does it differently:

D:\>perl -MPOSIX -wle "print unpack 'H*', pack 'F>', 'nan';"
7ff8000000000000

On that Windows build, the POSIX signbit tests are skipped, as the signbit function doesn't work correctly anyway.

The current signbit(NAN) test in t/math.t apparently assumes that the POSIX::NAN will always have the sign bit unset.
If that assumption is deemed to be valid and desirable, then you've found a bug in the way POSIX.xs creates the NAN - and someone should fix that bug.

But if that assumption is not deemed "valid and desirable", then the signbit(NAN) test needs to be rewritten to cater for the possibility that the sign bit has been set.
Something like:

my $hex_nan = unpack "H*", pack "F>", NAN;
my $offset = 0;
# 80-bit extended precision long doubles
# require different offsets
$offset = 4 if $Config{nvtype} eq 'long double' &&
  ($Config{longdblkind} == 3 || $Config{longdblkind} == 4);

if(substr($hex_nan, $offset, 1) eq '7') {
  ok(!signbit(NAN), "signbit(NAN)");
}
else {
  ok(signbit(NAN), "signbit(NAN)");
}

That snippet requires that the POSIX, Test::More and Config modules have been loaded - which t/math.t has already done.
I think (hope ?) that correctly handles the big-endian extended-precision long doubles correctly. I have only little-endian architectures for testing.

I haven't really thought about what would be needed for the "Double-Double" long double builds. (My PPC64 box running Debian is currently unusable,)

Cheers,
Rob

@tonycoz
Copy link
Contributor

tonycoz commented Oct 9, 2023

Is signbit(copysign(NAN, 1.0)) zero on solaris?

copysign() supports NAN, from C99 "7.12.11.1 The copysign functions": "They produce a NaN (with the sign of y) if x is a NaN"

tonycoz added a commit to tonycoz/perl5 that referenced this issue Nov 2, 2023
The test in Configure warns:

/* Note that whether the sign bit is on or off
 * for NaN depends on the CPU/FPU, and possibly
 * can be affected by the build toolchain.

but this test assumed that the default NaN was always positive,
but this isn't the case with the Sun/Oracle workshop cc, whether
on Oracle Linux or on Solaris.

copysign() is however well defined for NaN, so we can modify the
sign on NaN and test that with signbit().

Fixes Perl#21533
tonycoz added a commit to tonycoz/perl5 that referenced this issue Nov 6, 2023
The test in Configure warns:

/* Note that whether the sign bit is on or off
 * for NaN depends on the CPU/FPU, and possibly
 * can be affected by the build toolchain.

but this test assumed that the default NaN was always positive,
but this isn't the case with the Sun/Oracle workshop cc, whether
on Oracle Linux or on Solaris.

copysign() is however well defined for NaN, so we can modify the
sign on NaN and test that with signbit().

Fixes Perl#21533
tonycoz added a commit that referenced this issue Nov 8, 2023
The test in Configure warns:

/* Note that whether the sign bit is on or off
 * for NaN depends on the CPU/FPU, and possibly
 * can be affected by the build toolchain.

but this test assumed that the default NaN was always positive,
but this isn't the case with the Sun/Oracle workshop cc, whether
on Oracle Linux or on Solaris.

copysign() is however well defined for NaN, so we can modify the
sign on NaN and test that with signbit().

Fixes #21533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants