Conversation
68f27b8 to
9ac267a
Compare
311b2a4 to
4f0666e
Compare
f20a737 to
60903f8
Compare
|
Hi @ligurio Thanks for taking over this. I just tested on macOS ARM64 (M4), Darwin 24.6.0 Toolchain: LLVM 22.1.1, LuaJIT 2.1.1772619647, cmake 4.3.0 (all from homebrew) used: cmake -DCMAKE_C_COMPILER=/opt/homebrew/opt/llvm/bin/clang \
-DCMAKE_CXX_COMPILER=/opt/homebrew/opt/llvm/bin/clang++ \
-DENABLE_LUAJIT=ON \
-DLUA_INCLUDE_DIR=/opt/homebrew/Cellar/luajit/2.1.1772619647/include/luajit-2.1 \
-DLUA_LIBRARIES=/opt/homebrew/Cellar/luajit/2.1.1772619647/lib/libluajit-5.1.dylib \
-DCMAKE_BUILD_TYPE=Release -S . -B build
cmake --build build --parallelEverything worked cleanly
So LGTM :) |
|
@mzfr many thanks for the feedback! |
Buristan
left a comment
There was a problem hiding this comment.
Hi, Sergey!
Thanks for the patch set!
I'll proceed with the review per-patch below.
[PATCH 1/4] luzer/tests: refactoring
LGTM, after fixing one comment below.
Buristan
left a comment
There was a problem hiding this comment.
[PATCH 2/4] luzer: refactoring search_module_path()
LGTM, with two minor comments below.
luzer/luzer.c
Outdated
| search_module_path(char *base_path) { | ||
| Dl_info dlinfo; | ||
| int rc = dladdr((void *)&get_coverage_symbols_location, &dlinfo); | ||
| if (!rc) |
There was a problem hiding this comment.
Let's print dlerror(); here in case when it is available.
There was a problem hiding this comment.
Updated:
--- a/luzer/luzer.c
+++ b/luzer/luzer.c
@@ -154,8 +154,10 @@ NO_SANITIZE const char *
search_module_path(char *base_path) {
Dl_info dlinfo;
int rc = dladdr((void *)&get_coverage_symbols_location, &dlinfo);
- if (!rc)
+ if (!rc) {
+ fprintf(stderr, "%s\n", dlerror());
return NULL;
+ }
char *path = realpath(dlinfo.dli_fname, base_path);
if (!path) {
perror("realpath");There was a problem hiding this comment.
dlerror() may return NULL, but I suppose we OK with it.
Buristan
left a comment
There was a problem hiding this comment.
[PATCH 3/4] luzer: use CMAKE_SHARED_LIBRARY_SUFFIX
LGTM.
There was a problem hiding this comment.
[PATCH 4/4] luzer: add macOS ARM64 support
LGTM, in general with a few questions and comments below.
Some side notes below.
At first, I can't build luzer on macOS (I've using the same command as #77 (comment)):
/opt/homebrew/opt/llvm/bin/clang --version
Homebrew clang version 21.1.5
Target: arm64-apple-darwin25.3.0
Thread model: posix
InstalledDir: /opt/homebrew/Cellar/llvm/21.1.5/bin
Configuration file: /opt/homebrew/etc/clang/arm64-apple-darwin25.cfg
ld: warning: '/Users/s.kaplun/review/luzer/master/build/luzer/libclang_rt.asan_abi_osx.a[arm64][2](asan_abi_shim.cpp.o)' has malformed LC_DYSYMTAB, expected 45 undefined symbols to start at index 151, found 42 undefined symbols starting at index 151
[ 78%] Built target build_dso_ubsan
[ 78%] Built target build_dso_asan
[ 85%] Linking CXX shared library luzer_impl.dylib
Undefined symbols for architecture arm64:
"std::__1::__hash_memory(void const*, unsigned long)", referenced from:
std::__1::__hash_const_iterator<std::__1::__hash_node<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, void*>*> std::__1::__hash_table<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::hash<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::equal_to<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::allocator<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>::find<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) const in libclang_rt.fuzzer_no_main_osx.a[arm64][3](FuzzerDataFlowTrace.cpp.o)
It looks like that this caused by using the default AppleClang libraries instead.
The following lines before build fixes this issue for me, but ld warning still persisted.
export LDFLAGS="-L/opt/homebrew/opt/llvm/lib"
export CPPFLAGS="-I/opt/homebrew/opt/llvm/include"I suppose it would be nice to add this to the CONTRIBUTING.md.
With this fixup I've got the following errors for all ASAN/UBSAN-related tests (Debug or Release build):
28: dyld[47500]: terminating because inserted dylib '/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' could not be loaded: tried: '/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file)
28: dyld[47500]: tried: '/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/System/Volumes/Preboot/Cryptexes/OS/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file), '/opt/homebrew/Cellar/llvm/22.1.1/lib/clang/22/lib/darwin/libclang_rt.asan_osx_dynamic.dylib' (no such file)
28/28 Test #28: luzer_ffi_ubsan_null_pointer_deref ..............Subprocess aborted***Exception: 0.00 sec
But all ld warnings and test failures was fixed by updated brew's llvm to the latest version (looks like this is due to hardcoded path in tests configuration).
| LD_PRELOAD=${ASAN_DSO_PATH} | ||
| LIB_NAME=luac_asan | ||
| ERR_INJECTION=HEAP_BUFFER_OVERFLOW | ||
| ASAN_OPTIONS=abort_on_error=0 |
There was a problem hiding this comment.
Why do you want to disable this?
46616ba to
a61a76b
Compare
The patch simplifies the implementation of the function `SetHwArchString()` for obtaining the architecture name. Needed for the following patch with macOS support and Ubuntu 24.04 AArch64 (ARM64) support. Needed for #59
The replaces long strings with test environment variables with a list of environment variables joined to a single string. Needed for #59
The luzer module relies on other shared libraries: library with custom mutator and sanitizer libraries built with libFuzzer. These libraries are located in the directory with luzer_impl.so and on module loading we search a path to this shared library to found other shared libraries. It is done by the function `search_module_path()` - it iterates through the directories specified in the environment variable LUA_CPATH and tries to find the directory with luzer_impl.so. However, there's a simpler way to find a directory path with luzer_impl.so - using the `dladdr(3)` function. The patch refactors the function `search_module_path()` to make its implementation easier for support. Related to #67 Needed for #59
The patch replace static suffix ".so" for shared libraries with CMAKE_SHARED_LIBRARY_SUFFIX [1]. Needed for the following patch with macOS support. Needed for #59 1. https://cmake.org/cmake/help/latest/variable/CMAKE_SHARED_LIBRARY_SUFFIX.html
There are three ways to set Lua libraries, headers and executable: 1. Setting CMake option ENABLE_LUAJIT will trigger searching LuaJIT library, headers and executable. 2. Setting CMake options LUA_LIBRARIES, LUA_INCLUDE_DIR and optional LUA_EXECUTABLE with a path Lua library, directory with headers and to a path to Lua executable explicitly. 3. Searching PUC Rio Lua library, headers and executable automatically. LUA_EXECUTABLE is required for regression testing only, so if this CMake variable was not specified explicitly or was not found automatically the testing will not be enabled. The patch simplify a code for searching Lua library and makes the behavior the same for all three cases described above: - Move a code for searching `luajit` binary executable to CMake module. - Remove fatal message for a case when PUC Rio Lua executable was not found. - Remove excess `unset()` for ENABLE_TESTING. Needed for #59
The patch adds macOS ARM64 support. Notable changes are: - Fixed building sanitizers libraries. There's no need to strip libraries because they don't have the same object files as Linux. - Added the -Wno-builtin-memcpy-chk-size option to build the test library; otherwise, the build won't work. - Use the DYLD_INSERT_LIBRARIES environment variable to load the ASAN library. - Added an environment variable ASAN_OPTIONS=abort_on_error=0 for regression regression tests. Without option ASAN abort Lua runtime and CTest fails the test. - Added a directory with LuaJIT binary executable to HINTS for find_program(). Otherwise, `luajit` is not found automatically. - Added macOS 26 ARM64 [1] to the regression testing on Github Actions. 1. https://docs.github.com/en/actions/reference/runners/github-hosted-runners Closes #59 Co-authored-by: Sergey Bronnikov <estetus@gmail.com>
4eeb7a6 to
b406abf
Compare
Original changes in the PR #58
Closes #59
Related to #67