[WIP] Compile to buffer #3862

RichardAH · 2024-11-04T22:30:41Z

Hi guys!

We need this compile-to-buffer API for the Xahau blockchain (https://github.com/Xahau/xahaud ) which uses WasmEdge for smart contracts, specifically we want to use ahead of time compilation to cache execution of frequently used contracts.

I haven't had time to test, this is just a preliminary version of the PR. If someone else thinks this is a needed API change please feel free to assist.

…than to the filesystem

juntao · 2024-11-04T22:30:44Z

Hello, I am a code review agent on flows.network. Here are my reviews of changed source code files in this PR.

include/api/wasmedge/wasmedge.h

Potential issues

Issues:

Missing Include Guards:
- The source code does not include any include guards for the header file, which can lead to multiple inclusions if included more than once.
Inconsistent Naming Conventions:
- There is inconsistency in naming conventions used throughout the code, such as using WASMEDGE_CAPI_EXPORT in some places and WasmEdge_Result_Success with a mix of uppercase and lowercase.
Missing Function Definitions for C++ Compatibility:
- For C++ compatibility, the macros WasmEdge_Result_Success, etc., should be defined properly to avoid potential issues when using these constants in C++ code.

Summary of changes

Summary of Key Changes

Added New Functions:
- WasmEdge_CompilerCompileFromBufferToBuffer: Compiles a WebAssembly buffer and writes the result to another buffer.
- WasmEdge_CompilerCompileFromBytesToBytes: Compiles a WebAssembly WasmEdge_Bytes and stores the output in another WasmEdge_Bytes.
Deprecation Notice:
- The original function WasmEdge_CompilerCompileFromBuffer is marked for deprecation, suggesting future replacement by the new buffer-to-buffer compilation functions.

include/llvm/codegen.h

Potential issues

The codegen_buffer method's parameter OutByte is not marked as const, which could lead to unintended modifications of the output buffer by the caller.
The return type of codegen_buffer is Expect<void>, but it should ideally return an error code if there is an issue with writing to the output buffer, rather than throwing an exception.
The Conf member variable is not used within the class, which makes the parameter in the constructor redundant unless there are other methods or members using it.

Summary of changes

Added Buffer-based Compilation Method: Introduced a new method codegen_buffer that compiles LLVM Module into a buffer instead of writing to a file, returning the number of bytes written.
Modified Private Method Signature: Changed the private helper method codegen_osvec to return an LLVM::MemoryBuffer.
Added Forward Declaration for MemoryBuffer: Added a forward declaration for the MemoryBuffer class within the LLVM namespace.

lib/api/wasmedge.cpp

Potential issues

The code does not handle any potential errors that might occur during the compilation process.
There is no check for a nullptr when passing WasmEdge_ValueArr to fillWasmEdge_ValueArr, which could lead to undefined behavior if the pointer is null.
The genSpan functions do not check if the input buffer is null before returning a Span, which can lead to invalid memory accesses.

Summary of changes

Added a non-const version of genSpan function to handle mutable buffers.
Introduced two new C API functions: WasmEdge_CompilerCompileFromBufferToBuffer and WasmEdge_CompilerCompileFromBytesToBytes, which allow compiling WebAssembly code from an input buffer to an output buffer directly.
Implemented the compilation logic in WasmEdge_CompilerCompileFromBytesToBytes using existing methods, handling both LLVM and non-LLVM scenarios.

lib/llvm/codegen.cpp

Potential issues

Potential Memory Leak in outputWasmLibrary: The function outputWasmLibrary creates a temporary object file but does not clean up the intermediate .obj/.o file or the .lib file if it exists, leading to potential memory leaks.
Uninitialized Variable in getOSVersion: In the macOS implementation of getOSVersion, there is an issue where if the system call to get OS version fails, the function returns a default value but does not initialize the variables correctly.
Error Handling in codegen_buffer Function: The function codegen_buffer does not provide meaningful error handling and always returns an IllegalPath error when it encounters issues, which might make debugging harder without additional context.

Summary of changes

The function CodeGen::codegen has been renamed to CodeGen::codegen_osvec and now returns an LLVM::MemoryBuffer.
- A new function CodeGen::codegen_buffer has been added, which takes an output buffer and size pointer, writes the generated code to the buffer, and sets the size.
- The original codegen function has been updated to call codegen_osvec, handle the case where the result is invalid, and then write the generated code to the output file based on the compiler configuration.

test/api/APIAOTCoreTest.cpp

Potential issues

The function Compile is capturing the entire configuration context by value (auto Path = std::filesystem::u8path(Filename);), which could lead to unexpected behavior if ConfCxt is modified elsewhere.
In multiple places (e.g., in T.onModule, T.onLoad, etc.), the function Compile is called even before checking if the module or file needs to be recompiled, which can be inefficient.
There is no error handling for the case where WasmEdge_CompilerCompile fails and returns an invalid SOFilename. This could lead to subsequent operations on an uninitialized SOFilename.

Summary of changes

Added a new line after return SOPath; to separate it from the following code block.
- Introduced a lambda function for the T.onModule member variable, capturing local variables VM and Compile.
- Passed two parameters (ModName and Filename) to the lambda function, which are expected to be used within the function body.

hydai · 2024-11-05T03:45:30Z

include/api/wasmedge/wasmedge.h

@@ -1670,6 +1670,31 @@ WASMEDGE_CAPI_EXPORT extern WasmEdge_Result WasmEdge_CompilerCompileFromBuffer(
    WasmEdge_CompilerContext *Cxt, const uint8_t *InBuffer,
    const uint64_t InBufferLen, const char *OutPath);

+/// Compile the input WASM from the given buffer and output to another buffer.
+///
+/// CAUTION: This function will be deprecated and replaced by


If the following one will replace this function, why do we still need this now?

hydai · 2024-11-05T03:45:48Z

include/api/wasmedge/wasmedge.h

@@ -1685,6 +1710,24 @@ WASMEDGE_CAPI_EXPORT extern WasmEdge_Result
 WasmEdge_CompilerCompileFromBytes(WasmEdge_CompilerContext *Cxt,
                                  const WasmEdge_Bytes Bytes,
                                  const char *OutPath);
+/// Compile the input WASM from a WasmEdge_Bytes, and output it to a WasmEdge_Bytes.///


Extra /// at the end of the line.

And also lost an empty line.

hydai · 2024-11-05T03:46:42Z

include/llvm/codegen.h

 /// Compiling LLVM Module into loadable executable binary.
 class CodeGen {
 public:
  CodeGen(const Configure &Conf) noexcept : Conf(Conf) {}
  Expect<void> codegen(Span<const Byte> WasmData, Data D,
                       std::filesystem::path OutputPath) noexcept;
+  // returns number of bytes written to output buffer
+  Expect<void> codegen_buffer(Span<const Byte> WasmData,


It should be codegenBuffer.

I think codegenToBuffer is better.

hydai · 2024-11-05T03:47:45Z

include/llvm/codegen.h


 private:
+  Expect<LLVM::MemoryBuffer> codegen_osvec


Maybe codegenMemBuf or codegenOSVec is better?

hydai · 2024-11-05T03:48:20Z

include/llvm/codegen.h

 /// Compiling LLVM Module into loadable executable binary.
 class CodeGen {
 public:
  CodeGen(const Configure &Conf) noexcept : Conf(Conf) {}
  Expect<void> codegen(Span<const Byte> WasmData, Data D,
                       std::filesystem::path OutputPath) noexcept;
+  // returns number of bytes written to output buffer
+  Expect<void> codegen_buffer(Span<const Byte> WasmData,
+                       Data D, Span<const Byte> OutByte, // not actually const


This is weird. If the OutByte is not actually const, then we should not use const Byte here.

hydai · 2024-11-05T03:52:27Z

lib/llvm/codegen.cpp

+  std::filesystem::path LLPath(OutputPath);
+  LLPath.replace_extension("ll"sv);
+
+  auto result = codegen_osvec(WasmData, D);


Naming style: Result.

hydai · 2024-11-05T04:01:09Z

lib/llvm/codegen.cpp

+    // TODO:return error
+    // RH NOTE: the error values are all IllegalPath in this code when
+    // I got here, I'm not sure if I should change the enum and update them
+    // or if there's already a plan and scheme to do that...


Since we are having the new release for 0.15, it's fine to update the enum and add more ErrCode to show the real error value.

hydai · 2024-11-05T04:01:40Z

lib/llvm/codegen.cpp

+  // avoiding a copy here would also involve modifying LLVM::MemoryBuffer.
+  // Also trying to glue this function back to the CAPI proved annoying for
+  // const preservation, so here is an ugly const cast.
+  memcpy(const_cast<uint8_t*>(OutByte.data()), buf.data(), buf.size());


Prefer to use copy_n instead of any memcpy series function.

hydai · 2024-11-05T04:02:04Z

lib/llvm/codegen.cpp

  }

+  LLVM::MemoryBuffer& buf = *result;


Naming issue: Buf.

hydai · 2024-11-05T04:02:14Z

lib/llvm/codegen.cpp

+
+  *OutSize = 0;
+
+  auto result = codegen_osvec(WasmData, D);


Naming issue: Result.

RichardAH · 2024-11-09T05:06:46Z

Thanks for the reviews I'll get back to this hopefully in the coming week

RichardAH added 4 commits October 24, 2024 15:25

Add AOT CAPI that allows AOT compilation directly to a buffer rather …

b7cd594

…than to the filesystem

remove maybe_unused from capi

2c93c3f

add missing api

ab92efc

Merge branch 'WasmEdge:master' into compile-to-buffer

b95747e

RichardAH requested review from q82419 and ibmibmibm as code owners November 4, 2024 22:30

github-actions bot added c-CAPI An issue related to the WasmEdge C API c-Test An issue/PR to enhance the test suite labels Nov 4, 2024

hydai requested changes Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Compile to buffer #3862

[WIP] Compile to buffer #3862

RichardAH commented Nov 4, 2024

juntao commented Nov 4, 2024 •

edited

Loading

hydai Nov 5, 2024

hydai Nov 5, 2024

q82419 Nov 5, 2024

hydai Nov 5, 2024

q82419 Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

hydai Nov 5, 2024

RichardAH commented Nov 9, 2024

[WIP] Compile to buffer #3862

Are you sure you want to change the base?

[WIP] Compile to buffer #3862

Conversation

RichardAH commented Nov 4, 2024

juntao commented Nov 4, 2024 • edited Loading

Potential issues

Issues:

Summary of changes

Summary of Key Changes

Potential issues

Summary of changes

Potential issues

Summary of changes

Potential issues

Summary of changes

Potential issues

Summary of changes

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

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

RichardAH commented Nov 9, 2024

juntao commented Nov 4, 2024 •

edited

Loading