Skip to content

Apply custom compilation flags only to 1P code #7235

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

Closed
wants to merge 11 commits into from
Closed

Apply custom compilation flags only to 1P code #7235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bec67eb
Apply custom compilation flags only to 1P code
tlively Jan 22, 2025
5741be9
add llvm include directory for binaryen-unittest
tlively Jan 22, 2025
24b576d
Declare include directories first
tlively Jan 22, 2025
94c95f0
move some options up
tlively Jan 22, 2025
88e1d45
move library output directory config up
tlively Jan 22, 2025
eda8a24
Merge branch 'main' into cmake-update
tlively Jan 22, 2025
c6a0749
fix js and wasm links
tlively Jan 22, 2025
96d7454
declare binaryen_js and binaryen_wasm early
tlively Jan 23, 2025
3714bb1
add -fno-rtti to gtest
tlively Jan 23, 2025
16f4402
modernize and hoist thread config
tlively Jan 23, 2025
9324e74
Off -> OFF, On -> ON
tlively Jan 23, 2025
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
235 changes: 110 additions & 125 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Version set according the the cmake versions available in Ubuntu/Bionic:
# https://packages.ubuntu.com/bionic/cmake
cmake_minimum_required(VERSION 3.10.2)
# Version set according the the cmake versions available in Ubuntu/Focal:
# https://packages.ubuntu.com/focal/cmake
cmake_minimum_required(VERSION 3.16.3)
Copy link
Member

@kripken kripken Jan 23, 2025

Choose a reason for hiding this comment

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

Is this enough for say Emscripten's requirements (both versions and current builders)?

Copy link
Member Author

@tlively tlively Jan 23, 2025

Choose a reason for hiding this comment

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

I believe so. I took a look at Emscripten's CI config before making this change and as far as I could tell, it was using focal instead of bionic.


# Needed for C++17 (std::variant)
# TODO(https://github.com/WebAssembly/binaryen/issues/4299): We need
Expand Down Expand Up @@ -37,12 +37,23 @@ option(BUILD_TESTS "Build GTest-based tests" ON)
# Turn this off to build only the library.
option(BUILD_TOOLS "Build tools" ON)

# Turn this off to install only tools and not static/dynamic libs
option(INSTALL_LIBS "Install libraries" ON)

# Turn this on to build only the subset of tools needed for emscripten
option(BUILD_EMSCRIPTEN_TOOLS_ONLY "Build only tools needed by emscripten" OFF)

# Build LLVM's DWARF support. This is unconditionally disabled when building with Emscripten.
option(BUILD_LLVM_DWARF "Enable full DWARF support" ON)

option(BUILD_STATIC_LIB "Build as a static library" OFF)
if(MSVC)
# We don't have dllexport declarations set up for windows yet.
set(BUILD_STATIC_LIB ON)
endif()

option(BYN_ENABLE_LTO "Build with LTO" OFF)

# Turn this off to install only tools and not static/dynamic libs
option(INSTALL_LIBS "Install libraries" ON)

# Turn this on to build binaryen.js as ES5, with additional compatibility configuration for js_of_ocaml.
option(JS_OF_OCAML "Build binaryen.js for js_of_ocaml" OFF)

Expand Down Expand Up @@ -80,43 +91,42 @@ endif()

configure_file(config.h.in config.h)

# Configure threads

set(THREADS_PREFER_PTHREAD_FLAG ON)
find_package(Threads REQUIRED)

# Support functionality.

function(add_compile_flag value)
message(STATUS "Building with ${value}")
foreach(variable CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
set(${variable} "${${variable}} ${value}" PARENT_SCOPE)
endforeach(variable)
endfunction()
# Use a property to emulate a global variable.
set_property(GLOBAL PROPERTY binaryen_1p_targets binaryen)
Copy link
Member

@kripken kripken Jan 23, 2025

Choose a reason for hiding this comment

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

can't a setting work as well? (I've never seen cmake properties used before, and at a glance at the usage and docs, it's not obvious to me what is added here, since this is global)

Copy link
Member Author

@tlively tlively Jan 23, 2025

Choose a reason for hiding this comment

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

I'm not sure what you mean by a "setting." AFAIK cmake has no such concept. I can't use a normal variable because they can only be set in current or immediate parent scopes, so there's no way for binaryen_add_executable calls from subdirectories to modify a shared list of targets. The only other kind of "variable" cmake has is properties. This particular property is not stored in the cache because there's no reason for the user to have visibility into it.

Copy link
Member

@kripken kripken Jan 23, 2025

Choose a reason for hiding this comment

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

By "setting" I mean e.g. if(NOT EMSCRIPTEN) that we use in several CMakeLists.txt files. Those can be set from the commandline but also using set() IIANM.

But, taking a step back here, doesn't CMake apply settings to child directories? What if we moved the flags stuff from the toplevel CMakeLists.txt to src/, which contains only our code?

Copy link
Member Author

@tlively tlively Jan 23, 2025

Choose a reason for hiding this comment

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

Moving the flags management one scope down wouldn't be sufficient to use a normal variable here because executables are added several scopes down (and also in test/gtest/, which is not under src/). Maybe you have a larger refactoring in mind? I would strongly prefer to declare Mission Accomplished on this and not try to get a very different architecture working, given how difficult it is to make correct changes here.

Copy link
Member Author

@tlively tlively Jan 23, 2025

Choose a reason for hiding this comment

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

Also, FWIW EMSCRIPTEN and MSVC are normal CMake variables. They are visible in child scopes, but mutations performed in those scopes would be done to copies of the variables and would not be visible back in the parent or in sibling scopes. (Yes this is awful.)

Copy link
Member

@kripken kripken Jan 23, 2025

Choose a reason for hiding this comment

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

Moving the flags management one scope down wouldn't be sufficient to use a normal variable here

Why do we need a normal variable here? The goal of the PR IIUC is to apply custom flags only to 1P code. If we had a directory structure like this:

src/        ;; all of this is 1P code
thirdparty/ ;; all of this is 3P code

then wouldn't the problem be trivial? We would apply custom flags inside src/ and that's it. Sorry if I'm missing something obvious, but this seems simple to do (so likely I am confused).


function(add_cxx_flag value)
function(add_compile_flag value)
message(STATUS "Building with ${value}")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${value}" PARENT_SCOPE)
get_property(targets GLOBAL PROPERTY binaryen_1p_targets)
foreach(target ${targets})
target_compile_options(${target} PRIVATE ${value})
endforeach()
endfunction()

function(add_debug_compile_flag value)
if("${CMAKE_BUILD_TYPE}" MATCHES "Debug")
message(STATUS "Building with ${value}")
add_compile_flag(${value})
endif()
foreach(variable CMAKE_C_FLAGS_DEBUG CMAKE_CXX_FLAGS_DEBUG)
set(${variable} "${${variable}} ${value}" PARENT_SCOPE)
endforeach(variable)
endfunction()

function(add_nondebug_compile_flag value)
if(NOT "${CMAKE_BUILD_TYPE}" MATCHES "Debug")
message(STATUS "Building with ${value}")
add_compile_flag(${value})
endif()
foreach(variable CMAKE_C_FLAGS_RELEASE CMAKE_CXX_FLAGS_RELEASE CMAKE_C_FLAGS_RELWITHDEBINFO CMAKE_CXX_FLAGS_RELWITHDEBINFO CMAKE_C_FLAGS_MINSIZEREL CMAKE_CXX_FLAGS_MINSIZEREL)
set(${variable} "${${variable}} ${value}" PARENT_SCOPE)
endforeach(variable)
endfunction()

function(add_link_flag value)
message(STATUS "Linking with ${value}")
foreach(variable CMAKE_EXE_LINKER_FLAGS CMAKE_SHARED_LINKER_FLAGS)
set(${variable} "${${variable}} ${value}" PARENT_SCOPE)
endforeach(variable)
get_property(targets GLOBAL PROPERTY binaryen_1p_targets)
foreach(target ${targets})
target_link_options(${target} PRIVATE ${value})
endforeach()
endfunction()

function(binaryen_setup_rpath name)
Expand All @@ -138,48 +148,27 @@ function(binaryen_setup_rpath name)
endif()

set_target_properties(${name} PROPERTIES
BUILD_WITH_INSTALL_RPATH On
BUILD_WITH_INSTALL_RPATH ON
INSTALL_RPATH "${_install_rpath}"
${_install_name_dir})
endfunction()

function(binaryen_add_executable name sources)
add_executable(${name} ${sources})
target_link_libraries(${name} ${CMAKE_THREAD_LIBS_INIT})
target_link_libraries(${name} Threads::Threads)
target_link_libraries(${name} binaryen)
binaryen_setup_rpath(${name})
install(TARGETS ${name} DESTINATION ${CMAKE_INSTALL_BINDIR})
set_property(GLOBAL APPEND PROPERTY binaryen_1p_targets ${name})
endfunction()

# Options

option(BUILD_STATIC_LIB "Build as a static library" OFF)
if(MSVC)
# We don't have dllexport declarations set up for windows yet.
set(BUILD_STATIC_LIB ON)
endif()

# For now, don't include full DWARF support in JS builds, for size.
if(NOT EMSCRIPTEN)
option(BUILD_LLVM_DWARF "Enable full DWARF support" ON)

if(BUILD_LLVM_DWARF)
if(MSVC)
ADD_COMPILE_FLAG("/DBUILD_LLVM_DWARF")
else()
ADD_COMPILE_FLAG("-DBUILD_LLVM_DWARF")
endif()
endif()
endif()

# Compiler setup.
# Set include paths

include_directories(${CMAKE_CURRENT_SOURCE_DIR}/src)
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/FP16/include)
if(BUILD_LLVM_DWARF)
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/llvm-project/include)
endif()

