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

Build error on Windows #235

Closed
Tracked by #44650
targos opened this issue Jul 20, 2022 · 18 comments
Closed
Tracked by #44650

Build error on Windows #235

targos opened this issue Jul 20, 2022 · 18 comments

Comments

@targos
Copy link
Member

targos commented Jul 20, 2022

https://ci.nodejs.org/job/node-compile-windows-debug/12735/nodes=win-vs2019/console

14:33:41 C:\workspace\node-compile-windows-debug\node\deps\v8\src\wasm\wasm-disassembler-impl.h(77,52): error C2487: 'v8::internal::wasm::WasmDecoder<v8::internal::wasm::Decoder::kFullValidation,v8::internal::wasm::kFunctionBody>::StackEffect': member of dll interface class may not be declared with dll interface [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
@targos
Copy link
Member Author

targos commented Jul 20, 2022

@nodejs/platform-windows

@targos
Copy link
Member Author

targos commented Jul 20, 2022

This is actually not specific to debug builds.

@targos targos changed the title Build error on Windows (debug) Build error on Windows Jul 20, 2022
@targos
Copy link
Member Author

targos commented Jul 20, 2022

I just created an upstream issue: https://bugs.chromium.org/p/v8/issues/detail?id=13093

@targos
Copy link
Member Author

targos commented Aug 30, 2022

Unfortunately this is still happening...

https://github.com/nodejs/node-v8/runs/8087799918?check_suite_focus=true

@bnoordhuis
Copy link
Member

Bit of a long shot but does this help?

diff --git a/src/wasm/function-body-decoder-impl.h b/src/wasm/function-body-decoder-impl.h
index f8ca69d8485..5ef0eed5aed 100644
--- a/src/wasm/function-body-decoder-impl.h
+++ b/src/wasm/function-body-decoder-impl.h
@@ -2136,7 +2136,7 @@ class WasmDecoder : public Decoder {
   }
 
   // TODO(clemensb): This is only used by the interpreter; move there.
-  V8_EXPORT_PRIVATE std::pair<uint32_t, uint32_t> StackEffect(const byte* pc) {
+  std::pair<uint32_t, uint32_t> StackEffect(const byte* pc) {
     WasmOpcode opcode = static_cast<WasmOpcode>(*pc);
     // Handle "simple" opcodes with a fixed signature first.
     const FunctionSig* sig = WasmOpcodes::Signature(opcode);

WasmDecoder is a template class. Annotating members with __declspec(dllexport) doesn't seem appropriate.

@targos
Copy link
Member Author

targos commented Sep 15, 2022

@targos
Copy link
Member Author

targos commented Sep 15, 2022

@bnoordhuis looks good (arm64 build failed because of #240)!

@bnoordhuis
Copy link
Member

Nice. I'll open a V8 CL tomorrow.

@targos
Copy link
Member Author

targos commented Sep 20, 2022

@bnoordhuis can you please post a link to the V8 CL here?

@pbo-linaro
Copy link

Just a thought, but this might be related to nodejs build system layer (gyp).

V8_EXPORT_PRIVATE is defined here following other macros:

// Setup for Windows shared library export.
#ifdef BUILDING_V8_SHARED
#define V8_EXPORT_PRIVATE __declspec(dllexport)
#elif USING_V8_SHARED
#define V8_EXPORT_PRIVATE __declspec(dllimport)
#else
#define V8_EXPORT_PRIVATE
#endif  // BUILDING_V8_SHARED

Those macros are set in tools/v8_gypfiles/v8.gyp.

The solution to remove V8_EXPORT_PRIVATE for StackEffect function is equivalent to not declare BUILDING_V8_SHARED nor USING_V8_SHARED.

@pbo-linaro
Copy link

@targos I can confirm this compilation error is not present when building v8 directly. I'd suggest a fix around nodejs build system with macros explained above.

@targos
Copy link
Member Author

targos commented Sep 22, 2022

@pbo-linaro would you be able to identify exactly where the wrong macro is set?

@pbo-linaro
Copy link

I'll try to look at it, but that would be better if someone who worked on gyp/gn layer tackles this.
First, I'm working to upstream #240.

@pbo-linaro
Copy link

@targos: For now, how do you deal with custom patches that nodejs applies to v8? Are there kept somewhere?

@targos
Copy link
Member Author

targos commented Sep 22, 2022

I keep them on the canary-base branch (https://github.com/nodejs/node/tree/canary-base) which I rebase from time to time (usually when the daily update workflow of this repo fails because of a conflict)

@pbo-linaro
Copy link

You can safely remove that V8_EXPORT_PRIVATE in src/wasm/function-body-decoder-impl.h.
Another class inherits from the one defining this StackEffect function, and reuse V8_EXPORT_PRIVATE, which is forbidden by msvc (should be defined either at class or method level, but not both).

@pbo-linaro
Copy link

@targos After trying random stuff around node build system, I could not get something that was solving this issue. I suggest you keep a patch (on node side) removing that export for now.

@targos
Copy link
Member Author

targos commented Sep 23, 2022

Will do, thanks a lot for investigating!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants