Skip to content

Commit

Permalink
Fix build scripts forcing Debug builds. Add LTO mode and fix VW defau…
Browse files Browse the repository at this point in the history
…lt visibility. (#1735)

* Fix release build and unix symbol visibility. Add LTO mode.

Release builds were not working for two reasons.
First, it would always override -DCMAKE_BUILD_TYPE with "Debug"
Second, it would use CONFIG, which is not set on OSX with cmake 3.13.1 at least.

Finally, set the default symbol visibility to hidden on unix - linux is a misnomer in the build script...
Without it, we export something like 700 extra symbols that corresponds to all public functions in VW.
Exporting them on unix means making them available to be interposed and requiring an indirect call to be used
at all call sites.

The LTO mode, which can be used with -DLTO=1, enables clang's thinlto mode.

* Fix python module visiblity.

* Switch default to release.

* Handle Release + (PROFILE or GCOV). -pg and -fomit-frame-pointer are not compatible.
  • Loading branch information
kumpera authored and JohnLangford committed Feb 11, 2019
1 parent 4b84704 commit 18bddab
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 3 deletions.
30 changes: 27 additions & 3 deletions CMakeLists.txt
Expand Up @@ -2,6 +2,9 @@ cmake_minimum_required(VERSION 3.5)

# Only allow Release and Debug configurations
set(CMAKE_CONFIGURATION_TYPES Debug Release CACHE TYPE INTERNAL FORCE)
if(NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE "Release" CACHE STRING "Choose the type of build." FORCE)
endif()

project(vowpal_wabbit C CXX)
set(CMAKE_CXX_STANDARD 11)
Expand All @@ -21,7 +24,6 @@ ProcessorCount(NumProcessors)
message(STATUS "Number of processors: ${NumProcessors}")
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/nprocs.txt ${NumProcessors})

set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "Choose the type of build, options are: Debug, Release" FORCE)

option(PROFILE "Turn on flags required for profiling" OFF)
option(VALGRIND_PROFILE "Turn on flags required for profiling with valgrind" OFF)
Expand All @@ -34,11 +36,33 @@ option(BUILD_TESTS "Build and enable tests." ON)
option(BUILD_JAVA "Add Java targets." Off)
option(BUILD_PYTHON "Add Python targets." Off)
option(BUILD_DOCS "Add documentation targets." Off)
option(LTO "Enable Link Time optimization (Requires Release build, only works with clang and linux/mac for now)." Off)

string(TOUPPER "${CMAKE_BUILD_TYPE}" CONFIG)

# Add -ffast-math for speed, remove for testability.
set(linux_release_config -O3 -fomit-frame-pointer -fno-strict-aliasing -msse2 -mfpmath=sse)
set(linux_release_config -O3 -fno-strict-aliasing -msse2 -mfpmath=sse)
set(linux_debug_config -g -O0)
set(linux_flags $<$<CONFIG:DEBUG>:${linux_debug_config}> $<$<CONFIG:RELEASE>:${linux_release_config}>)

if((NOT PROFILE) AND (NOT GCOV))
set(linux_release_config ${linux_release_config} -fomit-frame-pointer)
endif()

#Use default visiblity on UNIX otherwise a lot of the C++ symbols end up for exported and interpose'able
set(linux_flags -fvisibility=hidden $<$<CONFIG:DEBUG>:${linux_debug_config}> $<$<CONFIG:RELEASE>:${linux_release_config}>)

if(LTO)
if(NOT "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
message(FATAL_ERROR "LTO requires Clang")
endif()
if("${CMAKE_CXX_COMPILER_VERSION}" VERSION_LESS "8.0.0")
message(FATAL_ERROR "LTO requires Clang 8.0 (llvm 3.9) or later")
endif()
If("${CONFIG}" STREQUAL "DEBUG")
message(FATAL_ERROR "LTO only works with Release builds")
endif()
set(linux_flags ${linux_flags} -flto=thin)
endif()

# for profiling -- note that it needs to be gcc
if(PROFILE)
Expand Down
7 changes: 7 additions & 0 deletions python/pylibvw.cc
@@ -1,4 +1,5 @@
#include "../vowpalwabbit/vw.h"

#include "../vowpalwabbit/multiclass.h"
#include "../vowpalwabbit/cost_sensitive.h"
#include "../vowpalwabbit/cb.h"
Expand All @@ -15,6 +16,10 @@
#include <boost/python.hpp>
#include <boost/python/suite/indexing/vector_indexing_suite.hpp>

//Brings VW_DLL_MEMBER to help control exports
#define VWDLL_EXPORTS
#include "../vowpalwabbit/vwdll.h"

using namespace std;
namespace py=boost::python;

Expand Down Expand Up @@ -678,6 +683,8 @@ void my_set_condition_range(predictor_ptr P, ptag hi, ptag count, char name0) {
void my_set_learner_id(predictor_ptr P, size_t id) { P->set_learner_id(id); }
void my_set_tag(predictor_ptr P, ptag t) { P->set_tag(t); }

//We need to forward declare this here to be able to add VW_DLL_MEMBER as BOOST_PYTHON_MODULE doesn't help
extern "C" VW_DLL_MEMBER void initpylibvw();

BOOST_PYTHON_MODULE(pylibvw)
{ // This will enable user-defined docstrings and python signatures,
Expand Down
6 changes: 6 additions & 0 deletions vowpalwabbit/vwdll.h
Expand Up @@ -30,10 +30,16 @@ license as described in the file LICENSE.
#define VW_DLL_MEMBER __declspec(dllimport)
#endif

#else

#ifdef VWDLL_EXPORTS
#define VW_DLL_MEMBER __attribute__ ((__visibility__ ("default")))
#else
#define VW_DLL_MEMBER
#endif

#endif

#ifdef __cplusplus
extern "C"
{
Expand Down

0 comments on commit 18bddab

Please sign in to comment.