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

Fix for win32 build with gcc #130

Merged
merged 1 commit into from
Sep 25, 2019
Merged

Fix for win32 build with gcc #130

merged 1 commit into from
Sep 25, 2019

Conversation

hyc
Copy link
Collaborator

@hyc hyc commented Sep 25, 2019

Compiling with gcc on win32 is broken. This breaks gitian/depends builds of Monero.

@tevador
Copy link
Owner

tevador commented Sep 25, 2019

I'm not sure why GCC defines a MSVC-specific macro. https://docs.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=vs-2019#microsoft-specific-predefined-macros

A simpler fix would be:

#if defined(_MSC_VER) && defined(_M_IX86)

because the whole purpose of the asm function is to fix a bug in the MSVC sqrt function. It shouldn't be required for GCC.

In any case, please run randomx-tests to verify that all rounding modes are working correctly.

@hyc
Copy link
Collaborator Author

hyc commented Sep 25, 2019

When building on Windows, all binaries are linked against the MSVC C runtime library, so if the bug is in the library as your comment indicates, then this is still needed even with gcc.

And I'd guess that particular macro was defined by the windows header files, not the compiler. Dunno though, didn't check.

Yeah, these are the only builtins

ubuntu@bbca643d0942:~$ i686-w64-mingw32-cpp -dM | grep 86
#define __DBL_DENORM_MIN__ ((double)4.94065645841246544176568792868221372e-324L)
#define __FLT_EVAL_METHOD_TS_18661_3__ 2
#define __FLT64X_EPSILON__ 1.08420217248550443400745280086994171e-19F64x
#define __i686 1
#define __DBL_MAX__ ((double)1.79769313486231570814527423731704357e+308L)
#define __i686__ 1
#define __FLT64_DENORM_MIN__ 4.94065645841246544176568792868221372e-324F64
#define __i386 1
#define _X86_ 1
#define __i386__ 1
#define __LDBL_EPSILON__ 1.08420217248550443400745280086994171e-19L
#define __FLT64_MAX__ 1.79769313486231570814527423731704357e+308F64
#define __FLT32X_MAX__ 1.79769313486231570814527423731704357e+308F32x
#define i386 1
#define __FLT32X_DENORM_MIN__ 4.94065645841246544176568792868221372e-324F32x
ubuntu@bbca643d0942:~$ 

@hyc
Copy link
Collaborator Author

hyc commented Sep 25, 2019

64bit randomx-tests ran fine. I need to rebuild the 32bit test...

32bit test failed.

[67] FSQRT_R (decode)                         ... PASSED
[68] FSQRT_R RoundToNearest (execute)         ...
This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.
Assertion failed!

@tevador
Copy link
Owner

tevador commented Sep 25, 2019

binaries are linked against the MSVC C runtime library

OK, then the assembly code is also needed. Let me know if the 32-bit tests pass, then I will merge it.

@hyc
Copy link
Collaborator Author

hyc commented Sep 25, 2019

No looks like you're right. The tests with my assembly patch failed. Just #if'ing it out works though. Then the tests proceed until test 84, compiler hash tests. I presume these are meant to fail since there is no 32bit JIT compiler.

@hyc hyc changed the title Fix for gcc asm syntax on win32 Fix for win32 build with gcc Sep 25, 2019
@tevador
Copy link
Owner

tevador commented Sep 25, 2019

tests proceed until test 84

Then it's OK.

There was a bug in the tests. It should skip the compiler hash tests if JIT is not supported. Fixed in 7c405a7

@tevador tevador merged commit a70dfd8 into tevador:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants