Add C/C++ bindings #645

TheGreatRambler · 2022-06-24T06:22:31Z

This PR adds two exported functions, along with a header, through Rust FFI so that other languages can generate random modules for fuzzing as well.

struct WasmSmithModule {
	uint8_t* data;
	uint32_t size;
};

enum WasmSmithResult { NOT_ENOUGH_ENTROPY = -2, NULL_PTR = -1, SUCCESS = 0 };

int32_t wasm_smith_create(const char* seed, WasmSmithModule* c_output);
int32_t wasm_smith_free(WasmSmithModule* module);

An example Cmake script for linking:

include(ExternalProject)
set_directory_properties(PROPERTIES EP_PREFIX ${CMAKE_BINARY_DIR}/wasm-tools)
ExternalProject_Add(
	wasm-smith
	DOWNLOAD_COMMAND ""
	CONFIGURE_COMMAND ""
	BUILD_COMMAND cargo build
	BINARY_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../third_party/wasm-tools/crates/wasm-smith"
	INSTALL_COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_SOURCE_DIR}/../third_party/wasm-tools/target/debug/wasm_smith.dll ${CMAKE_BINARY_DIR}/wasm_smith.dll
	LOG_BUILD ON
	BUILD_ALWAYS ON)
add_dependencies(test wasm-smith)

...

target_include_directories(test PUBLIC include src ${CMAKE_CURRENT_SOURCE_DIR}/../third_party/wasm-tools/crates/wasm-smith/include)
target_link_libraries(test PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/../third_party/wasm-tools/target/release/wasm_smith.dll.lib)

While building in ./build with wasm-tools in ./third_party/wasm-tools.

Usage is as follows:

WasmSmithModule module;
std::string test_seed("THIS IS A TEST SEED, MUST BE NULL TERMINATED");
if(!wasm_smith_create(test_seed.c_str(), &module)) {
	std::vector<uint8_t> data(module.data, module.data + module.size);
	// Do with the module what you want
	wasm_smith_free(&module);
}

TheGreatRambler · 2022-06-24T06:27:27Z

Hmm I'm new to Rust (this PR is because I'm implementing testing for a C++ library), is the failure of some tests due to crate-type = ["cdylib"]?

alexcrichton · 2022-06-24T14:37:01Z

Thanks for this! Adding a C API seems reasonable to me, although I don't think we'd want to do it precisely this way. Instead I think we'd probably want to do something similar to Wasmtime itself where the C API is a separate crate (e.g. under crates/c-api) and has its own area for implementing APIs. I think it might also make sense to make this something like libwasmtools instead of libwasm-smith to enabling exposing other functionality as part of the C API as well (e.g. wasmparser's validate function, wat's text parsing, wasmprinter's binary parsing, etc.)

Would you be up for splitting this C API to a separate crate? It's ok to not expose functionality from other crates for now and we can fill that in later. Otherwise though when creating a C API I think there's a few conventions we also want to handle:

Naming-wise we need a name and a prefix for all structs/types.
Convention wise I think we'll want to stick to lower_snake_case for both structs and functions (similar to the Wasmtime C API)
We'll want documentation for the functions which for now I think is best placed in the headers which can explain what the functions do, the ownership semantics, etc. Some top-level documentation can also clarify how to build/link the C API for projects too.
This'll want to be tested on Windows/macOS/Linux as well. The Wasmtime repository has a few example C programs which exercise the C API and having a similar scheme I think would be fine here.

Eventually longer-term I think we'll want binary released for the C API as well but that can wait too. I apologize if this seems like a lot, but supporting a C API "officially" is not a trivial thing to do unfortunately and there's a surprising amount of ground to cover to ensure it works well.

TheGreatRambler · 2022-06-24T16:35:32Z

Would crates/c-api be a good place to put it for now?

alexcrichton · 2022-06-24T18:06:21Z

If by crates/c-api you mean the Wasmtime repository itself I could go either way on that. If you instead mean in this repository itself then yes I think it's fine to move it over to there.

TheGreatRambler · 2022-06-24T22:17:18Z

Oh I wasn't aware that's exactly how wasmtime does it, I'll make sure to model this off of wasmtime