# Add output directory to include path so config.h can be found
include_directories(${CMAKE_CURRENT_BINARY_DIR})

Expand All @@ -190,7 +179,75 @@ foreach(SUFFIX "_DEBUG" "_RELEASE" "_RELWITHDEBINFO" "_MINSIZEREL" "")
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY${SUFFIX} "${PROJECT_BINARY_DIR}/lib")
endforeach()

option(BYN_ENABLE_LTO "Build with LTO" Off)
# Declare libbinaryen

if(BUILD_STATIC_LIB)
message(STATUS "Building libbinaryen as statically linked library.")
add_library(binaryen STATIC)
add_definitions(-DBUILD_STATIC_LIBRARY)
else()
message(STATUS "Building libbinaryen as shared library.")
add_library(binaryen SHARED)
endif()

# Add sources and declare executable targets

add_subdirectory(src/ir)
add_subdirectory(src/asmjs)
add_subdirectory(src/cfg)
add_subdirectory(src/emscripten-optimizer)
add_subdirectory(src/interpreter)
add_subdirectory(src/passes)
add_subdirectory(src/parser)
add_subdirectory(src/support)
add_subdirectory(src/wasm)
add_subdirectory(src/analysis)

if(BUILD_TOOLS)
# Build binaryen tools
add_subdirectory(src/tools)
endif()

add_subdirectory(third_party)

# Configure lit tests
add_subdirectory(test/lit)

if(BUILD_TESTS)
# Configure GTest unit tests
add_subdirectory(test/gtest)
endif()

# Sources.

