WebAssembly / wasm-c-api

Wasm C API prototype
Apache License 2.0
550 stars 77 forks source link

Add WASM_C_API to declarations that may need declspec(dllimport/dllexport) on Windows #77

Closed AndrewScheidecker closed 5 years ago

AndrewScheidecker commented 5 years ago

This allows implementations to define WASM_C_API=__declspec(dllexport), and users to define WASM_C_API=__declspec(dllimport), which is the most concise way to import and export from a DLL on Windows.

jakobkummerow commented 5 years ago

I'm in favor of this PR; I believe this is required to make shared library builds work cleanly.

In fact, I'd consider going a step further to make it fully automatic on the user side, something like:

#ifndef WASM_C_API
#ifdef _WIN32
#define WASM_C_API __declspec(dllimport)
#else
#define WASM_C_API
#endif  // _WIN32
#endif  // WASM_C_API
AndrewScheidecker commented 5 years ago

In fact, I'd consider going a step further to make it fully automatic on the user side

I was thinking about this, but wasn't sure what the right predicate was. Is it _WIN32, or _MSC_VER?

jakobkummerow commented 5 years ago

Is it _WIN32, or _MSC_VER?

They're probably effectively equivalent. Strictly speaking, _MSC_VER means that the current compiler is MSVC, whereas _WIN32 means that the target OS is Windows (on any hardware: arm32, arm64, x86, or x64). MSVC definitely sets both; other compilers masquerading as MSVC (e.g. Clang-on-Windows, GCC-on-Cygwin, GCC-on-Mingw) should too.

V8 checks for _WIN32 when deciding whether to specify __declspec(dllimport), and that seems to work, so that's why I suggested that.

AndrewScheidecker commented 5 years ago

I pushed some changes:

AndrewScheidecker commented 5 years ago

I rebased this PR onto the latest master branch.

sbc100 commented 5 years ago

In my PR which was basically a complete dup of this although I simply used WASM_API. Similar to what we did with BINARYEN_API in binaryen. Does the _EXTERN add anything?

I initially used it to limit the exports from the linux shared object when working an a libwasm.so in wabt, but it served roughly the same purpose.

rossberg commented 5 years ago

@sbc100, somewhere upthread we discussed picking a name that is more symmetric to the already existing WASM_API_DEBUG. Other than that, no strong reason.

AndrewScheidecker commented 5 years ago

I rebased this PR on the latest master branch.

AndrewScheidecker commented 5 years ago

@rossberg FYI I don't have commit access on this repo, so I've been waiting for you to merge this.

rossberg commented 5 years ago

Ah, sorry, didn't realise that.