TheGreatRambler · 2022-06-25T01:18:52Z

How does wasmtime format its headers? I want to commit something with a consistent style

TheGreatRambler · 2022-06-27T00:18:24Z

I've moved my bindings into the c-api crate and provided a CMakeLists.txt to build them, as well as an examples folder to show how to build them

TheGreatRambler · 2022-06-27T01:13:44Z

Doxygen comments are in the header, but I'm waiting for your input on that point

alexcrichton

Thanks for this! This is all looking pretty good to me. One thing though, can you update CI to build the C API and build/test the example program here as well?

I'm not sure how best to integrate the cmake bits here since I've never managed a cmake project myself for something like this. If possible though I think it would be good to avoid placing the cmake bits at the root of the project to avoid mistaking which build system is needed to work with wasm-tools.

alexcrichton · 2022-06-27T14:11:48Z

crates/c-api/src/lib.rs

+        }
+    }
+
+    wasm_tools_error::WASM_TOOLS_INVALID_POINTER


Personally API-wise I think it would be best to remove this error variant, assume everything is non-null, and then proceed as usual. I think it's better to document that this shouldn't be null instead of handling it.

Changed it so the function receives a reference like wasmtime does, the calling C code crashes when it passes a null pointer, I guess that's what we want?

crates/c-api/src/lib.rs

alexcrichton · 2022-06-27T14:14:45Z

CMakeLists.txt

+	# TODO linux
+endif()
+
+target_include_directories(wasm-tools INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/crates/c-api/include)


Currently we don't use cmake to build the Rust code itself, so is this necessary to add? This seems somewhat nontrivial and at least personally I have no idea how to maintain cmake code myself.

I thought exposing the bindings to other code through the Cmake ecosystem would be more convenient, I personally used ExternalProject_Add when building before so anyone using the library would probably use the same solution. Since there's also a Cargo.toml I thought developers would recognize one is used for Rust and the other for C bindings

Though I can see the value of moving the CMakeLists.txt into crates/c-api, it would be the same other than different relative paths

crates/c-api/src/lib.rs

alexcrichton

If possible I think it's probably best to keep the new cmake files (and possibly examples too?) in the new crates/c-api directory. We can always lift them up later of course depending on how things go.

examples/main.c

crates/c-api/src/lib.rs

alexcrichton · 2022-06-28T14:52:06Z

crates/c-api/src/lib.rs

