Skip to content
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.

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

[contrib][vcl] Fix VCL builds with GCC #37075

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

krinkinmu
Copy link
Contributor

Commit Message:

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 abandoned this option
  4. Use --start-group and --end-group linker options to tell to the linker that all VPP static libraries should be considered together as a single unit - this is the option I implemented in the end.

Additional Description:

This is part of the work done to fix gcc builds of Envoy tracked in #31807. This change by itself does not address the issue completely yet, but it moves us a bit closer.

Risk Level: Low
Testing: builds with --config=gcc and --config=docker-gcc
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 8, 2024
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #37075 was opened by krinkinmu.

see: more, trace.

@krinkinmu
Copy link
Contributor Author

@florincoras This is what I ultimately ended up doing to make GCC builds of Envoy work. Please take a look.

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]>
@florincoras
Copy link
Member

Thanks @krinkinmu!

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @krinkinmu

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Nov 11, 2024
@phlax phlax merged commit 2c83bf8 into envoyproxy:main Nov 11, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants