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

Mingw-w64: 'sigset_t' does not name a type #237

Closed
anonimal opened this issue Jul 31, 2016 · 9 comments
Closed

Mingw-w64: 'sigset_t' does not name a type #237

anonimal opened this issue Jul 31, 2016 · 9 comments

Comments

@anonimal
Copy link
Contributor

I'll preface this issue by saying I almost never use windows so I apologize in advance if the error is only on my end.

Building against 0b8cea5 on Kovri's win build machine:

$ mkdir kovri/deps/cryptopp/build && cd kovri/deps/cryptopp/build
$ cmake -G 'MSYS Makefiles' ../
-- The C compiler identification is GNU 5.4.0
-- The CXX compiler identification is GNU 5.4.0
-- Check for working C compiler: C:/msys64/mingw64/bin/gcc.exe
-- Check for working C compiler: C:/msys64/mingw64/bin/gcc.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: C:/msys64/mingw64/bin/g++.exe
-- Check for working CXX compiler: C:/msys64/mingw64/bin/g++.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Check if the system is big endian
-- Searching 16 bit integer
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of unsigned short
-- Check size of unsigned short - done
-- Using unsigned short
-- Check if the system is big endian - little endian
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - found
-- Found Threads: TRUE
-- Configuring done
-- Generating done
-- Build files have been written to: C:/msys64/home/Administrator/kovri/deps/cryptopp/build
$ make
...
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp: In function 'bool CryptoPP::CpuId(CryptoPP::word32, CryptoPP::word32*)':                                    [3/13]
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:106:11: error: 'sigset_t' does not name a type
  volatile sigset_t oldMask;
           ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:107:28: error: 'sigset_t' was not declared in this scope
  if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
                            ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:107:37: error: expected primary-expression before ')' token
  if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
                                     ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:107:39: error: 'oldMask' was not declared in this scope
  if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
                                       ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:107:46: error: 'sigprocmask' was not declared in this scope
  if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
                                              ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:128:28: error: 'sigset_t' was not declared in this scope
  sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
                            ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:128:37: error: expected primary-expression before ')' token
  sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
                                     ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:128:39: error: 'oldMask' was not declared in this scope
  sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
                                       ^
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:128:52: error: 'sigprocmask' was not declared in this scope
  sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
                                                    ^
make[2]: *** [CMakeFiles/cryptopp-object.dir/build.make:87: CMakeFiles/cryptopp-object.dir/cpu.cpp.obj] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [CMakeFiles/Makefile2:68: CMakeFiles/cryptopp-object.dir/all] Error 2
make: *** [Makefile:139: all] Error 2
$ cd c:/msys64/mingw64 && grep -R sigset_t | grep types
x86_64-w64-mingw32/include/sys/types.h:typedef unsigned long long _sigset_t;
x86_64-w64-mingw32/include/sys/types.h:typedef unsigned long    _sigset_t;
x86_64-w64-mingw32/include/sys/types.h:typedef _sigset_t        sigset_t;

Note: mingw is not yet supported for Kovri so building manually within deps/cryptopp is needed for testing.

  • The same results are given when running cryptest.sh
  • Simply including <sys/types.h> in cpu.cpp has no effect
@noloader
Copy link
Collaborator

noloader commented Aug 1, 2016

MinGW is the one platform I cannot test, despite sincere efforts. At one time I was trying to get it to work on 4 different machines with no joy.

According to the Open Group the standard header for sigset_t is signal.h. cpu.cpp includes the header:

$ grep signal * | grep include
cpu.cpp:#include <signal.h>
trap.h:#    include <signal.h>

It looks like the problem is here, in cpu.cpp line 15 or so:

016     #ifndef CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY
017     #include <signal.h>
018     #include <setjmp.h>
019     #endif

CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY is set in cpu.h. It should be set around line 490:

487    #elif defined(_MSC_VER) || defined(__BORLANDC__)
488    #define CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY
489    ...

It looks like we need to change line 19 to:

016     #if !defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY) || defined(__MINGW32__)
017     #include <signal.h>
018     #include <setjmp.h>
019     #endif

Can you try making the change above?

@anonimal
Copy link
Contributor Author

anonimal commented Aug 1, 2016

Hmm... that looks good but I get the same results:

diff --git a/cpu.cpp b/cpu.cpp
index fba5922..08f5221 100644
--- a/cpu.cpp
+++ b/cpu.cpp
@@ -13,7 +13,7 @@
 #include "misc.h"
 #include <algorithm>

-#ifndef CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY
+#if !defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY) || defined(__MINGW32__)^M
 #include <signal.h>
 #include <setjmp.h>
 #endif
$ cd build/ && rm -fr  * && cmake -G 'MSYS Makefiles' ../ && make
...
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp: In function 'bool CryptoPP::CpuId(CryptoPP::word32, CryptoPP::word32*)':
C:/msys64/home/Administrator/kovri/deps/cryptopp/cpu.cpp:106:11: error: 'sigset_t' does not name a type
  volatile sigset_t oldMask;
...

I don't have time to do any real work on this at the moment (working on other kovri build items) but I can test more patches that you send.

@noloader
Copy link
Collaborator

noloader commented Aug 1, 2016

 -#ifndef CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY
+#if !defined(CRYPTOPP_MS_STYLE_INLINE_ASSEMBLY) || defined(__MINGW32__)^M

Ah, emacs. Me too.

Hmm... that looks good but I get the same results...

It just occurred to me... MinGW may need the Windows code path with Structured Exception Handling (SEH) and the try/except/finally.

Do you know what MinGW needs?


Now open on the MinGW-w64 mailing list: Does MinGW support Signals and sigset_t ?

Lets get some feedback from them since they know their platform best.


