Add `-fno-rtti`/`fno-exceptions` based on LLVM's configuration #1291

Johanmyst · 2023-12-19T15:13:53Z

Currently the top-level CMakeLists.txt script force-adds the flags -fno-rtti and -fno-exceptions using the add_compile_options() function from CMake; adding the flags for all modules built thereafter. This causes SVF to disregard LLVM's configuration and always build SvfLLVM without RTTI or exceptions.

This pull request moves the loading of LLVM to the top-level configuration file, where -fno-rtti and -fno-exceptions are added for both SvfCore and SvfLLVM iff the LLVM instance being used is configured that way.

Additionally, the way the compile definitions now match the way recommended by LLVM for including LLVM from a CMakeLists.txt script.

…ption handling from LLVM's configuration, rather than hardcoding

yuleisui · 2023-12-20T00:31:11Z

There seems a build error here. https://github.com/SVF-tools/SVF/actions/runs/7263779025/job/19806338524?pr=1291#step:6:197

yuleisui · 2023-12-20T01:17:36Z

CMakeLists.txt

+  endif()
+
+  if(CMAKE_BUILD_TYPE STREQUAL "Debug" AND NOT ${LLVM_BUILD_TYPE} STREQUAL "Debug")
+    message(FATAL_ERROR "Got build type 'Debug' but LLVM is not built in Debug mode!")


We don't need LLVM to be built in debug mode if SVF is built under debug

Ah, alright, fair enough. I added this check because of this remark from this setup guide:

Build the debug version (make sure you have also installed the debug version of LLVM)

That page hasn't been updated since 2018 though, so I assume this is either no longer the case or was written erroneously and just never updated? Either way, I'll remove this check from the CMake script and update the setup guide page!

…built in debug-mode when SVF is being built in debug-mode

…nd add some new functions sepcfications in extapi.c

The getPointerElementType will be deprecated in future versions of LLVM. Considering compatibility, avoid using getPointerElementType()->isIntegerTy(8) to determine if the arguments and return values are of the void * type.

…an forcing unique_ptr to avoid erronous garbage collection clearing LLVM internal objects when SVF is used from within an LLVM pass

yuleisui · 2023-12-20T08:52:17Z

CMakeLists.txt

+    set(CMAKE_CXX_STANDARD 14)
+    message(STATUS "Got LLVM version ${LLVM_VERSION}; using C++ standard: ${CMAKE_CXX_STANDARD}")
+  else()
+    message(FATAL_ERROR "Found unsupported LLVM version; need <= 15, got: ${LLVM_VERSION}")


We will update to LLVM-16 in the coming weeks. Would be good to have this else branch open with C++14 for now?

Interesting; I thought upgrading to LLVM-16 or higher was not planned due to the switch to opaque pointers? Either way, I'll modify this check to a warning for versions greater than 16. 👍

Done; for versions greater than 15 the C++17 standard will now be used (a warning will be printed for now, feel free to remove that when the switch to version LLVM 16 is released).

… to cater to future LLVM 16 support

…tories from only to `SvfLLVM` to global additions so that modules defined after `SvfLLVM` or in its subdirectories automatically inherit/can find LLVM as well

Johanmyst · 2023-12-20T09:18:52Z

Pushed another small fix; because the LLVM definitions/include dirs/link dirs were added directly to the SvfCore target rather than globally the tools in svf-llvm/tools/... could not find the libLLVM.so library file, so this last commit adds the definitions/dirs globally so that any targets defined after/in subdirectories of SvfLLVM can also find this correctly.

yuleisui · 2023-12-20T09:33:17Z

build fails again, looks to be the same issue when finding llvm.

