-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
calling convention fixes for better consistency #2050
base: master
Are you sure you want to change the base?
Conversation
@@ -14,7 +14,7 @@ | |||
#include <kphdyn.h> | |||
|
|||
_Must_inspect_result_ | |||
NTSTATUS KphDynDataGetConfiguration( | |||
NTSTATUS NTAPI KphDynDataGetConfiguration( |
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.
None of the Kph functions are exported or external.
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.
Agreed, should not be necessary.
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.
3rd party projects which use the kphlib may link against un exported functions, and if they by default use a different calling convention then __stdcall a fix like this is required.
@@ -229,7 +229,7 @@ typedef VOID (WINAPI* _UnsubscribeServiceChangeNotifications)( | |||
_In_ PSC_NOTIFICATION_REGISTRATION pSubscription | |||
); | |||
|
|||
#define PH_DECLARE_IMPORT(Name) _##Name Name##_Import(VOID) | |||
#define PH_DECLARE_IMPORT(Name) _##Name NTAPI Name##_Import(VOID) |
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.
These types don't have calling conventions.
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.
well my VS2022 begs to differ without that fix i can not build a x86 version of Task Explorer
@@ -14,23 +14,23 @@ | |||
|
|||
EXTERN_C_START | |||
|
|||
HRESULT PhAppResolverGetAppIdForProcess( | |||
HRESULT NTAPI PhAppResolverGetAppIdForProcess( |
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.
The AppResolver functions are not external or exported.
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.
3rd party projects which use the kphlib may link against un exported functions, and if they by default use a different calling convention then __stdcall a fix like this is required.
@@ -14,7 +14,7 @@ | |||
|
|||
#include <kphmsg.h> | |||
|
|||
NTSTATUS KphFilterLoadUnload( | |||
NTSTATUS NTAPI KphFilterLoadUnload( |
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.
These functions are not exported or external.
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.
3rd party projects which use the kphlib may link against un exported functions, and if they by default use a different calling convention then __stdcall a fix like this is required.
@@ -1853,6 +1853,7 @@ PhHungWindowFromGhostWindow( | |||
|
|||
PHLIBAPI | |||
NTSTATUS | |||
NTAPI |
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.
✔️
VOID PhAddSetting( | ||
VOID | ||
NTAPI | ||
PhAddSetting( |
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.
✔️
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.
Many of the calling convention changes aren't necessary upstream. The request is to support oddity calling conventions that might be default in a secondary project. These are internal libraries and not intended to be exported. If there is a use case of a secondary project, those should be maintained in an alternate fork that is specific to the oddities of that other project.
The calling convention use by VS by default is __cdecl so if anything the default use of __stdcall is an oddity. |
I'm not sure that's accurate. For kphlib, it's an x64 or ARM64 static library linked and compiled into the target binary. So the calling convention shouldn't be |
x64 and ARM64 each only supports one calling convention which booth somewhat resembles __vectorcall the explicit calling convention specifier have only a meaning on x86, so the issue only arises when trying to compile a project using kphlib for x86, the others x64 and ARM64 work just fine no matter what. Process Hacker opted to, for whatever reason, to use __stdcall in its projects, which as described is not the VS default nowadays new projects start out with __cdecl preset. If you don't want to support the use case of kphlib being linked by 3rd parties than this pull request is irrelevant and should be closed. PS: IMHO fostering 3rd party usage would help to engage more contributions to the project and be in the projects very own self interest. |
The driver doesn't support 32-bit. We should probably stop referencing |
Only a couple of the changes addresses the kphlib all other changes address the user mode phlib. |
The changes helps to compile the phlib into projects set up for different default calling conventions (like Task Explorer)