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.

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

Jump to bottom

Misplaced/dead Windows code in node.cc? #52404

Closed
targos opened this issue Apr 7, 2024 · 5 comments
Closed

Misplaced/dead Windows code in node.cc? #52404

targos opened this issue Apr 7, 2024 · 5 comments
Labels
build Issues and PRs related to build files or the CI. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.

Comments

@targos
Copy link
Member

targos commented Apr 7, 2024

In

node/src/node.cc

Lines 633 to 640 in 756acd0

#if NODE_USE_V8_WASM_TRAP_HANDLER
#if defined(_WIN32)
{
constexpr ULONG first = TRUE;
per_process::old_vectored_exception_handler =
AddVectoredExceptionHandler(first, TrapWebAssemblyOrContinue);
}
#else
, we have Windows-specific code.
The problem is that this code is inside a #ifdef __POSIX__ section:

node/src/node.cc

Line 553 in 756acd0

#ifdef __POSIX__

This seems to be unexpected dead code for Windows.
I found this from a compiler warning: src\node.cc(428,13): warning : unused function 'TrapWebAssemblyOrContinue' [-Wunused-function] [D:\Git\nodejs\node\libnode.vcxproj]
@targos targos added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Apr 7, 2024
@targos
Copy link
Member Author

targos commented Apr 8, 2024

The code was added in 0577127 and I think it's always been inside the __POSIX__ section.

@targos
Copy link
Member Author

targos commented Apr 11, 2024

@nodejs/cpp-reviewers

@tniessen
Copy link
Member

cc @devsnek

@devsnek
Copy link
Member

devsnek commented Apr 11, 2024

nice catch. given i've never had a windows development machine i would assume i was relying on our windows ci when i wrote the linked commit. i guess since the trap handler was never installed and V8:EnableWebAssemblyTrapHandler(false); is also inside the POSIX, the entire commit was effectively no-op. if anyone cares about wasm performance on windows it could be nice to fix.

@joyeecheung joyeecheung added the good first issue Issues that are suitable for first-time contributors. label Apr 13, 2024
@thisalihassan
Copy link
Contributor

I will make a PR for this

thisalihassan added a commit to thisalihassan/node that referenced this issue Apr 15, 2024
@thisalihassan
src: remove misplaced windows code under posix guard in node.cc
83a1b8b 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 checks.
This fix will ensure that the intended exception handling is active on
Windows builds, potentially improving WebAssembly performance.

Fixes: nodejs#52404
Refs: nodejs#35033
thisalihassan added a commit to thisalihassan/node that referenced this issue Apr 15, 2024
@thisalihassan
src: remove misplaced windows code under posix guard in node.cc
bb59cee 
The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds.
Fixes: nodejs#52404
Refs: nodejs#35033
thisalihassan added a commit to thisalihassan/node that referenced this issue Apr 15, 2024
@thisalihassan
src: remove misplaced windows code under posix guard in node.cc
2b3d7b0 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: nodejs#52404
Refs: nodejs#35033
@thisalihassan thisalihassan mentioned this issue Apr 15, 2024
Closed
aduh95 pushed a commit that referenced this issue Apr 30, 2024
@thisalihassan @aduh95
src: remove misplaced windows code under posix guard in node.cc
0c58d03 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: #52404
Refs: #35033
PR-URL: #52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ch3nYuY pushed a commit to Ch3nYuY/node that referenced this issue May 8, 2024
@thisalihassan
src: remove misplaced windows code under posix guard in node.cc
b244ed1 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: nodejs#52404
Refs: nodejs#35033
PR-URL: nodejs#52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
marco-ippolito pushed a commit that referenced this issue May 23, 2024
@thisalihassan @marco-ippolito
src: remove misplaced windows code under posix guard in node.cc
07c45ba 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: #52404
Refs: #35033
PR-URL: #52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
joyeecheung pushed a commit to joyeecheung/node that referenced this issue May 28, 2024
@thisalihassan @joyeecheung
src: remove misplaced windows code under posix guard in node.cc
98606b2 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: nodejs#52404
Refs: nodejs#35033
PR-URL: nodejs#52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
@thisalihassan @marco-ippolito
src: remove misplaced windows code under posix guard in node.cc
d02907e 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: #52404
Refs: #35033
PR-URL: #52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
@thisalihassan @eliphazbouye
src: remove misplaced windows code under posix guard in node.cc
92b30e5 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: nodejs#52404
Refs: nodejs#35033
PR-URL: nodejs#52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
@thisalihassan @bmeck
src: remove misplaced windows code under posix guard in node.cc
f610e0d 
The V8 WebAssembly trap handler setup for Windows was incorrectly
nested within a POSIX conditional compilation block in src/node.cc.
This caused the related functions to be effectively non-operational
on Windows. The changes involve removing the Windows-specific code from
the POSIX section and correctly placing it under the WIN32 check.
This fix will ensure that the intended exception handling is active
on Windows builds.

Fixes: nodejs#52404
Refs: nodejs#35033
PR-URL: nodejs#52545
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. good first issue Issues that are suitable for first-time contributors. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

src: remove misplaced windows code under posix guard in node.cc thisalihassan/node
5 participants
@targos @tniessen @joyeecheung @devsnek @thisalihassan