From ef21fe9b02c4e096ad2602dcf76fed95f7a21029 Mon Sep 17 00:00:00 2001 From: Mikhail Krinkin Date: Fri, 8 Nov 2024 23:01:53 +0000 Subject: [PATCH] [contrib][vcl] Fix VCL builds with GCC There were a few issues with building Envoy with VCL. The fist issue is that vppinfra library is built with LTO enabled. While there is nothing wrong with enabling LTO, it apparently triggers some bug in GCC - during linking one of the LTO passes just consumes all the memory in the system and eventually crashes without finishing ( I tried to build Envoy on a system with 256GiB of memory and it wasn't enough, so it's way past what is reasonable). To workaround the issue I updated vpp_vcl.patch to conditionally disable LTO when building using GCC. Once LTO was disabled I hit another issue - the order of libraries in linker command line does matter, at least in the world of Unix-like systems. Normally, Bazel can figure out the right order, but with VPP static libraries that are built by CMake Bazel has no information to figure out what is the proper order of those libraries. And that ultimately resulted in linking failures. I considered a few options to address the issue: 1. Use alwayslink = True - while it should be the simplest and the least surprising solution to the problem, apparently, alwayslink does not do anything for static libraries, so this option does not work 2. Maintain the right order of libraries in the BUILD file - that works, but it's unusual when order of targets in Bazel srcs and deps matters, so to avoid surprising behaviour I didn't go for that option 3. Use genrule and combine different static libraries into a single static library - it should work in theory, but I couldn't refer to the ar tool from genrule and abandon this option. 4. Use --start-group and --end-group linker options to tell to the linker that all those static libraries should be considered a single unit - that is the option I implemented in this change. Signed-off-by: Mikhail Krinkin --- bazel/foreign_cc/vpp_vcl.patch | 64 +++++++++++++++++++++++++++------- contrib/vcl/source/BUILD | 23 ++++++++---- 2 files changed, 69 insertions(+), 18 deletions(-) diff --git a/bazel/foreign_cc/vpp_vcl.patch b/bazel/foreign_cc/vpp_vcl.patch index 2ce6b455ed04..74085b777cef 100644 --- a/bazel/foreign_cc/vpp_vcl.patch +++ b/bazel/foreign_cc/vpp_vcl.patch @@ -1,5 +1,5 @@ diff --git src/CMakeLists.txt src/CMakeLists.txt -index 68d0a4fe6..958918ef1 100644 +index 68d0a4f..9bf7ade 100644 --- src/CMakeLists.txt +++ src/CMakeLists.txt @@ -50,13 +50,8 @@ include(cmake/ccache.cmake) @@ -12,7 +12,7 @@ index 68d0a4fe6..958918ef1 100644 - OUTPUT_VARIABLE VPP_VERSION - OUTPUT_STRIP_TRAILING_WHITESPACE -) - + +set(VPP_VERSION 24.03-dev) if (VPP_PLATFORM) set(VPP_VERSION ${VPP_VERSION}-${VPP_PLATFORM_NAME}) @@ -28,7 +28,7 @@ index 68d0a4fe6..958918ef1 100644 elseif("${CMAKE_SYSTEM_NAME}" STREQUAL "Darwin") set(SUBDIRS vppinfra) diff --git src/cmake/ccache.cmake src/cmake/ccache.cmake -index a7b395bc6..d6a4c5b30 100644 +index a7b395b..d6a4c5b 100644 --- src/cmake/ccache.cmake +++ src/cmake/ccache.cmake @@ -14,7 +14,7 @@ @@ -41,26 +41,26 @@ index a7b395bc6..d6a4c5b30 100644 find_program(CCACHE_FOUND ccache) message(STATUS "Looking for ccache") diff --git src/cmake/library.cmake src/cmake/library.cmake -index 45b3944eb..b1dcc56e1 100644 +index 45b3944..b1dcc56 100644 --- src/cmake/library.cmake +++ src/cmake/library.cmake @@ -24,7 +24,7 @@ macro(add_vpp_library lib) set_target_properties(${lo} PROPERTIES POSITION_INDEPENDENT_CODE ON) target_compile_options(${lo} PUBLIC ${VPP_DEFAULT_MARCH_FLAGS}) - + - add_library(${lib} SHARED) + add_library(${lib} STATIC) target_sources(${lib} PRIVATE $) - + if(VPP_LIB_VERSION) diff --git src/tools/vppapigen/CMakeLists.txt src/tools/vppapigen/CMakeLists.txt -index 04ebed548..bfabc3a67 100644 +index 04ebed5..bfabc3a 100644 --- src/tools/vppapigen/CMakeLists.txt +++ src/tools/vppapigen/CMakeLists.txt @@ -11,22 +11,6 @@ # See the License for the specific language governing permissions and # limitations under the License. - + -find_package( - Python3 - REQUIRED @@ -81,7 +81,7 @@ index 04ebed548..bfabc3a67 100644 FILES vppapigen.py RENAME vppapigen diff --git src/tools/vppapigen/vppapigen.py src/tools/vppapigen/vppapigen.py -index 2b0ce9999..f28a17302 100755 +index 2b0ce99..f28a173 100755 --- src/tools/vppapigen/vppapigen.py +++ src/tools/vppapigen/vppapigen.py @@ -7,6 +7,13 @@ import logging @@ -97,17 +97,57 @@ index 2b0ce9999..f28a17302 100755 + import ply.lex as lex import ply.yacc as yacc - + diff --git src/vcl/CMakeLists.txt src/vcl/CMakeLists.txt -index 610b422d1..c5e6f8ca8 100644 +index 610b422..c5e6f8c 100644 --- src/vcl/CMakeLists.txt +++ src/vcl/CMakeLists.txt @@ -35,6 +35,8 @@ if (LDP_HAS_GNU_SOURCE) add_compile_definitions(HAVE_GNU_SOURCE) endif(LDP_HAS_GNU_SOURCE) - + +file(COPY vppcom.h DESTINATION ${CMAKE_LIBRARY_OUTPUT_DIRECTORY}) + add_vpp_library(vcl_ldpreload SOURCES ldp_socket_wrapper.c +diff --git src/vppinfra/CMakeLists.txt src/vppinfra/CMakeLists.txt +index f34ceed..51fd2be 100644 +--- src/vppinfra/CMakeLists.txt ++++ src/vppinfra/CMakeLists.txt +@@ -233,13 +233,28 @@ option(VPP_USE_EXTERNAL_LIBEXECINFO "Use external libexecinfo (useful for non-gl + if(VPP_USE_EXTERNAL_LIBEXECINFO) + set(EXECINFO_LIB execinfo) + endif() +-add_vpp_library(vppinfra +- SOURCES ${VPPINFRA_SRCS} +- LINK_LIBRARIES m ${EXECINFO_LIB} +- INSTALL_HEADERS ${VPPINFRA_HEADERS} +- COMPONENT libvppinfra +- LTO +-) ++ ++# GCC versions 11 and 12 at least have some kind of bug when ++# LTO runs out of memory and breaking Envoy builds with gcc ++# as a result. So we conditionally disable LTO on GCC builds ++# to workaround the issue. ++if(CMAKE_C_COMPILER_ID STREQUAL "GNU") ++ add_vpp_library(vppinfra ++ SOURCES ${VPPINFRA_SRCS} ++ LINK_LIBRARIES m ${EXECINFO_LIB} ++ INSTALL_HEADERS ${VPPINFRA_HEADERS} ++ COMPONENT libvppinfra ++ ) ++else() ++ add_vpp_library(vppinfra ++ SOURCES ${VPPINFRA_SRCS} ++ LINK_LIBRARIES m ${EXECINFO_LIB} ++ INSTALL_HEADERS ${VPPINFRA_HEADERS} ++ COMPONENT libvppinfra ++ LTO ++ ) ++endif() ++ + + ############################################################################## + # vppinfra headers diff --git a/contrib/vcl/source/BUILD b/contrib/vcl/source/BUILD index 8a5a5ca0c2b9..53c8bc7f7836 100644 --- a/contrib/vcl/source/BUILD +++ b/contrib/vcl/source/BUILD @@ -16,17 +16,25 @@ envoy_contrib_package() cc_library( name = "vpp_vcl", - srcs = [ + hdrs = ["external/vppcom.h"], + additional_linker_inputs = [ "external/libsvm.a", - "external/libvlibapi.a", "external/libvlibmemoryclient.a", + "external/libvlibapi.a", "external/libvppcom.a", "external/libvppinfra.a", - "external/vppcom.h", ], - hdrs = ["external/vppcom.h"], defines = ["VPP_VCL"], includes = ["external/"], + linkopts = [ + "-Wl,--start-group", + "$(location external/libsvm.a)", + "$(location external/libvlibmemoryclient.a)", + "$(location external/libvlibapi.a)", + "$(location external/libvppcom.a)", + "$(location external/libvppinfra.a)", + "-Wl,--end-group", + ], tags = ["skip_on_windows"], visibility = ["//visibility:public"], ) @@ -40,7 +48,10 @@ envoy_cmake( "BUILD_SHARED_LIBS": "OFF", "CMAKE_ENABLE_EXPORTS": "OFF", }, - copts = ["-Wno-unused-variable"], + copts = [ + "-Wno-unused-variable", + "-Wno-error=array-bounds", + ], default_cache_entries = {}, env = { "PLYPATHS": "$(locations %s)" % requirement("ply"), @@ -73,8 +84,8 @@ genrule( name = "build_files", outs = [ "external/libsvm.a", - "external/libvlibapi.a", "external/libvlibmemoryclient.a", + "external/libvlibapi.a", "external/libvppcom.a", "external/libvppinfra.a", "external/vppcom.h",