Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ASAN #1394

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ASAN #1394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ option(AVM_RELEASE "Build an AtomVM release" OFF)
option(AVM_CREATE_STACKTRACES "Create stacktraces" ON)
option(AVM_BUILD_RUNTIME_ONLY "Only build the AtomVM runtime" OFF)
option(COVERAGE "Build for code coverage" OFF)
option(ENABLE_ASAN "Use Address Sanitizer" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this should be the default: it is very useful during development, but it should be off during release.
Also I'm not sure how interacts with platforms that do not properly support -fsanitize=address such as ESP32.

So maybe it should be manually enabled (and we might suggest it in our documentation) and of course we should add a job in our CI that enables it. After all what really matter is not merging code that fails those checks.

I think we should have jobs and options for undefined behavior sanitizer, thread sanitizer and memory sanitizer and they should be part of our CI as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I don't have means to test embedded platforms so most of my testing comes from unix and WASM port.

What do you think about adding

set(SANITIZER none) # available values: address, memory, undefined, none

and corresponding ifs in libAtomVM CMakeList?

Is there any way to create local CMakeLists or you need to use env variables when running CMake to set those options?


if((${CMAKE_SYSTEM_NAME} STREQUAL "Darwin") OR
(${CMAKE_SYSTEM_NAME} STREQUAL "Linux") OR
Expand Down
7 changes: 6 additions & 1 deletion src/libAtomVM/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ target_compile_features(libAtomVM PUBLIC c_std_11)
if (CMAKE_C_COMPILER_ID STREQUAL "GNU")
target_compile_options(libAtomVM PUBLIC -Wall ${MAYBE_PEDANTIC_FLAG} ${MAYBE_WERROR_FLAG} -Wextra -ggdb -Werror=incompatible-pointer-types)
elseif (CMAKE_C_COMPILER_ID MATCHES "Clang")
target_compile_options(libAtomVM PUBLIC -Wall --extra-warnings -Werror=incompatible-pointer-types ${MAYBE_WERROR_FLAG})
target_compile_options(libAtomVM PUBLIC -Wall --extra-warnings -Werror=incompatible-pointer-types -g ${MAYBE_WERROR_FLAG})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that -g is already the default when building with cmake in Debug mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is build mode dictated by DEBUG env variable? I couldn't find reference to debug/release in the code.

endif()

if (ENABLE_REALLOC_GC)
Expand All @@ -132,6 +132,11 @@ if (ADVANCED_TRACING)
target_compile_definitions(libAtomVM PUBLIC ENABLE_ADVANCED_TRACE)
endif()

if(ENABLE_ASAN)
target_compile_options(libAtomVM PUBLIC -fsanitize=address -fno-omit-frame-pointer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is -fno-omit-frame-pointer cross platform? I remember that was something quite common on i386 that has few available registers, but I think it is not the default on several platforms (and maybe it is not even available as an option on other archs).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Logically, omitting frame pointer is an optimization so every platform should support that (making one less available register if disabled) but from reading GCC docs it's noop on archs that don't benefit from it.

https://gcc.gnu.org/onlinedocs/gcc-4.9.2/gcc/Optimize-Options.html
look for -fomit-frame-pointer

target_link_options(libAtomVM PUBLIC -fsanitize=address)
endif()

target_link_libraries(libAtomVM PUBLIC m)
include(CheckCSourceCompiles)
set(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Werror=unknown-pragmas")
Expand Down
Loading