WasmEdge icon indicating copy to clipboard operation
WasmEdge copied to clipboard

add initial support for FreeBSD

Open vigsterkr opened this issue 8 months ago • 2 comments

this patch is mostly based on @rusty-eagle (kudos for this!) see: https://github.com/WasmEdge/WasmEdge/issues/3145#issuecomment-187960107:0

  • [ ] fix failing unit tests
  • [ ] add CI job for FreeBSD target

vigsterkr avatar Apr 16 '25 20:04 vigsterkr

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


include/common/defines.h

Potential issues

  1. Incorrect FreeBSD macro definitions: The macros for FreeBSD include _FREEBSD and __FREEBSD__, which are not standard. They should be only __FreeBSD__.

  2. Inconsistent library extension for FreeBSD: While FreeBSD uses .so for shared libraries, the code defines WASMEDGE_LIB_EXTENSION as .so but could benefit from an explicit definition to avoid confusion.

  3. Error in export macro for Windows: The WASMEDGE_EXPORT macro uses a GNU-specific attribute [[gnu::visibility("default")]] which is not portable on other compilers, including MSVC used by default on Windows. It should conditionally use __declspec(dllexport) for Windows and [[gnu::visibility("default")]] for other platforms.

Summary of changes

    • Added support for FreeBSD by defining WASMEDGE_OS_FREEBSD in preprocessor directives.
  • Included conditions to detect FreeBSD using __FreeBSD__, _FREEBSD, or __FREEBSD__.
  • Set library prefix and extension for FreeBSD to "lib" and ".so", similar to Linux.

include/common/endian.h

Potential issues

  1. The #undef directive before redefining WASMEDGE_ENDIAN_LITTLE_BYTE is unnecessary since WASMEDGE_OS_FREEBSD is mutually exclusive with the previous conditions.
  2. There's no handling for big-endian systems other than Linux, which could lead to incorrect assumptions on FreeBSD if it were ever configured in a big-endian mode (though unlikely).
  3. The use of #define for conditional compilation can be error-prone and harder to debug; consider using constexpr or inline functions for such logic if possible.

Summary of changes

  • Key Changes:
  • Extended Endianness Definition: Added WASMEDGE_OS_FREEBSD to the condition that defines WASMEDGE_ENDIAN_LITTLE_BYTE, ensuring FreeBSD systems are recognized as little-endian.
  • OS Support Expansion: Introduced initial support for FreeBSD by modifying preprocessor conditions, indicating compatibility or specific behavior adjustments for this OS.
  • Consistency in Endianness Handling: Ensured consistent endianness handling across macOS and FreeBSD by extending the existing macro definition logic.

include/host/wasi/inode.h

Potential issues

  1. Issue 1: Incorrect preprocessor directive in Poller class.

    • The line #if WASMEDGE_OS_LINUX | WASMEDGE_OS_MACOS || WASMEDGE_OS_FREEBSD uses a single pipe (|) instead of double pipes (||). This could lead to incorrect conditional compilation.
  2. Issue 2: Missing closing brace in Poller class.

    • The code snippet ends abruptly with an incomplete line std::unordered_, which is likely meant to be part of a larger structure or function and requires a closing brace for proper compilation.
  3. Issue 3: Potential resource management issue in DirHolder.

    • In the destructor ~DirHolder() noexcept { reset(); } and member function reset() noexcept;, it's assumed that reset() correctly handles resource deallocation. If DIR *Dir is not properly closed with closedir(Dir), this would lead to resource leaks. Ensure reset() implementation calls closedir if Dir is valid.

Summary of changes

    • Added support for FreeBSD by including WASMEDGE_OS_FREEBSD in preprocessor directives.
  • Updated FdHolder, INode, and Poller classes to conditionally include FreeBSD in their definitions.
  • Modified the Poller class to handle FreeBSD-specific event handling mechanisms, similar to macOS.

lib/host/wasi/CMakeLists.txt

Potential issues

  1. Issue 1: The variable vinode.cpp is included in the wasmedge_add_library command, but it seems to be a typo and should likely be inode.cpp to match other source files.
  2. Issue 2: The condition for linking rt library checks if the platform is not Apple, not Windows, and not Android, which implies all Unix-like systems including FreeBSD. This might unintentionally include systems that do not require rt.
  3. Issue 3: There are no specific conditions to handle FreeBSD specifically; it falls under the default else case (not APPLE AND NOT WIN32) which includes inode-unix.cpp. If FreeBSD requires different sources or libraries, this should be explicitly handled.

Summary of changes

    • Updated WASMEDGE_WASI_SRCS for Unix systems: Changed the source files to use clock-unix.cpp, environ-unix.cpp, and inode-unix.cpp instead of Linux-specific ones, broadening support.
  • Added FreeBSD compatibility: By using generic Unix sources, the patch implicitly adds initial support for FreeBSD.
  • Generalized platform handling: This change ensures that non-Windows and non-Mac platforms use a common set of Unix-compatible source files.

lib/host/wasi/clock-unix.cpp

Potential issues

  1. Unused Parameter: In clockTimeGet, the second parameter __wasi_timestamp_t is declared but never used, which can lead to confusion and should be removed or utilized.
  2. Error Handling Inconsistency: The use of unlikely(Res != 0) for error checking in both functions might not clearly convey intent; consider using a more descriptive macro or inline comment explaining the error handling strategy.
  3. Potential Data Loss: Conversion from timespec to __wasi_timestamp_t via fromTimespec could result in precision loss if __wasi_timestamp_t does not support nanosecond precision as timespec. Ensure that fromTimespec handles this appropriately.

Summary of changes

    • Extended OS Support: Added FreeBSD to the list of supported operating systems in preprocessor directives.
  • Header File Change: Replaced linux.h with a more generic unix.h to accommodate both Linux and FreeBSD.
  • Namespace Usage: Ensured that changes do not alter existing namespace usage, maintaining consistency.

lib/host/wasi/environ-unix.cpp

Potential issues

  1. SIGPOLL and SIGPWR Usage: SIGPOLL and SIGPWR are conditionally defined only for Linux, which may cause build issues on FreeBSD if these signals are used without similar conditions.
  2. Error Handling in schedYield: The sched_yield() function does not return a value indicating success or failure, so checking its result is unnecessary and misleading.
  3. Potential Missing Signal Mapping: If __WASI_SIGNAL_POLL or __WASI_SIGNAL_PWR are expected to be used on FreeBSD, their mappings should be added within a conditional block for FreeBSD as well, similar to how they are handled for Linux.

Summary of changes

    • Added FreeBSD support by modifying the preprocessor directive to include FreeBSD in addition to Linux.
  • Changed the included header from linux.h to unix.h to generalize for Unix-like systems, including FreeBSD.
  • Wrapped signal definitions (SIGPOLL and SIGPWR) with a Linux-specific preprocessor directive to prevent compilation issues on FreeBSD.

lib/host/wasi/inode-freebsd.cpp

Potential issues

  1. Incorrect fdSeek Functionality: The fdSeek function does not handle all valid __wasi_whence_t values (AT_SET, AT_CUR, AT_END). It should be updated to include these cases using a mapping or switch statement for better accuracy and robustness.

  2. Potential Data Corruption in sockSendTo: The function sockSendTo uses MSG_NOSIGNAL which is conditionally defined but not universally available on all systems. If the system does not define MSG_NOSIGNAL, it sets SysSiFlags to 0, potentially leading to signals being raised during send operations and causing undefined behavior if unhandled.

  3. Improper Error Handling in sockAddressAssignHelper: The function assumes that the size of Address is sufficient for the address family without proper validation (assuming(Address.size() >= sizeof(in_addr));). This can lead to buffer overflows or incomplete data being copied, causing undefined behavior or security vulnerabilities.

Summary of changes

    • Added FreeBSD Support: The patch introduces initial support for FreeBSD by including necessary headers and defining OS-specific constants, functions, and classes.
  • File Descriptor Management: Implemented file descriptor management specific to FreeBSD, including opening, reading, writing, seeking, and closing files.
  • Socket Operations: Added socket operations such as binding, listening, accepting connections, sending, receiving data, and setting options, tailored for FreeBSD's system calls.

lib/host/wasi/inode-unix.cpp

Potential issues

  1. The #error directive will halt compilation without a descriptive message, making it difficult to diagnose why the build failed.
  2. Including .cpp files directly in headers can lead to multiple definition errors if the header is included in multiple translation units.
  3. Missing #endif at the end of the file could cause issues in environments where this code might be concatenated or included conditionally.

Summary of changes

    • Added conditional inclusion for FreeBSD-specific code.
  • Included inode-freebsd.cpp when building on FreeBSD systems.
  • Ensured an error is generated for unsupported operating systems.

lib/host/wasi/unix.h

Potential issues

  1. Duplicate Inclusions and Definitions: The #include <sys/stat.h> directive appears twice within the FreeBSD section, leading to potential compilation issues due to redefinition of macros or functions.
  2. Missing Constants for FreeBSD in fromErrNo Function: The fromErrNo function does not handle FreeBSD-specific error codes such as EPROGMISMATCH, ESTRPIPE, etc., which could lead to incorrect error mappings on FreeBSD systems.
  3. epoll Exclusivity for Linux: The Linux section includes <sys/epoll.h>, which is not available on FreeBSD, leading to compilation errors unless conditionally included or replaced with a compatible FreeBSD alternative like kqueue.

Summary of changes

  • Key Changes:

  • FreeBSD Support Addition: Added conditional compilation for FreeBSD, allowing the codebase to compile and run on FreeBSD operating systems.

  • Error Mapping Functions: Introduced functions to map between FreeBSD-specific error codes and WASI error codes, ensuring compatibility with WASI's error handling.

  • Utility Conversion Functions: Included utility functions to convert various data types (e.g., timestamps, file types) between WASI and FreeBSD representations.

lib/llvm/codegen.cpp

Potential issues

  1. Issue in getOSVersion() for FreeBSD: The function is intended to parse and format the OS version string, which works correctly for macOS. However, the logic inside this function will not work on FreeBSD because the kernel version string on FreeBSD does not follow the same semantic versioning pattern as macOS. This can lead to incorrect parsing and formatting of the OS version.

  2. Missing getOSVersion() Implementation for FreeBSD: The code includes a placeholder TODO comment regarding the SDK version for macOS but does not provide any mechanism or implementation to retrieve either the OS or SDK version on FreeBSD. This is crucial as it affects the linking process, which relies on this information.

  3. Hardcoded File Extensions for Temporary Files: The createTemp() function uses hardcoded file extensions ("%%%%%%%%%%.o" for Linux and FreeBSD vs "%%%%%%%%%%.obj" for Windows). While this might work in the context of creating temporary files that are later linked, it can lead to confusion if these intermediate files are used outside the expected linking process, especially since .o and .obj serve similar purposes but have different expectations in their contents and use cases across systems.

Summary of changes

    • Added support for FreeBSD by including it in the conditional compilation checks.
  • Updated SYMBOL macro and driver handling to accommodate FreeBSD alongside Linux and macOS.
  • Assigned a unique identifier (UINT8_C(4)) for FreeBSD when writing byte codes, distinguishing it from other operating systems.

lib/loader/ast/section.cpp

Potential issues

  1. Potential Overflow in HostOSType: The return value for UINT8_C(-1) is not valid since uint8_t cannot represent negative values, leading to undefined behavior.

  2. Error Logging in loadSection(AST::AOTSection &Sec): Using spdlog::info to log errors can be misleading as it does not indicate the severity of the issue; spdlog::error should be used for error messages instead.

  3. Inconsistent Error Handling: The error handling mechanism in loadSection(AST::AOTSection &Sec) logs errors using spdlog::info and then returns an Unexpect(ErrCode::Value::...), which is inconsistent with the detailed error logging elsewhere that uses spdlog::error. This discrepancy may confuse users and developers about the severity of logged messages.

Summary of changes

    • Added WASMEDGE_OS_FREEBSD macro to support FreeBSD.
  • Assigned UINT8_C(4) for FreeBSD in the HostOSType function.
  • Updated the default return value comment to reflect ongoing OS support.

lib/loader/shared_library.cpp

