Fixes math library compiling on msvc #4511

lygstate · 2021-01-19T00:49:20Z

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

jerry-math/jerry-math-internal.h

jerry-math/include/math.h

akosthekiss · 2021-01-19T11:33:01Z

jerry-math/expm1.c

+        double_accessor const_0x1p1023;
+        const_0x1p1023.as_int.hi = 0x7fe00000;
+        const_0x1p1023.as_int.lo = 0;
+        y = y * 2.0 * const_0x1p1023.dbl;


I'd prefer a slightly different approach:

const double twop1023 = ((double_accessor) { .as_int = { .hi = 0x7fe00000, .lo = 0 } }).dbl; // 0x1p1023 y = y * 2.0 * twop1023;

not works on old msvc，

That will get problematic at some point. Please, specify what old means. JerryScript is a C99 project. We will have to draw the line somewhere, what deviations from the standard do we still tolerate. MSVC starts to build up quite a list.

That will get problematic at some point. Please, specify what old means. JerryScript is a C99 project. We will have to draw the line somewhere, what deviations from the standard do we still tolerate. MSVC starts to build up quite a list.

you are right, works:)

I'd prefer a slightly different approach:

const double twop1023 = ((double_accessor) { .as_int = { .hi = 0x7fe00000, .lo = 0 } }).dbl; // 0x1p1023 y = y * 2.0 * twop1023;

./jerry-math/expm1.c:268: error: brace should be placed on a separate line
Error: Process completed with exit code 1.

single line are invalid style

akosthekiss · 2021-01-19T16:53:52Z

BTW, why is it a must to compile jerry-math for Windows / with MSVC? MSVC has its own math implementation, why not just disable jerry-math on Windows? What's the gain with jerry-math?

Note that jerry-math, previously known as jerry-libm, was originally written for embedded systems where size constraints were so strict that even the math lib had to be optimized. I don't think that desktop Windows faces such constraints.

lygstate · 2021-01-19T17:19:24Z

BTW, why is it a must to compile jerry-math for Windows / with MSVC? MSVC has its own math implementation, why not just disable jerry-math on Windows? What's the gain with jerry-math?

Note that jerry-math, previously known as jerry-libm, was originally written for embedded systems where size constraints were so strict that even the math lib had to be optimized. I don't think that desktop Windows faces such constraints.

jerry-math already support on osx and linux, we can do it on msvc/win32 to match with these two os.
by using jerry-math, we can remove the need of msvcr(I want it), msvcrt also are a big things. so we can create minimal size javascript engine on win32, because small size is the advantage of jerryscript, we can take the advantage of that
we can creating cross-platform(win32,osx,linux,bare-metal) javascript engine and testing it with same code path(jerry-math also contained in the code path)

lygstate · 2021-01-19T17:43:48Z

jerry-math/log2.c

