Skip to content

Commit

Permalink
[contrib][vcl] Fix VCL builds with GCC
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
krinkinmu committed Nov 8, 2024
1 parent 1922c84 commit 94c8e14
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 17 deletions.
64 changes: 52 additions & 12 deletions bazel/foreign_cc/vpp_vcl.patch
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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})
Expand All @@ -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 @@
Expand All @@ -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 $<TARGET_OBJECTS:${lo}>)

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
Expand All @@ -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
Expand All @@ -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
18 changes: 13 additions & 5 deletions contrib/vcl/source/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@ envoy_contrib_package()

cc_library(
name = "vpp_vcl",
srcs = [
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"],
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",
],
defines = ["VPP_VCL"],
includes = ["external/"],
tags = ["skip_on_windows"],
Expand All @@ -40,7 +48,7 @@ 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"),
Expand Down Expand Up @@ -73,8 +81,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",
Expand Down

0 comments on commit 94c8e14

Please sign in to comment.