build: remove cc_configure and cc_wrapper #7555

lizan · 2019-07-12T04:31:15Z

Signed-off-by: Lizan Zhou [email protected]

Description:
Use BAZEL_LINKLIBS to statically link libstdc++ and libc++, get rid of cc_wrapper. Requires Bazel >= 0.28. Additional compiler options which was in cc_wrapeer moved into envoy_copts with select.

Side effect: static linking libstdc++ applies to all binaries built with the toolchain, so tests are now statically linked with libstdc++.

Risk Level: Med, affecting depending projects
Testing: CI
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lizan Zhou <[email protected]>

jmillikin-stripe · 2019-07-12T04:36:34Z

.bazelrc

@@ -8,6 +8,8 @@ startup --host_jvm_args=-Xmx512m
 build --workspace_status_command=bazel/get_workspace_status
 build --experimental_remap_main_repo
 build --host_force_python=PY2
+build --action_env=BAZEL_LINKLIBS=-l%:libstdc++.a


What effect does this have on macOS or Windows builds? Is there any way to set it only on Linux?

From my read of https://github.com/bazelbuild/bazel/tree/0.28.0/tools/cpp this env doesn't have any effect on configure_osx_toolchain or configure_windows_toolchain so it should be no-op.

Signed-off-by: Lizan Zhou <[email protected]>

jmillikin-stripe

LGTM

Signed-off-by: Lizan Zhou <[email protected]>

htuch · 2019-07-12T12:32:40Z

bazel/cc_wrapper.py

-    # it in clang builds causes a build error because of unknown command line
-    # flag.
-    # See https://github.com/envoyproxy/envoy/issues/2987
-    argv.append("-Wno-maybe-uninitialized")


In principle, this PR is really great, it will hugely simplify build. One downside is we lose an interception point to workaround Bazel issues, but I guess we can always introduce it again later. Beyond libc++ handling, it looks like this line is something that we were using cc_wrapper for previously, what happens to this?

In my TODO above:

migrate compiler options to envoy_copts with select

will update for this and make it ready for review.

Signed-off-by: Lizan Zhou <[email protected]>

lizan · 2019-07-15T23:11:48Z

@htuch this is ready to review, PTAL again

htuch · 2019-07-16T04:18:58Z

test/integration/stats_integration_test.cc

@@ -210,6 +210,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) {
  // 2019/06/29  7364     45685       46000   combine 2 levels of stat ref-counting into 1
  // 2019/06/30  7428     42742       43000   remove stats multiple inheritance, inline HeapStatData
  // 2019/07/06  7477     42742       43000   fork gauge representation to drop pending_increment_
+  // 2019/07/15  7555     42806       43000   static link libstdc++ in tests


How did test behavior change in the PR? I thought we were just removing wrappers.

This changes how we link libstdc++ in tests, previously, libstdc++ is dynamically linked for tests as envoy_static_link_libstdcpp_linkopts isn't added to envoy_cc_test linkopts. With this PR tests are linked with libstdc++ too so the number changed. Current master with following patch produces same number:

diff --git a/bazel/envoy_test.bzl b/bazel/envoy_test.bzl index ea564d619..c502fa007 100644 --- a/bazel/envoy_test.bzl +++ b/bazel/envoy_test.bzl @@ -150,7 +150,7 @@ def envoy_cc_test( native.cc_test( name = name, copts = envoy_copts(repository, test = True) + copts, - linkopts = _envoy_test_linkopts(), + linkopts = _envoy_test_linkopts() + envoy_static_link_libstdcpp_linkopts(), linkstatic = envoy_linkstatic(), malloc = tcmalloc_external_dep(repository), deps = [

Do we need to do this at the same time as this PR? If so, please update the top-level GH comment so we can also explain that this is happening.

Yes because BAZEL_LINKLIBS is toolchain configuration so applies to all binary. Updated description.

Signed-off-by: Lizan Zhou <[email protected]>

htuch

Thanks!

PiotrSikora · 2019-07-17T02:13:46Z

This seems to break foreign_cc C++ targets (e.g. llvm in envoyproxy/envoy-wasm), e.g.

Performing C++ SOURCE FILE Test HAVE_CXX_ATOMICS_WITHOUT_LIB failed with the following output:
Change Dir: /tmp/tmp.IkzQVESUR0/CMakeFiles/CMakeTmp

Run Build Command(s):/usr/bin/ninja cmTC_61032
[1/2] Building CXX object CMakeFiles/cmTC_61032.dir/src.cxx.o
[2/2] Linking CXX executable cmTC_61032
FAILED: cmTC_61032
: && /usr/lib/llvm-8/bin/clang  -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -fcolor-diagnostics -fno-omit-frame-pointer -std=c++0x -no-canonical-prefixes -Wno-builtin-macro-redefined -D__DATE__="redacted" -D__TIMESTAMP__="redacted" -D__TIME__="redacted" -UDEBUG -DHAVE_CXX_ATOMICS_WITHOUT_LIB  -Werror=unguarded-availability-new -std=c++11  -fuse-ld=gold -Wl,-no-as-needed -Wl,-z,relro,-z,now -B/usr/lib/llvm-8/bin -lm -static-libgcc -l:libstdc++.a CMakeFiles/cmTC_61032.dir/src.cxx.o  -o cmTC_61032  -lm && :
CMakeFiles/cmTC_61032.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to '__cxa_begin_catch'
CMakeFiles/cmTC_61032.dir/src.cxx.o:src.cxx:function __clang_call_terminate: error: undefined reference to 'std::terminate()'
CMakeFiles/cmTC_61032.dir/src.cxx.o(.eh_frame+0x12b): error: undefined reference to '__gxx_personality_v0'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

Source file was:

#include <atomic>
std::atomic<int> x;
int main() {
  return x;
}

There might be some other silent failures, due to optional features that can no longer be detected.

@lizan any ideas?

lizan · 2019-07-17T05:27:02Z

@PiotrSikora hmm, not sure why #7329 didn't break this, cc_wrapper -std=c++11 atomic.cc broke too. This patch seems work, you're only using static libraries from LLVM so should be no-op to that:

diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD
index e031e6a1c..d010a34f0 100644
--- a/bazel/foreign_cc/BUILD
+++ b/bazel/foreign_cc/BUILD
@@ -142,6 +142,7 @@ envoy_cmake_external(
         "LLVM_ENABLE_ZLIB": "off",
         "LLVM_TARGETS_TO_BUILD": "X86",
         "CMAKE_BUILD_TYPE": "RelWithDebInfo",
+        "CMAKE_CXX_FLAGS": "-lstdc++",
     },
     env_vars = {
         # Workaround for the -DDEBUG flag added in fastbuild on macOS,

Signed-off-by: Lizan Zhou <[email protected]>

build: remove cc_configure and cc_wrapper

b0628db

Signed-off-by: Lizan Zhou <[email protected]>

lizan requested a review from jmillikin-stripe July 12, 2019 04:31

lizan assigned htuch Jul 12, 2019

jmillikin-stripe reviewed Jul 12, 2019

View reviewed changes

lizan added 2 commits July 12, 2019 04:51

-lm

48fb40f

Signed-off-by: Lizan Zhou <[email protected]>

format

b44f716

Signed-off-by: Lizan Zhou <[email protected]>

jmillikin-stripe previously approved these changes Jul 12, 2019

View reviewed changes

asan tweak

aeffc64

Signed-off-by: Lizan Zhou <[email protected]>

lizan dismissed jmillikin-stripe’s stale review via aeffc64 July 12, 2019 07:38

htuch reviewed Jul 12, 2019

View reviewed changes

htuch added the waiting:any label Jul 12, 2019

repokitteh-read-only bot removed the waiting:any label Jul 12, 2019

htuch added the waiting label Jul 12, 2019

lizan added 3 commits July 15, 2019 23:07

select copts, fix memory test

1a81475

Signed-off-by: Lizan Zhou <[email protected]>

Merge remote-tracking branch 'upstream/master' into no_wrapper

c12d291

Merge remote-tracking branch 'upstream/master' into no_wrapper

b81d0d8

repokitteh-read-only bot removed the waiting label Jul 15, 2019

lizan marked this pull request as ready for review July 15, 2019 23:11

htuch reviewed Jul 16, 2019

View reviewed changes

htuch added the waiting:any label Jul 16, 2019

real memory fix

8f6c92f

Signed-off-by: Lizan Zhou <[email protected]>

repokitteh-read-only bot removed the waiting:any label Jul 16, 2019

htuch approved these changes Jul 16, 2019

View reviewed changes

htuch merged commit 14dd85d into envoyproxy:master Jul 16, 2019

lizan deleted the no_wrapper branch July 16, 2019 20:04

lizan added a commit to lizan/envoy-filter-example that referenced this pull request Jul 17, 2019

ci: update WORKSPACE from envoyproxy/envoy#7555

ac3c64f

Signed-off-by: Lizan Zhou <[email protected]>

lizan added a commit to envoyproxy/envoy-filter-example that referenced this pull request Jul 17, 2019

ci: update WORKSPACE from envoyproxy/envoy#7555 (#93)

f37fb53

Signed-off-by: Lizan Zhou <[email protected]>

lizan mentioned this pull request Jul 19, 2019

Potential memory leak in dynamic_ot tracers #7647

Closed

Yikun mentioned this pull request Jul 23, 2019

Build: bazel build failed on aarch64/arm64 #7686

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: remove cc_configure and cc_wrapper #7555

build: remove cc_configure and cc_wrapper #7555

lizan commented Jul 12, 2019 •

edited

Loading

jmillikin-stripe Jul 12, 2019

lizan Jul 12, 2019

jmillikin-stripe left a comment

htuch Jul 12, 2019

lizan Jul 12, 2019

lizan commented Jul 15, 2019

htuch Jul 16, 2019

lizan Jul 16, 2019

htuch Jul 16, 2019

lizan Jul 16, 2019

htuch left a comment

PiotrSikora commented Jul 17, 2019 •

edited

Loading

lizan commented Jul 17, 2019

build: remove cc_configure and cc_wrapper #7555

build: remove cc_configure and cc_wrapper #7555

Conversation

lizan commented Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmillikin-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizan commented Jul 15, 2019

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

htuch left a comment

Choose a reason for hiding this comment

PiotrSikora commented Jul 17, 2019 • edited Loading

lizan commented Jul 17, 2019

lizan commented Jul 12, 2019 •

edited

Loading

PiotrSikora commented Jul 17, 2019 •

edited

Loading