@@ -52,6 +51,7 @@
 double
 log2 (double x)
 {
+  const double zero = 0.0;


By doing this, can remove dive by zero warning/error on msvc

Please describe what kind of diagnostics are reported by MSVC. Division by a zero double literal should behave just like a division by a constant zero double variable. Usually. In a decent compiler where constants are propagated.

error messages：

build] [18/385 0% :: 0.065] Building C object jerry-math\CMakeFiles\jerry-math.dir\log10.c.obj [build] FAILED: jerry-math/CMakeFiles/jerry-math.dir/log10.c.obj [build] C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe /nologo -I..\..\jerry-math\include /DWIN32 /D_WINDOWS /W3 /wd4996 /MD /O1 /Ob1 /DNDEBUG /showIncludes /Fojerry-math\CMakeFiles\jerry-math.dir\log10.c.obj /Fdjerry-math\CMakeFiles\jerry-math.dir\jerry-math.pdb /FS -c ..\..\jerry-math\log10.c [build] ..\..\jerry-math\log10.c(87): error C2124: divide or mod by zero [build] [18/385 0% :: 0.065] Building C object jerry-math\CMakeFiles\jerry-math.dir\log1p.c.obj [build] FAILED: jerry-math/CMakeFiles/jerry-math.dir/log1p.c.obj [build] C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe /nologo -I..\..\jerry-math\include /DWIN32 /D_WINDOWS /W3 /wd4996 /MD /O1 /Ob1 /DNDEBUG /showIncludes /Fojerry-math\CMakeFiles\jerry-math.dir\log1p.c.obj /Fdjerry-math\CMakeFiles\jerry-math.dir\jerry-math.pdb /FS -c ..\..\jerry-math\log1p.c [build] ..\..\jerry-math\log1p.c(125): error C2124: divide or mod by zero [build] [18/385 0% :: 0.092] Building C object jerry-math\CMakeFiles\jerry-math.dir\asinh.c.obj [build] [18/385 1% :: 0.093] Building C object jerry-math\CMakeFiles\jerry-math.dir\atan2.c.obj [build] ..\..\jerry-math\atan2.c(77): warning C4146: unary minus operator applied to unsigned type, result still unsigned [build] [18/385 1% :: 0.102] Building C object jerry-math\CMakeFiles\jerry-math.dir\asin.c.obj [build] [18/385 1% :: 0.103] Building C object jerry-math\CMakeFiles\jerry-math.dir\acosh.c.obj [build] [18/385 1% :: 0.106] Building C object jerry-math\CMakeFiles\jerry-math.dir\cbrt.c.obj [build] [18/385 2% :: 0.110] Building C object jerry-math\CMakeFiles\jerry-math.dir\copysign.c.obj [build] [18/385 2% :: 0.113] Building C object jerry-math\CMakeFiles\jerry-math.dir\ceil.c.obj [build] ..\..\jerry-math\ceil.c(116): warning C4018: '<': signed/unsigned mismatch [build] [18/385 2% :: 0.115] Building C object jerry-math\CMakeFiles\jerry-math.dir\floor.c.obj [build] ..\..\jerry-math\floor.c(115): warning C4018: '<': signed/unsigned mismatch [build] [18/385 2% :: 0.116] Building C object jerry-math\CMakeFiles\jerry-math.dir\atanh.c.obj [build] c:\work\study\languages\typescript\jerryscript\jerry-math\atanh.c(69) : warning C4723: potential divide by 0 [build] [18/385 3% :: 0.116] Building C object jerry-math\CMakeFiles\jerry-math.dir\fabs.c.obj [build] [18/385 3% :: 0.116] Building C object jerry-math\CMakeFiles\jerry-math.dir\cosh.c.obj [build] [18/385 3% :: 0.118] Building C object jerry-math\CMakeFiles\jerry-math.dir\acos.c.obj [build] [18/385 3% :: 0.119] Building C object jerry-math\CMakeFiles\jerry-math.dir\expm1.c.obj [build] [18/385 4% :: 0.120] Building C object jerry-math\CMakeFiles\jerry-math.dir\atan.c.obj [build] [18/385 4% :: 0.126] Building C object jerry-math\CMakeFiles\jerry-math.dir\fmod.c.obj [build] ..\..\jerry-math\fmod.c(54): warning C4146: unary minus operator applied to unsigned type, result still unsigned [build] [18/385 4% :: 0.126] Building C object jerry-math\CMakeFiles\jerry-math.dir\exp.c.obj [build] ninja: build stopped: subcommand failed. [build] Build finished with exit code 1

on msvc, they becave differently, warning vs error

akosthekiss · 2021-01-20T22:10:59Z

by using jerry-math, we can remove the need of msvcr(I want it), msvcrt also are a big things. so we can create minimal size javascript engine on win32, because small size is the advantage of jerryscript, we can take the advantage of that

Please, note that jerry-math is not a fully fledged libm. It does not contain all mathematical functions mandated by the C standard. Only those functions were imported/adapted that are directly used by the engine itself. If the engine is embedded into an application, as it is intended, it is highly probable that other parts of the application will use mathematical functions that are not provided by jerry-math. And in that case the toolchain's own libm implementation (msvcrt, in the case of MSVC) will be the only way to move forward.

lygstate · 2021-01-20T22:15:02Z