Potential issues

  1. Issue: The ::dlopen function call in the load method does not handle Path.c_str() correctly on FreeBSD, as it expects a C-style string that might not be properly handled by std::filesystem::path::c_str().
  2. Issue: The error message logging for failed library loading is inconsistent between Windows and other systems (WASMEDGE_OS_LINUX, WASMEDGE_OS_MACOS, WASMEDGE_OS_FREEBSD). Specifically, on non-Windows systems, the error message from dlerror() might be truncated or not formatted as intended.
  3. Issue: The use of winapi::LPSTR_ and related macros in the Windows-specific code is unclear and might lead to issues with type safety and compatibility if these are custom wrappers that do not directly map to standard Windows types.

Summary of changes

    • Added FreeBSD Support: Included FreeBSD in the conditional compilation to support dynamic linking functionality.
  • Updated Conditional Check: Modified the preprocessor directives to recognize WASMEDGE_OS_FREEBSD alongside Linux and macOS.
  • Error Handling: Ensured that unsupported operating systems trigger an error, maintaining robust configuration management.

lib/system/CMakeLists.txt

Potential issues

  1. Incorrect Directory Variable: target_include_directories uses ${CMAKE_CURRENT_BINARY_DIR} which typically contains generated files, not source headers. Use ${CMAKE_CURRENT_SOURCE_DIR} or a dedicated include directory instead.

  2. Lack of FreeBSD-Specific Source Files: The patch adds support for linking execinfo on FreeBSD but does not account for any FreeBSD-specific source files or configurations that might be necessary.

  3. No Error Handling for Unsupported Platforms: There is no error handling or fallback mechanism if the platform is neither Windows nor FreeBSD, potentially leaving other platforms without proper configuration or dependencies.

Summary of changes

    • Added support for FreeBSD: The patch introduces conditional logic to detect the FreeBSD system.
  • Linked execinfo library: For FreeBSD, it links the execinfo library which is used for stack traces and backtracing functionality.
  • Integrated with existing CMake configuration: This change ensures that when building on FreeBSD, specific libraries are included without affecting other platforms.

lib/system/allocator.cpp

Potential issues

  1. Incorrect Macro Definition for FreeBSD: The macro MAP_NORESERVE is incorrectly defined as MAP_RESERVED0040 on FreeBSD, which is not a valid flag. It should be defined appropriately or omitted if the flag is unsupported.

  2. Misuse of k12G on Non-MMAP Platforms: The constant k12G is used in code that doesn’t involve mmap, specifically in platforms where memory allocation relies on std::malloc. This usage can lead to confusion and potential errors if the value is mistakenly interpreted or used incorrectly.

  3. Lack of Error Handling in allocate_chunk: The allocate_chunk function for non-MMAP platforms lacks error handling when calling std::malloc. If std::malloc fails, it returns nullptr, but this should be checked and handled to maintain consistency with the other branches.

Summary of changes

    • Added a preprocessor directive to define MAP_NORESERVE as MAP_RESERVED0040 for FreeBSD, addressing the difference in memory mapping flags.
  • Included an OS-specific conditional compilation check (WASMEDGE_OS_FREEBSD) to ensure the definition is applied only when building on FreeBSD.
  • Introduced comments explaining the reason for the change, indicating that FreeBSD does not define MAP_NORESERVE.

lib/system/stacktrace.cpp

Potential issues

  1. Incorrect Use of Func->getInstrs().end() as Key: In the function interpreterStackTrace, using Instrs.end() as a key in the std::map Funcs is incorrect because map keys must be valid and comparable, but Instrs.end() is an invalid iterator that cannot be used as a key. This will lead to undefined behavior.

  2. Potential Undefined Behavior with SymGetModuleBase64: In the Windows-specific part of stackTrace, SymGetModuleBase64 is called with addresses of functions like RtlCaptureStackBackTrace. If these symbols are not found or resolved correctly, SymGetModuleBase64 can return 0, which might not be checked properly, leading to potential undefined behavior when comparing module bases.

  3. Assumption on Stack Frame Size in compiledStackTrace: The function compiledStackTrace uses a fixed-size stack buffer (StackTraceBuffer) of 256 elements. This assumption may fail if the actual stack depth exceeds this size, potentially truncating the stack trace and leading to incomplete or incorrect stack information.

Summary of changes

    • Added FreeBSD Support: Included FreeBSD in the preprocessor conditions for both #include <execinfo.h> and the stack trace handling logic, allowing compatibility with FreeBSD systems.
  • Modified Stack Trace Logic: Updated the stack trace retrieval to handle FreeBSD by using the same method as macOS, leveraging backtrace() from <execinfo.h>.
  • Consolidated Conditional Statements: Simplified the code by combining conditions for macOS and FreeBSD, reducing redundancy.

plugins/wasmedge_process/processfunc.cpp

Potential issues

  1. Memory Handling in Argument Conversion: The execve and execvpe system calls use raw pointers (char *) from strings that may go out of scope when the lambda captures expire, causing undefined behavior.
  2. Error Handling in Pipe Creation: If any pipe creation fails, the function returns Env.ExitCode, which is initialized to -1. This value should be a defined error code instead for clarity.
  3. Inconsistent Use of spdlog: The spdlog::error calls use string literals with the sv suffix, but there are instances where plain strings are used without checks or handling, leading to potential inconsistencies in logging style and functionality.

Summary of changes

    • Added FreeBSD Support in Conditional Compilation: Included support for FreeBSD by adding WASMEDGE_OS_FREEBSD to the conditional checks.
  • Modified Includes for FreeBSD: Ensured necessary headers (errno.h, fcntl.h, etc.) are included when building on FreeBSD.
  • Adjusted Function Body for FreeBSD Compatibility: Modified the function body of WasmEdgeProcessRun::body() to include FreeBSD in the supported operating systems.

test/host/socket/wasi_socket.cpp

Potential issues

  1. IPv6 Socket Initialization Issue: The TrySetUpIPV6Socket function uses a hardcoded port number (10000) without ensuring its availability, which can lead to test failures if the port is already in use. Consider using an ephemeral port or checking for port availability.

  2. Memory Overwriting Risk in allocateAddrinfoArray: The calculation of Base assumes that all allocated structures fit into the memory instance without exceeding its bounds. This could lead to buffer overflows if Length is too large or CanonnameMaxSize is improperly managed. Validate that the total memory required does not exceed the available memory size.

  3. Redundant Socket Opening in Tests: In multiple test cases, a third call to WasiSockOpen.run is made with the same parameters (AddressFamily, SockType, FdClientPtr). This redundant call should be removed as it does not change the state and could confuse test readers.

Summary of changes

    • Added sys/socket.h include: This change is crucial as it ensures compatibility with FreeBSD's socket system calls.
  • Extended AF_UNIX support to FreeBSD: The modification in the conditional compilation directive allows UNIX domain sockets on FreeBSD, expanding functionality.
  • Conditional inclusion of <netinet/in.h> based on OS: While not directly related to FreeBSD, this adjustment suggests a cleaner build system across different operating systems.

test/host/wasi/linuxTest.cpp

Potential issues

  1. Typo in Test Function Name: TEST(LiuxTest, fromFileType) should be TEST(LinuxTest, fromFileType). This typo will cause the test to not be recognized or executed.

  2. Duplicate Test Functions: There are two test functions named fromFileType. The second definition of TEST(LinuxTest, fromFileType) will override the first one, causing the first set of tests in that function to be ignored.

  3. Incorrect Macro Check for FreeBSD Support: The code is currently wrapped in #if WASMEDGE_OS_LINUX and does not include any checks or implementations for FreeBSD, which contradicts the title "add initial support for FreeBSD". This macro check needs to be adjusted or expanded to handle FreeBSD separately.

Summary of changes

    • Changed the header file inclusion: Replaced linux.h with unix.h to support multiple Unix-like operating systems, including FreeBSD.
  • Added conditional compilation for FreeBSD: Although not explicitly shown in this patch, it suggests that similar changes would be needed to conditionally compile code for FreeBSD alongside Linux.
  • Implication of broader OS support: This change is part of a larger effort to add initial support for FreeBSD by making the WASI host layer more generic and portable across Unix-like systems.

juntao avatar Apr 16 '25 20:04 juntao

Hi @vigsterkr,
Just wanted to know if you are still working on this PR. If not, I will close it. Thanks for your contributions.

hydai avatar Sep 24 '25 20:09 hydai