file(GLOB binaryen_HEADERS src/*.h)
set(binaryen_SOURCES
src/binaryen-c.cpp
${binaryen_HEADERS}
)
target_sources(binaryen PRIVATE ${binaryen_SOURCES})
target_link_libraries(binaryen ${CMAKE_THREAD_LIBS_INIT})

# Declare binaryen.js targets

if(EMSCRIPTEN)
binaryen_add_executable(binaryen_wasm ${binaryen_SOURCES})
binaryen_add_executable(binaryen_js ${binaryen_SOURCES})
endif()

# For now, don't include full DWARF support in JS builds, for size.
if(NOT EMSCRIPTEN)
if(BUILD_LLVM_DWARF)
if(MSVC)
ADD_COMPILE_FLAG("/DBUILD_LLVM_DWARF")
else()
ADD_COMPILE_FLAG("-DBUILD_LLVM_DWARF")
endif()
endif()
endif()

# Compiler setup.

if(BYN_ENABLE_LTO)
if(NOT CMAKE_CXX_COMPILER_ID MATCHES "Clang")
message(FATAL_ERROR "ThinLTO is only supported by clang")
Expand Down Expand Up @@ -264,9 +321,6 @@ else()

option(ENABLE_WERROR "Enable -Werror" ON)

set(THREADS_PREFER_PTHREAD_FLAG ON)
set(CMAKE_THREAD_PREFER_PTHREAD ON)
find_package(Threads REQUIRED)
if(NOT EMSCRIPTEN)
if(CMAKE_SYSTEM_PROCESSOR MATCHES "^i.86$")
# wasm doesn't allow for x87 floating point math
Expand Down Expand Up @@ -310,8 +364,6 @@ else()
else()
add_link_flag("-Wl,--stack,8388608")
endif()
elseif(NOT EMSCRIPTEN)
add_compile_flag("-fPIC")
endif()
add_debug_compile_flag("-g3")
if(BYN_ENABLE_ASSERTIONS)
Expand Down Expand Up @@ -400,69 +452,6 @@ if(UNIX AND CMAKE_GENERATOR STREQUAL "Ninja")
endif()
endif()

# Static libraries
# Current (partial) dependency structure is as follows:
# tools -> passes -> wasm -> asmjs -> support
# TODO: It's odd that wasm should depend on asmjs, maybe we should fix that.
add_subdirectory(src/ir)
add_subdirectory(src/asmjs)
add_subdirectory(src/cfg)
add_subdirectory(src/emscripten-optimizer)
add_subdirectory(src/interpreter)
add_subdirectory(src/passes)
add_subdirectory(src/parser)
add_subdirectory(src/support)
add_subdirectory(src/wasm)
add_subdirectory(src/analysis)

if(BUILD_TOOLS)
# Build binaryen tools
add_subdirectory(src/tools)
endif()

add_subdirectory(third_party)

# Configure lit tests
add_subdirectory(test/lit)

if(BUILD_TESTS)
# Configure GTest unit tests
add_subdirectory(test/gtest)
endif()

# Object files
set(binaryen_objs
$<TARGET_OBJECTS:passes>
$<TARGET_OBJECTS:wasm>
$<TARGET_OBJECTS:asmjs>
$<TARGET_OBJECTS:emscripten-optimizer>
$<TARGET_OBJECTS:ir>
$<TARGET_OBJECTS:cfg>
$<TARGET_OBJECTS:support>
$<TARGET_OBJECTS:analysis>
$<TARGET_OBJECTS:parser>
$<TARGET_OBJECTS:interpreter>)

if(BUILD_LLVM_DWARF)
SET(binaryen_objs ${binaryen_objs} $<TARGET_OBJECTS:llvm_dwarf>)
endif()

# Sources.

file(GLOB binaryen_HEADERS src/*.h)
set(binaryen_SOURCES
src/binaryen-c.cpp
${binaryen_HEADERS}
)
if(BUILD_STATIC_LIB)
message(STATUS "Building libbinaryen as statically linked library.")
add_library(binaryen STATIC ${binaryen_SOURCES} ${binaryen_objs})
add_definitions(-DBUILD_STATIC_LIBRARY)
else()
message(STATUS "Building libbinaryen as shared library.")
add_library(binaryen SHARED ${binaryen_SOURCES} ${binaryen_objs})
endif()
target_link_libraries(binaryen ${CMAKE_THREAD_LIBS_INIT})
if(INSTALL_LIBS OR NOT BUILD_STATIC_LIB)
install(TARGETS binaryen
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
Expand All @@ -481,9 +470,7 @@ endif()
# in an incorrect way for emscripten.
if(EMSCRIPTEN)
# binaryen.js WebAssembly variant
add_executable(binaryen_wasm
${binaryen_SOURCES})
target_link_libraries(binaryen_wasm wasm asmjs emscripten-optimizer passes ir cfg support analysis parser interpreter wasm)
target_link_libraries(binaryen_wasm binaryen)
target_link_libraries(binaryen_wasm "-sFILESYSTEM")
target_link_libraries(binaryen_wasm "-sEXPORT_NAME=Binaryen")
target_link_libraries(binaryen_wasm "-sNODERAWFS=0")
Expand Down Expand Up @@ -511,9 +498,7 @@ if(EMSCRIPTEN)
install(TARGETS binaryen_wasm DESTINATION ${CMAKE_INSTALL_BINDIR})

# binaryen.js JavaScript variant
add_executable(binaryen_js
${binaryen_SOURCES})
target_link_libraries(binaryen_js wasm asmjs emscripten-optimizer passes ir cfg support analysis parser interpreter wasm)
target_link_libraries(binaryen_js binaryen)
target_link_libraries(binaryen_js "-sWASM=0")
target_link_libraries(binaryen_js "-sWASM_ASYNC_COMPILATION=0")
if(${CMAKE_CXX_COMPILER_VERSION} STREQUAL "6.0.1")
Expand Down
2 changes: 1 addition & 1 deletion src/analysis/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ set(analysis_SOURCES
cfg.cpp
${analysis_HEADERS}
)
add_library(analysis OBJECT ${analysis_SOURCES})
target_sources(binaryen PRIVATE ${analysis_SOURCES})
2 changes: 1 addition & 1 deletion src/asmjs/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ set(asmjs_SOURCES
shared-constants.cpp
${asmjs_HEADERS}
)
add_library(asmjs OBJECT ${asmjs_SOURCES})
target_sources(binaryen PRIVATE ${asmjs_SOURCES})
2 changes: 1 addition & 1 deletion src/cfg/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ set(cfg_SOURCES
Relooper.cpp
${cfg_HEADERS}
)
add_library(cfg OBJECT ${cfg_SOURCES})
target_sources(binaryen PRIVATE ${cfg_SOURCES})
2 changes: 1 addition & 1 deletion src/emscripten-optimizer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,4 @@ set(emscripten-optimizer_SOURCES
simple_ast.cpp
${emscripten-optimizer_HEADERS}
)
add_library(emscripten-optimizer OBJECT ${emscripten-optimizer_SOURCES})
target_sources(binaryen PRIVATE ${emscripten-optimizer_SOURCES})
2 changes: 1 addition & 1 deletion src/interpreter/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ set(interpreter_SOURCES
interpreter.cpp
${interpreter_HEADERS}
)
add_library(interpreter OBJECT ${interpreter_SOURCES})
target_sources(binaryen PRIVATE ${interpreter_SOURCES})
2 changes: 1 addition & 1 deletion src/ir/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,4 @@ set(ir_SOURCES
module-splitting.cpp
${ir_HEADERS}
)
add_library(ir OBJECT ${ir_SOURCES})
target_sources(binaryen PRIVATE ${ir_SOURCES})
2 changes: 1 addition & 1 deletion src/parser/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,4 @@ set(parser_SOURCES
wat-parser.cpp
${parser_HEADERS}
)
add_library(parser OBJECT ${parser_SOURCES})
target_sources(binaryen PRIVATE ${parser_SOURCES})
2 changes: 1 addition & 1 deletion src/passes/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,4 +144,4 @@ if(EMSCRIPTEN)
list(REMOVE_ITEM passes_SOURCES "hash-stringify-walker.cpp")
list(REMOVE_ITEM passes_SOURCES "Outlining.cpp")
endif()
add_library(passes OBJECT ${passes_SOURCES})
target_sources(binaryen PRIVATE ${passes_SOURCES})
Loading
Loading