by using jerry-math, we can remove the need of msvcr(I want it), msvcrt also are a big things. so we can create minimal size javascript engine on win32, because small size is the advantage of jerryscript, we can take the advantage of that

Please, note that jerry-math is not a fully fledged libm. It does not contain all mathematical functions mandated by the C standard. Only those functions were imported/adapted that are directly used by the engine itself. If the engine is embedded into an application, as it is intended, it is highly probable that other parts of the application will use mathematical functions that are not provided by jerry-math. And in that case the toolchain's own libm implementation (msvcrt, in the case of MSVC) will be the only way to move forward.

Yes, use jerry-math is not seeking for fully fledged libm, we are always looking for small code size other than fully fledged libm

akosthekiss · 2021-01-20T22:18:51Z

jerry-math/jerry-math-internal.h

+#if defined (_MSC_VER)
+#if defined (_M_IX86) || defined (_M_AMD64) || defined (_M_ARM) || defined (_M_ARM64)
+#define __LITTLE_ENDIAN
+#endif


Don't do this. Just add the defined (_M_IX86) || defined (_M_AMD64) || defined (_M_ARM) || defined (_M_ARM64) line to the existing list. BTW, you've omitted _M_X64 from the original suggestion.

It doesn't matter if some macros are only defined by gcc or clang, others only by msvc. They can be tested in the same list. They are just macros. It makes no harm if a compiler doesn't define / use / recognize some of them (as long as they are tested with defined().

akosthekiss · 2021-01-20T22:42:30Z

jerry-math/log2.c

@@ -52,6 +51,7 @@
 double
 log2 (double x)
 {
+  const double zero = 0.0;


Please describe what kind of diagnostics are reported by MSVC. Division by a zero double literal should behave just like a division by a constant zero double variable. Usually. In a decent compiler where constants are propagated.

akosthekiss · 2021-01-22T22:46:47Z

jerry-math/include/math.h

-#define INFINITY    (1.0/0.0)
-#define NAN         (0.0/0.0)
+#define INFINITY    (1e+300 * 1e+300) /* 1e+300*1e+300 must overflow */
+#define NAN         (INFINITY * 0.0)


So, I did my homework. And it just turned out that (a) I did not know precisely how the standard specifies infinity and NaN, and (b) jerry-math's math.h was incorrect all along.

According to the specs:

INFINITY and NAN are float constant expressions, and

HUGE_VAL is a double constant expression (that evaluates to infinity).

Therefore, I suggest to have these macros as suggested for the Windows/MSVC configuration, but also keep a slightly updated version of the existing macros for other configs. That is:

#ifdef _MSC_VER #define INFINITY ((float) (1e+300 * 1e+300)) #define NAN ((float) (INFINITY * 0.0f)) #else #define INFINITY ((float) (1.0/0.0)) #define NAN ((float) (0.0/0.0)) #endif #define HUGE_VAL ((double) INFINITY)

akosthekiss · 2021-01-22T22:47:33Z

jerry-math/jerry-math-internal.h

@@ -74,7 +76,8 @@ typedef union
 #endif /* __LITTLE_ENDIAN */

 #ifndef NAN
-#define NAN (0.0/0.0)
+/* 1e+300*1e+300 must overflow */
+#define NAN         ((1e+300 * 1e+300) * 0.0)


This NAN definition should be changed likewise.

akosthekiss · 2021-01-22T22:50:37Z

jerry-math/log.c

@@ -93,6 +92,7 @@
 double
 log (double x)
 {
+  const double zero = 0.0;


And in these files, instead of using a zero variable to make division by zero possible on MSVC, the div0 expressions should be changed to +/-INFINITY or NAN. (This is how the patch looked like at some point of time, but then it has changed.)

akosthekiss · 2021-01-23T08:32:02Z

jerry-math/log.c

@@ -105,7 +105,7 @@ log (double x)
  {
    if (((hx & 0x7fffffff) | lx) == 0) /* log(+-0) = -inf */
    {
-      return -two54 / zero;
+      return -INFINITY;


How come that line 112 does not have to be edited? That's a div0 as well, only for NaN.

weird， the compiler didn't complain that， anyway， I will update there

akosthekiss · 2021-01-23T08:33:16Z

jerry-math/log10.c

@@ -84,7 +84,7 @@ log10 (double x)
    if (((hx & 0x7fffffff) | lx) == 0)
    {
      /* log(+-0)=-inf */
-      return -two54 / zero;
+      return -INFINITY;


jerry-math/log1p.c

akosthekiss · 2021-01-23T08:35:16Z

jerry-math/jerry-math-internal.h

+#ifdef _MSC_VER
+#define INFINITY    ((float) (1e+300 * 1e+300)) /* 1e+300*1e+300 must overflow */
+#define NAN         ((float) (INFINITY * 0.0f))
+#else


(minor) If the PR gets updated, it would be nice to add a comment here: /* !_MSC_VER */

this suggestion did not get applied

akosthekiss · 2021-01-23T08:36:42Z

jerry-math/jerry-math-internal.h

+#define NAN         ((float) (INFINITY * 0.0f))
+#else
+#define INFINITY    ((float) (1.0/0.0))
+#define NAN         ((float) (0.0/0.0))


(minor) for some legacy reason, it seems that there were no spaces around the division operator. Now would be a chance to add then.

akosthekiss

LGTM (except for the missing preproc directive comment)

akosthekiss · 2021-01-23T11:36:00Z

jerry-math/jerry-math-internal.h

+#ifdef _MSC_VER
+#define INFINITY    ((float) (1e+300 * 1e+300)) /* 1e+300*1e+300 must overflow */
+#define NAN         ((float) (INFINITY * 0.0f))
+#else


this suggestion did not get applied

lygstate · 2021-01-23T11:49:35Z

LGTM (except for the missing preproc directive comment)

forgot, let's do it

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

dbatyai

LGTM

akosthekiss requested changes Jan 19, 2021

View reviewed changes

akosthekiss added jerry-libm Related to the jerry-libm library windows Windows specific labels Jan 19, 2021

lygstate force-pushed the math-msvc branch from b7c62d9 to c197bd2 Compare January 19, 2021 17:41

lygstate commented Jan 19, 2021

View reviewed changes

lygstate force-pushed the math-msvc branch from c197bd2 to 2be478a Compare January 19, 2021 17:53

lygstate requested a review from akosthekiss January 19, 2021 21:04

akosthekiss requested changes Jan 20, 2021

View reviewed changes

lygstate force-pushed the math-msvc branch 3 times, most recently from 3975385 to 44473c3 Compare January 21, 2021 07:03

akosthekiss requested changes Jan 22, 2021

View reviewed changes

lygstate force-pushed the math-msvc branch from 44473c3 to e20de34 Compare January 23, 2021 04:06

lygstate requested a review from akosthekiss January 23, 2021 04:23

akosthekiss reviewed Jan 23, 2021

View reviewed changes

lygstate force-pushed the math-msvc branch from e20de34 to dc3ad1c Compare January 23, 2021 11:26

akosthekiss approved these changes Jan 23, 2021

View reviewed changes

Fixes math library compiling on msvc

e1c25bb

JerryScript-DCO-1.0-Signed-off-by: Yonggang Luo luoyonggang@gmail.com

lygstate force-pushed the math-msvc branch from dc3ad1c to e1c25bb Compare January 23, 2021 11:57

dbatyai approved these changes Feb 4, 2021

View reviewed changes

dbatyai merged commit aa941ed into jerryscript-project:master Feb 4, 2021

lygstate deleted the math-msvc branch February 9, 2021 15:34

Fixes math library compiling on msvc #4511

Fixes math library compiling on msvc #4511

Uh oh!

Conversation

lygstate commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Jan 19, 2021

Uh oh!

lygstate commented Jan 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented Jan 20, 2021

Uh oh!

lygstate commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lygstate commented Jan 23, 2021

Uh oh!

dbatyai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lygstate commented Jan 19, 2021 •

edited

Loading

lygstate commented Jan 20, 2021 •

edited

Loading