I don't have time to do any real work on this at the moment (working on other kovri build items) but I can test more patches that you send.

OK, thanks.

Looking at 5.6.2's cpu.cpp, we did not remove anything specific to MinGW. It looks like the culprit is adding the call to sigprocmask to save/restore the signal mask under Linux and Unix.

@noloader
Copy link
Collaborator

noloader commented Aug 1, 2016

C:/msys64/.../cpu.cpp:106:11: error: 'sigset_t' does not name a type
volatile sigset_t oldMask;
^
C:/msys64/.../cpu.cpp:107:28: error: 'sigset_t' was not declared in this scope
if (sigprocmask(0, NULL, (sigset_t*)&oldMask))

Here's some history on this change....

Wei used the "try instruction/catch illegal instruction" pattern on x86 for years. Things "just worked" everywhere, and it avoided platform specific tests. I think it was a great design choice, given what I know from testing on Android, BSD, Linix, OS X, Solaris, Windows, etc. For example, HWCAPS and getauxvalue() only works on Linux; and does not work on iOS, Windows Phone or Windows Store apps. In fact, Linux's HWCAPS does not work as expected on Aarch32 execution environments on Aarch64!

When we moved into ARM we found some gaps that were not present under x86. The gaps arose because ARM does not have a userland equivalent to CPUID. A userland program cannot check feature bits because its a privileged operation. Trying to check for, say NEON, requires Exception Level 1 (EL1). Performing the operation from userland results in a SIGILL because it runs at EL0.

For ARM, we need to try a NEON instruction, [maybe] catch the SIGILL, then set g_hasNEON. We need to do the same thing PMULL, PMULL2, AES, SHA1, SHA2, etc. What we found was the first test worked as expected, but the second (and later) tests produced unexpected results. Also see Runtime feature testing, setjmp, longjmp, and signal mask on Stack Overflow (it arose because of this problem).

We found the OS corrupted (or maybe, did not preserve) the process signal mask after the first SIGILL, so second (and later) instruction probes resulted in unexpected results and eventually led to program termination. If a SIGILL was not raised, then everything worked as expected. Once we preserved the signal mask, everything worked as expected both with and without a SIGILL.

And now we are here :)


Commit that added runtime checks for ARM:

Commit that fixed the missing volatile on local objects (same as sigmask commit):

Commit that added sigmask preservation (same as volatile commit):

@anonimal
Copy link
Contributor Author

anonimal commented Aug 1, 2016

Do you know what MinGW needs?

I can't say with confidence that I do. Years of Linux development and I've always avoided MS like the plague. I'm going to talk to Monero devs though to see if they have any input on our issue. I know they have far more MinGW experience than I.

I think I'd like to try to force MinGW to use Windows SEH next. maybe something like:

As expected, attempting to compile that code brings up syntax errors. I don't know how/if MinGW can use MS style inline as long as I'm using gcc (maybe someone else knows?).

The gaps arose because ARM does not have a userland equivalent to CPUID

Wow, alot of hoops to jump through for ARM (I'll take note)! I implemented a crude, AES-NI runtime detection/implementation a while back and knew that ARM EL0/EL1 would be fun to deal with, but not that much fun 😆 I'm impressed by the solutions you were able to find.

@noloader
Copy link
Collaborator

noloader commented Aug 1, 2016

@anonimal, Since MinGW-w64 only works on x86_64, what do you think about the following?

The SIGILL handler is only needed for x86_64 OSes from 2003-2005 era. That's when OSes were adding SSE2 support. SSE2 takes more than CPU support because the OS must save FPU/XMM state with XSAVE. Once SSE2/OS is verified, the remainder of features can be tested with CPUID.

$ cat cpu-mingw.diff 
diff --git a/cpu.cpp b/cpu.cpp
index 532c5a1..ce9fc2e 100644
--- a/cpu.cpp
+++ b/cpu.cpp
@@ -101,9 +101,11 @@ bool CpuId(word32 input, word32 output[4])
    if (oldHandler == SIG_ERR)
        return false;

+# ifndef __MINGW32__
    volatile sigset_t oldMask;
    if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
        return false;
+# endif

    if (setjmp(s_jmpNoCPUID))
        result = false;
@@ -123,7 +125,10 @@ bool CpuId(word32 input, word32 output[4])
        );
    }

+# ifndef __MINGW32__
    sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
+# endif
+
    signal(SIGILL, oldHandler);
    return result;
 #endif
@@ -160,9 +165,11 @@ static bool TrySSE2()
    if (oldHandler == SIG_ERR)
        return false;

+# ifndef __MINGW32__
    volatile sigset_t oldMask;
    if (sigprocmask(0, NULL, (sigset_t*)&oldMask))
        return false;
+# endif

    if (setjmp(s_jmpNoSSE2))
        result = false;
@@ -176,7 +183,10 @@ static bool TrySSE2()
 #endif
    }

+# ifndef __MINGW32__
    sigprocmask(SIG_SETMASK, (sigset_t*)&oldMask, NULL);
+# endif
+
    signal(SIGILL, oldHandler);
    return result;
 #endif

@luigi1111
Copy link

Builds successfully here with these changes! Do I need to do something else to see if it worked properly? (I'm from the Monero Project and have never used cryptopp before.)

@noloader
Copy link
Collaborator

noloader commented Aug 1, 2016

Builds successfully here with these changes! Do I need to do something else to see if it worked properly?

I don't believe so.

How about a PR?

@anonimal
Copy link
Contributor Author

anonimal commented Aug 1, 2016

Patch confirmed! I spoke with @luigi1111, I'll PR in a few minutes.

noloader added a commit that referenced this issue Aug 1, 2016
cpu: fix MinGW-w64 build. Closes #237
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