Windows, unit test swipl/rational

Help! Predicate
1

Dear @jan, dear @ridgeworks,

1- The unit test for swipl/rational/test_ieee754 fails under Windows, testing for the sign of the fractional part of “negative zero”.

assertion(float_parts(-0.0,   -0.0,   -0.0)), % fails

I tested it in a separate C snippet:

#include <stdio.h>
#include <math.h>

int main()
{
  double u = -0.0 ;
  double integ, fract ;
  fract = modf(u, &integ) ;
  fprintf(stderr, "u = %lf, integ = %lf, fract = %lf\n", u, integ, fract) ;

  /* fix */
  fract = copysign(modf(u, &integ), u) ;
  fprintf(stderr, "copysign: integ = %lf, fract = %lf\n", integ, fract) ;
  return 0 ;
}

The mistake can be fixed by an extra “copysign” :

$ ./a.exe
u = -0.000000, integ = -0.000000, fract = 0.000000
copysign: integ = -0.000000, fract = -0.000000

I guess, this would go into pl-arith.c (line 3800f)

case V_FLOAT:
    { double ip;
#ifdef MODF_BROKEN
      r->value.f = copysign(modf(n1->value.f, &ip), n1->value.f);
#else
      r->value.f = modf(n1->value.f, &ip);
#endif
      r->type = V_FLOAT;
      return check_float(r);
    }

By the way, according to the MinGW sources (see below), the “ip” is not needed, so that this might work as well:

      r->value.f = modf(n1->value.f, NULL);

2- Moreover, there’s a typo in a comment:

** <module> Test IEEEE754 float handling

The mistake can be fixed by removing one E :slight_smile:

3- MinGW code for modf

/**
 * This file has no copyright assigned and is placed in the Public Domain.
 * This file is part of the mingw-w64 runtime package.
 * No warranty is given; refer to the file DISCLAIMER.PD within this package.
 */
#include <fenv.h>
#include <math.h>
#include <errno.h>

double
modf (double value, double* iptr)
{
  double int_part = 0.0;
  /* truncate */
#if defined(_AMD64_) || defined(__x86_64__)
  asm volatile ("subq $8, %%rsp\n"
    "fnstcw 4(%%rsp)\n"
    "movzwl 4(%%rsp), %%eax\n"
    "orb $12, %%ah\n"
    "movw %%ax, (%%rsp)\n"
    "fldcw (%%rsp)\n"
    "frndint\n"
    "fldcw 4(%%rsp)\n"
    "addq $8, %%rsp\n" : "=t" (int_part) : "0" (value) : "eax"); /* round */
#elif defined(_X86_) || defined(__i386__)
  asm volatile ("push %%eax\n\tsubl $8, %%esp\n"
    "fnstcw 4(%%esp)\n"
    "movzwl 4(%%esp), %%eax\n"
    "orb $12, %%ah\n"
    "movw %%ax, (%%esp)\n"
    "fldcw (%%esp)\n"
    "frndint\n"
    "fldcw 4(%%esp)\n"
    "addl $8, %%esp\n\tpop %%eax\n" : "=t" (int_part) : "0" (value) : "eax"); /* round */
#else
  int_part = trunc(value);
#endif
  if (iptr)
    *iptr = int_part;
  return (isinf (value) ?  0.0 : value - int_part);
}
2

Thanks for sorting this out. I can live with a work around as you propose. I have my doubts about using NULL for ip. The Linux docs say nothing about whether or not that is allowed. I fear that might cause other platforms to break. I think you have seen how to deal with this using cmake for adding a test, no?

Hope there are not too many of these :frowning:

3

I’ll try.

This will a build time test, of course. At some point, not necessarily now, a further complication might be considered, with features like FP math being tested at runtime. Since the program might be installed on a different computer.

1 Like
4

I doubt this would be an issue. It has been compiled with the compiler used to test the feature and the result is bundled with the same runtime library. The only thing different could be the CPU. So far that doesn’t seem to make a difference.

In this case we could consider using the copysign() always and make a note why. After all, this should be a very cheap operation using IEEE754 floats as the sign is an explicit bit. It is a borderline case. In general I do not like calls that should not be needed as it will trigger people to think why?. The docs of modf() state explicitly that the sign of both parts is the same as the sign of the input.

5

I’ve read this after having submitted the PR.

Let’s rather ask the MinGW people to fix the problem. Not now, but in the near future, when I am done with the unit tests. Actually, I am. There’s only one left, and that’s probably a rather high-hanging fruit. I’ll open a separate thread for that.

6

Thanks for the PR. Applied. I still think this is better. The only pity is that it is a lot more work. It makes the issue explicit though. Hopefully pointing the MinGW maintainers on these issues will help.

Its really great to see the Windows version is close to passing all tests!

7

Sorry, I lost the thread, what is “this” referring to? To all the #ifdefs?

8

Yes :slight_smile: They are ugly, but still …

9

Just my opinion FWIW:

Seems to me that the test has correctly identified a bug in MinGW so the best solution is to correct the bug there to the benefit of all MinGW users.

But that means the test will continue to fail on Windows and this requires a policy decision as to whether this is a gating issue for a release. I assume it hasn’t been to date.

The copysign fix to pl-arith will mask the bug and is a reasonable workaround. I agree with Jan and would prefer to see it applied universally (with a suitable comment), i.e., avoid use of MODF_BROKEN conditional flag, as copysign should be a very cheap operation.

10

:+1:

This is an old and reoccurring scenario. Typically we work around bugs if reasonably possible and document them otherwise (in addition to reporting). Just waiting for the upstream fix of a dependency takes multiple years to be available on all platforms/distributions. @mgondan1 wants fully clean passing of the tests for Windows. I fully support that (and have been too lazy to do it).

I didn’t say that. I’d probably have done that as it is so short and I’m sometimes lazy. I’m happy that @mgondan1 has spent the effort to detect the dependency bug and only work around it for affected platform(s).

11

That’s fine by me; I have no skin in this game.