…et_link_libraries()`); linked SvfCore and SvfLLVM for tools; LLVM link directory should be added to rpath for tools

yuleisui · 2023-12-20T10:10:09Z

svf-llvm/tools/CFL/CMakeLists.txt

+  separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
+  target_include_directories(cfl PUBLIC include)
+  target_include_directories(cfl SYSTEM PUBLIC ${LLVM_INCLUDE_DIRS})
+  target_link_directories(cfl PUBLIC ${LLVM_LIBRARY_DIRS})
+  target_compile_definitions(cfl PUBLIC ${LLVM_DEFINITIONS})
+
+  target_link_libraries(cfl SvfCore SvfLLVM)


It looks to me like every tool has these repetitive lines. Could we move them to parent cmakelist?

Of course; I did this because I thought the linking was going wrong but on my system this is difficult to reproduce as my LLVM instance's library directory is also in $LD_LIBRARY_PATH, so I cannot reproduce the issue with finding libLLVM.[so|a]. Especially since the CMake output during the git workflow don't include the build command itself, so I can't see if the issue is due to the rpath not being set right, or if somehow link_directories() is not propagating to targets defined in subdirectories.

Either way, it is not necessary to explicitly link against the LLVM library anyway since the library is defined using add_llvm_library, so I removed this part. Moreover, since SVF aims to support building in- and out-of-tree, the SVF-LLVM tools can/should also be defined using add_llvm_executable or even add_llvm_tool (I didn't do the latter for now as that means they only get built when specific build flags are set for LLVM). This simplifies the linking & the makefiles a bit since there is no longer a need for in-tree detection and what not.

Note that I've also modified build.sh and setup.sh to not only append LLVM's binary directory (for local installations by the build script) to the $PATH variable, but also its library directories to $LD_LIBRARY_PATH so that the linker can reliable find the libraries (idem ditto for Z3). I also did the same for the library files produced by SVF so targets linking against SVF can find them easier. Please let me know if you think this should be done differently!

yuleisui · 2023-12-20T10:16:14Z

CMakeLists.txt

+  find_package(LLVM REQUIRED CONFIG HINTS ${LLVM_DIR} $ENV{LLVM_DIR})
+  message(STATUS "LLVM STATUS:
+    Version       ${LLVM_VERSION}
+    Definitions   ${LLVM_DEFINITIONS}
+    Includes      ${LLVM_INCLUDE_DIRS}
+    Libraries     ${LLVM_LIBRARY_DIRS}
+    Targets       ${LLVM_TARGETS_TO_BUILD}
+    Build type    ${LLVM_BUILD_TYPE}
+    Exceptions    ${LLVM_ENABLE_EH}
+    RTTI          ${LLVM_ENABLE_RTTI}
+    Dynamic lib   ${LLVM_LINK_LLVM_DYLIB}"
+  )


After a second look, the current cmakelist in the root folder was not supposed to have anything related to LLVM and those LLVM options should be moved to svf-llvm/CMakelist.txt. I assume this change should be straight-forward?

SVF itself is now designed language-independent, and we may have another module later (e.g., svf-xxx) which may not rely on llvm-based language, so a clean root cmakelist would be helpful.

Alright; I moved this check to the top-level CMakeLists.txt because that was where flags like -fno-exceptions or -fno-rtti were added. I've moved this check back to the script in the svf-llvm directory; the flags for -fno-rtti or -fno-exceptions are now only added to SvfLLVM (if users want the main library to be built with it they can just specify it through $CFLAGS or $CXXFLAGS).

…ts to also include the installed LLVM/Z3 instances in $LD_LIBRARY_PATH (same for compiled SVF libraries)

…link LLVM libraries to avoid undefined symbols

codecov · 2023-12-20T13:10:11Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (dfeb685) 64.51% compared to head (1063318) 64.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1291      +/-   ##
==========================================
+ Coverage   64.51%   64.56%   +0.05%     
==========================================
  Files         223      223              
  Lines       23833    23841       +8     
==========================================
+ Hits        15376    15394      +18     
+ Misses       8457     8447      -10

Files	Coverage Δ
svf/lib/Util/ExtAPI.cpp	`67.10% <75.00%> (+1.77%)`	⬆️

... and 46 files with indirect coverage changes

yuleisui · 2023-12-21T02:34:22Z

build.sh

+# Add LLVM & Z3 to $PATH and $LD_LIBRARY_PATH (prepend so that selected instances will be used first)
+export PATH=$LLVM_DIR/bin:$Z3_DIR/bin:$PATH
+export LD_LIBRARY_PATH=$LLVM_DIR/lib:$Z3_BIN/lib:$LD_LIBRARY_PATH
+


There is no need to export since the end of build.sh will call setup.sh to establish the environmental variables.

Alright, I'll remove those references.

yuleisui · 2023-12-21T02:40:38Z

CMakeLists.txt

 install(
  DIRECTORY ${PROJECT_SOURCE_DIR}/svf/include/
            ${PROJECT_SOURCE_DIR}/svf-llvm/include/
  COMPONENT devel
-  DESTINATION include/svf
+  DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/include/svf
  FILES_MATCHING
  PATTERN "**/*.h")



Could we also move the following to svf-llvm/CMakeList.txt?

Compile extapi.c to extapi.bc

find_path(LLVM_CLANG_DIR
NAMES clang llvm
HINTS ${LLVM_DIR} ENV LLVM_DIR
PATH_SUFFIXES bin)
add_custom_target(extapi_ir ALL
COMMAND ${LLVM_CLANG_DIR}/clang -w -S -c -Xclang -disable-O0-optnone -fno-discard-value-names -emit-llvm ${PROJECT_SOURCE_DIR}/svf-llvm/lib/extapi.c -o ${PROJECT_BINARY_DIR}/svf-llvm/extapi.bc
DEPENDS ${PROJECT_SOURCE_DIR}/svf-llvm/lib/extapi.c
)

I also moved the installation commands to the subdirectory so the SvfLLVM target isn't referenced at all in the top-level CMakeLists.txt anymore.

While doing that I also fixed the remaining installation predicates:

<svf_root>/svf/include (i.e. its contents) is installed to <install_prefix>/include/svf/

<svf_root>/svf-llvm/include (i.e. the SVF-LLVM directory therein) is installed to <install_prefix>/include/svf/

<svf_root/[Debug|Release]-Build/include/Util/config.h is installed to <install_prefix>/include/svf/Util/config.h

<svf_root/[Debug|Release]-Build/svf/libSvfCore.[a|so] is installed to <install_prefix>/lib/libSvfCore.[a|so]

<svf_root/[Debug|Release]-Build/svf-llvm/libSvfLLVM.[a|so] is installed to <install_prefix>/lib/libSvfLLVM[a|so]

<svf_root>/[Debug|Release]-Build/svf-llvm/extapi.bc is installed to <install_prefix>/include/svf/SVF-LLVM/extapi.bc (I thought this was the best place as placing it by itself in <install_prefix>/lib/extapi.bc so that it's placed alongside libSVF.[a|so] seemed inconvenient)

Tools from <svf_root>/svf-llvm/tools/... (built to <svf_root>/[Debug|Release]-Build/bin/...) are installed to <install_prefix>/bin/...

This should be unnecessary for the way SVF is currently designed to be used (i.e. through the build.sh and setup.sh scripts), but it allows users who want to install SVF to a specific location to not have to manually link/include the different directories/library files so explicitly.

yuleisui · 2023-12-22T01:11:33Z

CMakeLists.txt

-add_compile_options("-fno-rtti")
-add_compile_options("-fno-exceptions")


If we want to enable svfcore as -fno-rtti/fno-exceptions, these two options should also be in the root/CMakeList or svf/CMakeList?

Sure, this was why I initially moved the LLVM finding into the top-level CMakeLists.txt since settings these options is based on whether the found LLVM instance supports them. I changed it now so that the CMakeLists.txt in svf-llvm sets a variable in its parent scope, which is then checked in the top-level CMakeLists.txt to conditionally set the flags for all targets.

xudon9 · 2023-12-22T04:30:30Z

svf-llvm/CMakeLists.txt

-  if(LLVM_LINK_LLVM_DYLIB)
-    set(llvm_libs LLVM)
+if(NOT LLVM_ENABLE_RTTI)
+  message(STATUS "Building SVF-llvm without RTII")


RTTI is misspelled in the message.

Fixed. Thanks. :)

yuleisui · 2023-12-22T14:11:32Z

Thanks and waiting for your push and will have another look.

… variables to the CMakeLists.txt script in svf-llm/; modified generated configuration variables in config.h (and updated ExtAPI.h/ExtAPI.cpp accordingly); removed unnecessary exports from build script; added additional configuration information to config.h

…vm; changes to SVF build system: * Updated CMake build system to install correctly; * Moved more LLVM-only variables to the CMakeLists.txt script in svf-llm/; * Modified generated configuration variables in config.h (updated ExtAPI.h/ExtAPI.cpp accordingly); * Removed unnecessary exports from build script; * Added additional configuration information to config.h

xudon9 · 2023-12-28T06:26:16Z

svf-llvm/CMakeLists.txt

+# Build SVF with C++ standard C++17
+set_property(TARGET SvfLLVM PROPERTY CXX_STANDARD 17)
+set_property(TARGET SvfLLVM PROPERTY CXX_EXTENSIONS ON)
+set_property(TARGET SvfLLVM PROPERTY CXX_STANDARD_REQUIRED ON)


Any better reason to set the C++ standard for SVFcore and SVF-LLVM separately? I think the original way is less repetitive.

I think CMake generally recommends applying options/properties to specific targets rather than using global options (e.g. the documentation for link_libraries() states: "The target_link_libraries() command should be preferred whenever possible".

However, I think the set_property() command -- by default -- applies the properties to the current directory and any subdirectories included later. I applied the option to each target separately to avoid clobbering the build configuration of any projects that might include SVF through add_subdirectory(), but I think this is indeed unnecessary. I'll move this back to the top-level CMakeLists.txt.

xudon9

Other changes look good to me.

xudon9 · 2023-12-28T06:34:50Z

CMakeLists.txt

-# We need to match the build environment for LLVM: In particular, we need C++14
-# and the -fno-rtti flag
-set(CMAKE_CXX_STANDARD 14)
-set(CMAKE_CXX_STANDARD_REQUIRED ON)


We can adjust the language standard here, like:

set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS ON)

Those properties will be effective for all new targets by default.

@Johanmyst could you add the options here and remove set_property in svf-llvm/CMakLiist.txt and svf/CMakeList.txt

…t script; removed target-specific setting of C++ standard

…ANDARD`

yuleisui · 2023-12-28T12:16:25Z

Merged and thanks @Johanmyst .

Johanmyst · 2023-12-28T12:16:50Z

No problem!

Modified CMakeLists.txt to take whether or not to disable RTTI & exce…

bdbae40

…ption handling from LLVM's configuration, rather than hardcoding

yuleisui reviewed Dec 20, 2023

View reviewed changes

Johanmyst and others added 6 commits December 20, 2023 09:48

Fixed setting of C++ standard and removed requirement for LLVM to be …

3959742

…built in debug-mode when SVF is being built in debug-mode

Fix matching parameters and return values for "OVERWRITE" functions a…

2c574c6

…nd add some new functions sepcfications in extapi.c

Remove getPointerElementType()

c24615f

The getPointerElementType will be deprecated in future versions of LLVM. Considering compatibility, avoid using getPointerElementType()->isIntegerTy(8) to determine if the arguments and return values are of the void * type.

SVF code formatter

8f01b0e

Take LLVMContext object from module used to build SVFModule rather th…

9ef3df0

…an forcing unique_ptr to avoid erronous garbage collection clearing LLVM internal objects when SVF is used from within an LLVM pass

Merge branch 'fix_ctx_src'

6e83d12

yuleisui reviewed Dec 20, 2023

View reviewed changes

Johanmyst added 2 commits December 20, 2023 09:58

Replaced fatal error for LLVM versions greater than 15 with a warning…

ca5632c

… to cater to future LLVM 16 support

Switched adding LLVM definitions, link directories, and include direc…

58a4ffc

…tories from only to `SvfLLVM` to global additions so that modules defined after `SvfLLVM` or in its subdirectories automatically inherit/can find LLVM as well

Switched to CMake-recommended way of linking libraries (through `targ…

af15d88

…et_link_libraries()`); linked SvfCore and SvfLLVM for tools; LLVM link directory should be added to rpath for tools

yuleisui reviewed Dec 20, 2023

View reviewed changes

Johanmyst added 3 commits December 20, 2023 13:30

Add the tools through add_llvm_executable; modify build/setup scrip…

3f51410

…ts to also include the installed LLVM/Z3 instances in $LD_LIBRARY_PATH (same for compiled SVF libraries)

Apparently this is not the case for older CMake versions; explicitly …

d0a1b83

…link LLVM libraries to avoid undefined symbols

Tests require specific output directory for tools

1fd01be

Johanmyst and others added 3 commits December 20, 2023 14:30

Merge branch 'fix_rtti_eh_from_llvm'

6cc852b

Merge branch 'SVF-tools:master' into fix_rtti_eh_from_llvm

6f1f351

Merge branch 'SVF-tools:master' into master

d4e6d6b

yuleisui reviewed Dec 21, 2023

View reviewed changes

yuleisui reviewed Dec 22, 2023

View reviewed changes

xudon9 reviewed Dec 22, 2023

View reviewed changes

Johanmyst and others added 2 commits December 22, 2023 18:38

Merge branch 'SVF-tools:master' into master

d8670ae

xudon9 reviewed Dec 28, 2023

View reviewed changes

xudon9 approved these changes Dec 28, 2023

View reviewed changes

Johanmyst and others added 3 commits December 28, 2023 12:06

Removed repetitive setting of C++ standard to top-level CMakeLists.tx…

d85eacb

…t script; removed target-specific setting of C++ standard

FIX: Replaced setting CXX_STANDARD (and related) with `CMAKE_CXX_ST…

1f54a14

…ANDARD`

Merge branch 'SVF-tools:master' into fix_rtti_eh_from_llvm

1063318

yuleisui merged commit 52b7034 into SVF-tools:master Dec 28, 2023
5 checks passed

Johanmyst deleted the fix_rtti_eh_from_llvm branch December 28, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `-fno-rtti`/`fno-exceptions` based on LLVM's configuration #1291

Add `-fno-rtti`/`fno-exceptions` based on LLVM's configuration #1291

Johanmyst commented Dec 19, 2023

yuleisui commented Dec 20, 2023

yuleisui Dec 20, 2023

Johanmyst Dec 20, 2023

yuleisui Dec 20, 2023

Johanmyst Dec 20, 2023

yuleisui Dec 20, 2023

Johanmyst Dec 20, 2023

Johanmyst commented Dec 20, 2023

yuleisui commented Dec 20, 2023

yuleisui Dec 20, 2023

Johanmyst Dec 20, 2023

yuleisui Dec 20, 2023

Johanmyst Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

yuleisui Dec 21, 2023

Johanmyst Dec 22, 2023

yuleisui Dec 21, 2023

Johanmyst Dec 22, 2023

Johanmyst Dec 22, 2023

yuleisui Dec 22, 2023

Johanmyst Dec 22, 2023

xudon9 Dec 22, 2023

Johanmyst Dec 22, 2023

yuleisui commented Dec 22, 2023

xudon9 Dec 28, 2023

Johanmyst Dec 28, 2023

xudon9 left a comment

xudon9 Dec 28, 2023

yuleisui Dec 28, 2023

Johanmyst Dec 28, 2023

yuleisui commented Dec 28, 2023

Johanmyst commented Dec 28, 2023

		add_compile_options("-fno-rtti")
		add_compile_options("-fno-exceptions")

Add -fno-rtti/fno-exceptions based on LLVM's configuration #1291

Add -fno-rtti/fno-exceptions based on LLVM's configuration #1291

Conversation

Johanmyst commented Dec 19, 2023

yuleisui commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johanmyst commented Dec 20, 2023

yuleisui commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Compile extapi.c to extapi.bc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuleisui commented Dec 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xudon9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuleisui commented Dec 28, 2023

Johanmyst commented Dec 28, 2023

Add `-fno-rtti`/`fno-exceptions` based on LLVM's configuration #1291

Add `-fno-rtti`/`fno-exceptions` based on LLVM's configuration #1291

codecov bot commented Dec 20, 2023 •

edited

Loading