calling convention fixes for better consistency #2050

DavidXanatos · 2024-05-04T10:49:23Z

The changes helps to compile the phlib into projects set up for different default calling conventions (like Task Explorer)

dmex · 2024-05-21T18:15:52Z

kphlib/include/kphdyndata.h

@@ -14,7 +14,7 @@
 #include <kphdyn.h>

 _Must_inspect_result_
-NTSTATUS KphDynDataGetConfiguration(
+NTSTATUS NTAPI KphDynDataGetConfiguration(


None of the Kph functions are exported or external.

Agreed, should not be necessary.

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.

dmex · 2024-05-21T18:17:15Z

phlib/include/apiimport.h

@@ -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)


These types don't have calling conventions.

well my VS2022 begs to differ without that fix i can not build a x86 version of Task Explorer

dmex · 2024-05-21T18:18:04Z

phlib/include/appresolver.h

@@ -14,23 +14,23 @@

 EXTERN_C_START

-HRESULT PhAppResolverGetAppIdForProcess(
+HRESULT NTAPI PhAppResolverGetAppIdForProcess(


The AppResolver functions are not external or exported.

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.

dmex · 2024-05-21T18:18:31Z

phlib/include/kphcomms.h

@@ -14,7 +14,7 @@

 #include <kphmsg.h>

-NTSTATUS KphFilterLoadUnload(
+NTSTATUS NTAPI KphFilterLoadUnload(


These functions are not exported or external.

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.

dmex · 2024-05-21T18:53:33Z

phlib/include/phutil.h

@@ -1853,6 +1853,7 @@ PhHungWindowFromGhostWindow(

 PHLIBAPI
 NTSTATUS
+NTAPI


dmex · 2024-05-21T18:54:24Z

phlib/include/settings.h

-VOID PhAddSetting(
+VOID
+NTAPI
+PhAddSetting(


jxy-s

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.

DavidXanatos · 2024-05-21T19:18:21Z

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.
Since SI is open source IMHO it is to be expected that other projects will use parts of it especially if they come prepackaged as convenient libraries.
For SI it is a change without a difference but for others its a great help hence I assumed it would be a good idea to be more explicit about the used calling convention.
Its disappointing that such a improvement is not welcome.

jxy-s · 2024-05-21T19:58:30Z

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.
Since SI is open source IMHO it is to be expected that other projects will use parts of it especially if they come prepackaged as convenient libraries.
For SI it is a change without a difference but for others its a great help hence I assumed it would be a good idea to be more explicit about the used calling convention.
Its disappointing that such a improvement is not welcome.

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 __cdecl. If it is, we should open an alternate discussion on correcting that problem separately from specifying it explicitly. The project might be misconfigured. But none of those changes would be in support of consuming that from a 3rd party.

DavidXanatos · 2024-05-21T20:08:15Z

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 __cdecl. If it is, we should open an alternate discussion on correcting that problem separately from specifying it explicitly. The project might be misconfigured. But none of those changes would be in support of consuming that from a 3rd party.

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.
So any 3rd party project which want to use kphlib needs to workaround this mismatch.

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.

jxy-s · 2024-05-24T21:04:05Z

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 __cdecl. If it is, we should open an alternate discussion on correcting that problem separately from specifying it explicitly. The project might be misconfigured. But none of those changes would be in support of consuming that from a 3rd party.

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. So any 3rd party project which want to use kphlib needs to workaround this mismatch.

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 kphlib for that build and ifdef out all the client kph logic instead.

DavidXanatos · 2024-05-24T21:08:34Z

The driver doesn't support 32-bit. We should probably stop referencing kphlib for that build and ifdef out all the client kph logic instead.

Only a couple of the changes addresses the kphlib all other changes address the user mode phlib.
If we could ensure phlib can be used by projects which don't use __stdcall as default that would already help a great lot :)

calling convention fixes for better consistency

63477fe

DavidXanatos requested a review from jxy-s as a code owner May 4, 2024 10:49

dmex requested changes May 21, 2024

View reviewed changes

jxy-s requested changes May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

calling convention fixes for better consistency #2050

calling convention fixes for better consistency #2050

DavidXanatos commented May 4, 2024

dmex May 21, 2024

jxy-s May 21, 2024

DavidXanatos May 21, 2024

dmex May 21, 2024

DavidXanatos May 21, 2024

dmex May 21, 2024

DavidXanatos May 21, 2024

dmex May 21, 2024

DavidXanatos May 21, 2024

dmex May 21, 2024

dmex May 21, 2024

jxy-s left a comment

DavidXanatos commented May 21, 2024

jxy-s commented May 21, 2024 •

edited

DavidXanatos commented May 21, 2024 •

edited

jxy-s commented May 24, 2024

DavidXanatos commented May 24, 2024 •

edited

calling convention fixes for better consistency #2050

Are you sure you want to change the base?

calling convention fixes for better consistency #2050

Conversation

DavidXanatos commented May 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxy-s left a comment

Choose a reason for hiding this comment

DavidXanatos commented May 21, 2024

jxy-s commented May 21, 2024 • edited

DavidXanatos commented May 21, 2024 • edited

jxy-s commented May 24, 2024

DavidXanatos commented May 24, 2024 • edited

jxy-s commented May 21, 2024 •

edited

DavidXanatos commented May 21, 2024 •

edited

DavidXanatos commented May 24, 2024 •

edited