+            bytes.data = std::ptr::null_mut();
+            wasm_tools_error::WASM_TOOLS_INSUFFICIENT_ENTROPY
+        }
+        Err(_e) => {


Could the other error variants get matched here and translated to an error enum?

Done. I'd like some examples of when those other two errors would be thrown so I can better document them in the header

Ah I think it's ok to not exhaustively document, we may not be hitting the other error cases for now anyway.

crates/c-api/src/lib.rs

alexcrichton

Looking good, thanks again! I think with the macOS build failure resolved as well as minor comments below this should be good to land.

alexcrichton · 2022-06-29T15:17:36Z

crates/c-api/CMakeLists.txt

+# Copy shared library into build directory
+if(WIN32)
+	set(WASM_TOOLS_INSTALL_COMMAND ${CMAKE_COMMAND} -E copy_if_different
+		${CMAKE_CURRENT_SOURCE_DIR}/../../target/release/wasmtools.dll
+		${CMAKE_BINARY_DIR})
+else()
+	set(WASM_TOOLS_INSTALL_COMMAND ${CMAKE_COMMAND} -E copy_if_different
+		${CMAKE_CURRENT_SOURCE_DIR}/../../target/release/libwasmtools.so
+		${CMAKE_BINARY_DIR})
+endif()
+


I think this'll need a case for macOS and libwasmtools.dylib

alexcrichton · 2022-06-29T15:18:13Z

crates/c-api/CMakeLists.txt

+if(WIN32)
+	target_link_libraries(wasm-tools INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/../../target/release/wasmtools.dll.lib)
+else()
+	target_link_libraries(wasm-tools INTERFACE ${CMAKE_CURRENT_SOURCE_DIR}/../../target/release/libwasmtools.so)
+	set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-rpath='$ORIGIN'")
+endif()


Similar to above I think this'll need a libwasmtools.dylib case?

crates/c-api/src/lib.rs

alexcrichton · 2022-06-29T15:19:53Z

crates/c-api/src/lib.rs

+            bytes.data = std::ptr::null_mut();
+            wasm_tools_error::WASM_TOOLS_INSUFFICIENT_ENTROPY
+        }
+        Err(_e) => {


Ah I think it's ok to not exhaustively document, we may not be hitting the other error cases for now anyway.

crates/c-api/src/lib.rs

examples/wasm-smith.rs

examples/CMakeLists.txt

alexcrichton · 2022-06-30T15:27:28Z

Looks great to me, thanks again for this! I appreciate all the work into making the initial infrastructure for this, and it may even end up making sense to lift some of these cmake bits into Wasmtime...

* Add wasm-smith C/C++ bindings * Add initial c-api * Add CMake support and examples * Add main function to examples * Remove references to wasmtime in doxygen.conf * Handle insufficent entropy in c-api and add CI * Fix c-api CI error * Simplify wasm_tools_byte_vec_delete * Move c-api CMakeLists into c-api crate * Suppress examples build errors * Add macos support to c-api building * Fix windows c-api build * Fix c-api type in readme

Add wasm-smith C/C++ bindings

9501515

TheGreatRambler added 2 commits June 26, 2022 02:10

Add initial c-api

f983196

Add CMake support and examples

8c620cd

TheGreatRambler changed the title ~~Add wasm-smith C/C++ bindings~~ Add C/C++ bindings Jun 27, 2022

Add main function to examples

471723f

Remove references to wasmtime in doxygen.conf

c30d883

alexcrichton reviewed Jun 27, 2022

View reviewed changes

TheGreatRambler added 3 commits June 27, 2022 20:07

Handle insufficent entropy in c-api and add CI

71762b4

Fix c-api CI error

c32c622

Simplify wasm_tools_byte_vec_delete

c21ac70

alexcrichton reviewed Jun 28, 2022

View reviewed changes

TheGreatRambler added 2 commits June 28, 2022 14:34

Move c-api CMakeLists into c-api crate

78431eb

Suppress examples build errors

24d2e14

alexcrichton reviewed Jun 29, 2022

View reviewed changes

Add macos support to c-api building

796d133

TheGreatRambler commented Jun 29, 2022

View reviewed changes

examples/CMakeLists.txt Outdated Show resolved Hide resolved

TheGreatRambler added 2 commits June 29, 2022 19:48

Fix windows c-api build

0839b32

Fix c-api type in readme

ae976ac

alexcrichton merged commit 77f1718 into bytecodealliance:main Jun 30, 2022

TheGreatRambler deleted the cpp-bindings branch July 29, 2022 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C/C++ bindings #645

Add C/C++ bindings #645

TheGreatRambler commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

alexcrichton commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

alexcrichton commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

TheGreatRambler commented Jun 25, 2022

TheGreatRambler commented Jun 27, 2022

TheGreatRambler commented Jun 27, 2022

alexcrichton left a comment

alexcrichton Jun 27, 2022

TheGreatRambler Jun 28, 2022

alexcrichton Jun 27, 2022

TheGreatRambler Jun 28, 2022

TheGreatRambler Jun 28, 2022

alexcrichton left a comment

alexcrichton Jun 28, 2022

TheGreatRambler Jun 28, 2022

alexcrichton Jun 29, 2022

alexcrichton left a comment

alexcrichton Jun 29, 2022

alexcrichton Jun 29, 2022

alexcrichton Jun 29, 2022

alexcrichton commented Jun 30, 2022

Add C/C++ bindings #645

Add C/C++ bindings #645

Conversation

TheGreatRambler commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

alexcrichton commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

alexcrichton commented Jun 24, 2022

TheGreatRambler commented Jun 24, 2022

TheGreatRambler commented Jun 25, 2022

TheGreatRambler commented Jun 27, 2022

TheGreatRambler commented Jun 27, 2022

alexcrichton left a 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

alexcrichton left a 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

alexcrichton left a 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

alexcrichton commented Jun 30, 2022