-
Notifications
You must be signed in to change notification settings - Fork 478
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 fix #162
base: dev
Are you sure you want to change the base?
MinGW fix #162
Conversation
OK, no problem. |
@psychocrypt Can you please address my comment on the source code? |
@fireice-uk Which comments? Do you miss to press the send buttom after your review? |
@@ -33,7 +33,11 @@ | |||
|
|||
void thd_setaffinity(std::thread::native_handle_type h, uint64_t cpu_id) | |||
{ | |||
#ifdef __MINGW64__ | |||
SetThreadAffinityMask(&h, 1ULL << cpu_id); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
@psychocrypt Yup, you are correct -.- Sorry for holding this one up. |
The original PR was #140, I made a mistake squashing and updating @ibroheem's branch therefore I opened a new one (the author of the commit is still @ibroheem)
Fixed issues preventing compilation when using a MinGW-64 GCC toolchain.
These fixes did not deal with linking issues, only makes sure everything compiles fine on a MinGW-64 GCC.
Tested with compilers from: https://sourceforge.net/projects/mingw-w64/files/mingw-w64/
You can test base:dev with my dev branch and see whats up.