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.

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

TrevorSundberg
Copy link

See the README.md I added for notes about building and running. I'm very open to make changes or answer questions :)

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -20,6 +20,29 @@ endif ()
project(remill)
cmake_minimum_required(VERSION 3.2)

if(EMSCRIPTEN)
set(CMAKE_BUILD_TYPE Release)
Copy link
Contributor

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

Copy link
Author

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!

Copy link
Collaborator

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 "\
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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} \
Copy link
Contributor

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) )

remill/OS/OS.h Outdated Show resolved Hide resolved
@TrevorSundberg
Copy link
Author

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 Outdated and the old code. In Devops when you push the new code is shown next to the comment so the reviewers can verify that it was changed. Is this normal GitHub behavior or is there something I'm supposed to do to tell the PR to use the latest commit? Seeing the Outdated just seems weird.

@pgoodman
Copy link
Collaborator

I think we might need some changes to cxx-common to add wasm as a supported target.

@pgoodman
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants