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

node-api: explicitly set __cdecl for Node-API functions #42780

Closed
wants to merge 1 commit into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Apr 18, 2022

The issue

The Node-API is a quite generic ABI-safe API that can be used as JavaScript engine ABI-safe API outside of Node.JS project.
The issue is that currently it does not specify calling conventions which is critical if Windows DLLs compiled with different default calling conventions. It is not important for x64 or non-Windows platforms because they use one predefined calling convention, but for Windows x86 applications there are multiple calling conventions, and their mismatch causes a runtime crash.

E.g. I had previously added __cdecl to v8jsi.dll copy of js_native_api.h, but we still saw crashes in Windows x86 because __cdecl was not added to function pointers in js_native_api_types.h. The issue is being addressed by microsoft/v8-jsi#122.
This example shows how important the calling conventions are for Window x86 platform.

The solution

In this PR we are adding __cdecl to all functions and function pointers that target Win32 platform.
To do that we add a new macro NAPI_CDECL. It is expanded to __cdecl for Win32 platforms and to nothing for other platforms.

Discussion

This PR sets __cdecl as the calling conventions because it is the default in C/C++ compilers.
It would be ideal to use __stdcall calling conventions to match Windows API because it produces more compact code on calling side, but my concern is that such change may affect existing code. Though, if Node.JS is not shipped for x86, then it may be still safe to use __stdcall instead of __cdecl.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 18, 2022
@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Apr 22, 2022
@KevinEady
Copy link
Contributor

We discussed this on today's Node API meeting. This makes sense to me. Does anyone else on the team have any issues / concerns? We may need to look at the node-addon-api wrapper as well, but since this is an inline header it is not particularly needed for the ABI stability guarantees that Node API has.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2022
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM. From the discussion in the last Node-api team meeting my understaning is that does not change anything since CDECL was already the default, but it does help my making it explicit.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 2, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

mhdawson pushed a commit that referenced this pull request May 13, 2022
PR-URL: #42780
Reviewed-By: Michael Dawson <midawson@redhat.com>
@mhdawson
Copy link
Member

Landed in fb74474

@mhdawson mhdawson closed this May 13, 2022
@vmoroz vmoroz deleted the PR/SetCDeclForFunctions branch May 15, 2022 22:58
BethGriggs pushed a commit that referenced this pull request May 16, 2022
PR-URL: #42780
Reviewed-By: Michael Dawson <midawson@redhat.com>
@RafaelGSS RafaelGSS mentioned this pull request May 16, 2022
@juanarbol
Copy link
Member

This is blocked by #42459

targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #42780
Reviewed-By: Michael Dawson <midawson@redhat.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42780
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants