-
Notifications
You must be signed in to change notification settings - Fork 146
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.
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
Remill running on the web via Emscripten #402
base: master
Are you sure you want to change the base?
Conversation
@@ -20,6 +20,29 @@ endif () | |||
project(remill) | |||
cmake_minimum_required(VERSION 3.2) | |||
|
|||
if(EMSCRIPTEN) | |||
set(CMAKE_BUILD_TYPE Release) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved at the top of the cmake/settings.cmake file? If this new feature does not support other build configurations (such as Debug), we could add a warning message and overwrite the setting.
Example:
if(EMSCRIPTEN)
..
if(NOT "${CMAKE_BUILD_TYPE}" STREQUAL "Release")
message(WARNING "remill: Using the EMSCRIPTEN toolchain overwrites the build type to Release")
set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Build type" FORCE)
endif()
endif()
EDIT: Replaced REMILL_EMSCRIPTEN with the value provided by the toolchain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I love this idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this been resolved?
# We want to manually invoke main for Lift instead of Emscripten calling it for us | ||
# So we disable INVOKE_RUN, export callMain (and FS for files). | ||
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/web/src") | ||
set(CMAKE_CXX_FLAGS "\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to move these settings in cmake/settings.cmake and use GLOBAL_CXX_FLAGS instead of CMAKE_CXX_FLAGS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you prefer this or REMILL_EMSCRIPTEN_CXXFLAGS
?
Or let me know if I'm not understanding what is wanted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is trying to not edit the CMake CXX variables by hand, and append them to the variables used there (which will eventually use property-based target settings such as target_compile_options
)
I think it is fine to append them directly with something similar to
if(EMSCRIPTEN)
list(GLOBAL_CXX_FLAGS APPEND
flag1
flag2
)
endif()
but you can also create a setting such as REMILL_EMSCRIPTEN_CXXFLAGS
if you think it may be useful
set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/web/src") | ||
set(CMAKE_CXX_FLAGS "\ | ||
${CMAKE_CXX_FLAGS} \ | ||
$ENV{EM_CXX_FLAGS} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this setting be moved away from the environment variable?
Example, in cmake/settings.cmake
set(REMILL_EMSCRIPTEN_CXXFLAGS "default_value" CACHE STRING "Semicolon separated list of flags")
This will make the flag list show up as a configurable setting (i.e.: make edit_cache).
EDIT: The goal is to avoid using environment variables to configure the project (related: #402 (comment) )
This is my first real GitHub PR (I'm used to Azure Devops), when I commit and push the comments that were made just show |
I think we might need some changes to cxx-common to add wasm as a supported target. |
Hey @TrevorSundberg , just as an FYI, we've all been on a company trip hence the lack of movement. What do you think of us using a cxx-common built clang for the compiler to produce WASM targets? It also seems like LLVM has some kind of support for wasm with 64-bit pointers [1]. [1] https://code.woboq.org/llvm/llvm/include/llvm/ADT/Triple.h.html#llvm::Triple::wasm64 |
See the README.md I added for notes about building and running. I'm very open to make changes or answer questions :)