Skip to content

MinGW fix #162

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

MinGW fix #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion crypto/cryptonight_aesni.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@

#ifdef __GNUC__
#include <x86intrin.h>
static inline uint64_t _umul128(uint64_t a, uint64_t b, uint64_t* hi)
#ifdef __MINGW64__
extern "C"
#else
static
#endif // __MINGW64__
inline uint64_t _umul128(uint64_t a, uint64_t b, uint64_t* hi)
{
unsigned __int128 r = (unsigned __int128)a * (unsigned __int128)b;
*hi = r >> 64;
Expand Down
4 changes: 4 additions & 0 deletions minethd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@

void thd_setaffinity(std::thread::native_handle_type h, uint64_t cpu_id)
{
#ifdef __MINGW64__
SetThreadAffinityMask(&h, 1ULL << cpu_id);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@psychocrypt Can you verify the correctness of this line by running it? HANDLE type on Windows maps to void* so it will build either way (value of h or address of h) but work only in one case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't verify it because I have no MinGW, I have only cleaned up this PR for the user.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get MinGW-GCC from: https://sourceforge.net/projects/mingw-w64/files/mingw-w64/

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibroheem Nice you are still with us =). Can you point us in the way of any mingw docs showing &h is the correct choice here?

Copy link

@ibroheem ibroheem Jun 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. Declaration of SetThreadAffinityMask in the file
..\x86_64-w64-mingw32\include\winbase.h

1160  WINBASEAPI DWORD_PTR WINAPI SetThreadAffinityMask (HANDLE hThread, DWORD_PTR dwThreadAffinityMask);

2. HANDLE typedefs ..\x86_64-w64-mingw32\include\winnt.h (line 397 - 403)
HANDLE --> void*

#ifdef STRICT
  typedef void *HANDLE;
#define DECLARE_HANDLE(name) struct name##__ { int unused; }; typedef struct name##__ *name
#else
typedef PVOID HANDLE;
#define DECLARE_HANDLE(name) typedef HANDLE name
#endif

6.2.0\x86_64-w64-mingw32\include\winnt.h

250   typedef void *PVOID;
251   typedef void *PVOID64;

Therefore SetThreadAffinityMask is just a in MinGW:

WINBASEAPI DWORD_PTR WINAPI SetThreadAffinityMask (void* hThread, DWORD_PTR dwThreadAffinityMask);

Hence, nothing goes into hThread, except ptr variable or an address of a (lvalue) variable.

Thanks

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HANDLEs on Windows are usually passed by value, as you can see in the code on the other side of the #else.

Then it seem there's a slight difference in the SetThreadAffinityMask provided by MSVC and MinGW.

My point is that the HANDLE = void* makes it possible for your code to fail silently (both at compile and run time) if you pass an address where a value is expected.

A pointer is expected in the MinGW version of SetThreadAffinityMask for the param hThread. As shown in the src x86_64-w64-mingw32\include\winnt.h (line 397 - 403), HANDLE is a typedef of void*. Thus a:

  • var, where var is T* var
  • &var, where var is T var

is expected in SetThreadAffinityMask (HANDLE hThread, NOT a value.

That is why there's a #ifdef guard specifically for MinGW64.

My point is that the HANDLE = void* makes it possible for your code to fail silently (both at compile and run time) if you pass an address where a value is expected.

The code only get compiled for MinGW64 platforms only.
The implementation that expects values can fall back to the #else code

In conclusion, the best way to know is to try it out yourself, then you'll have surity.

Copy link
Owner

@fireice-uk fireice-uk Jun 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not quite correct on theory side this is valid code in C or C++ (in fact it is quite useful for writing shellcodes)

#include <stdio.h>
int main()
{
	void* stack_top;
	stack_top = &stack_top;
	printf("%.16lX", (unsigned long)stack_top);
	return 0;
}

Or if you want to put it another way void** = void*

Do you mind if I put this one on hold until I can verify it myself?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more point of confusion - SetThreadAffinityMask is not provided by the compiler, it is part of the Windows kernel, it is called the same way whether you are using MSVC/GCC/VB/FORTRAN etc.

std::thread::native_handle_type on the other hand is a C++ wrapper around HANDLE that can be specific to the particular C++ library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetThreadAffinityMask is not provided by the compiler, it is part of the Windows kernel

I meant what I saw in the header file provided by MinGW64.
I knew that function is part of Windows Kernel or wherever they might put it, it's not compiler's.
But those headers were provided by MinGW.
I never used VS, so I don't know for sure if there's any difference.

Copy link

@ibroheem ibroheem Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bottom line: MinGW accepts &h and not h, as far as my tests goes.

You can hold, in fact you should hold this up until you're convinced, by testing... ofcourse!

#else
SetThreadAffinityMask(h, 1ULL << cpu_id);
#endif // __MINGW64__
}
#else
#include <pthread.h>
Expand Down