Rewriting HeaderString to use absl::variant. See #9593 by adisuissa · Pull Request #9952 · envoyproxy/envoy

adisuissa · 2020-02-06T21:53:36Z

Signed-off-by: Adi Suissa-Peleg adip@google.com

Description: Rewriting HeaderString to use absl::variant instead of union. Removed Type::Dynamic, updated the implementation, and tests.
Risk Level: Low/Medium - being used by the header_map, et al.
Testing: Kept and updated most unit tests as before. Removed one unit test that accessed a region of memory that was out of scope.
Docs Changes:
Release Notes:
Fixes #9593

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-02-06T21:58:23Z

Also ran the perf tests for this PR (test/common/http:header_map_impl_speed_test).
There's some performance hit, and further investigation might be required.

mattklein123 · 2020-02-07T01:10:07Z

Thanks for working on this. Haven't reviewed yet, but just wanted to say that I'm not surprised there is some perf delta, though it would be worth it to understand where the delta is maybe by taking a look at a cachegrind run? IMO some perf delta is probably worth it given the code simplification but very happy to hear what others think on this.

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-02-07T01:57:26Z

I'll look into the detailed results of cachegrind and submit the fixed PR tomorrow morning.

asraa · 2020-02-07T14:47:11Z

source/common/http/header_map_impl.cc


-void HeaderString::clear() {
-  switch (type_) {
+char* HeaderString::buffer() {


I'm assuming this is for the safety of not modifying a const reference string, correct? We might just want to get rid of this non-const accessor entirely.

I think the only call-site of this method is in the H/1 codec where we access the buffer and lower-case it. Can we just append lower-cased data to the HeaderString instead of lower-casing the appended data AND GET RID OF THIS METHOD FOREVER?!

IIUC it is also being used by HeaderMapImpl::addViaMove

envoy/source/common/http/header_map_impl.cc

Lines 394 to 395 in e176b30

key.clear();

value.clear();

I think the next step is to refactor header_map, so I'll remember this comment, and see if we can remove the clear method.

I meant the buffer() method!

+1 please kill it if possible.

I've removed the buffer() method.
When thinking how to add only lowercase strings, I was faced with 2 options, as the given data is const (

envoy/source/common/http/http1/codec_impl.cc

Line 473 in e176b30

void ConnectionImpl::onHeaderField(const char* data, size_t length) {

)

Allocate a new buffer, copy data, apply the lowercase function, and append the outcome to the HeaderString (=> will copy the data twice).

Append the original data (that may require lowercasing), and apply a "transform" function on the HeaderString (=> will copy the data once, and then lowercase in-place).
I've decided to use option 2, and introduce a new method called inlineTransform, that calls std::transform on the data. It should use less memory.

Let me know if you think I should use option 1, or if you have a better idea.

Ugh. Option (1) is pretty bad. It bothers me that there are at least 3 ways we tolower strings, including (1) LowerCaseString with ctype.h tolower, (2) the fast lookup ToLowerTable like here, and (3) StringUtil::toLower using absl::ascii_tolower (and also the absl::AsciiStrToLower string version)

Curious, is (3) fast? It seems to something similar to our lower table. CAN WE GET RID OF THE LOWER TABLE TOO AND JUST USE absl::ascii_tolower AS THE UNARY_OP?

Welcome to leave as TODO since (2) gives the option of switching out the unary op anyway

Good point. I'll change it to use absl::ascii_tolower, and get rid of the lower table.

source/common/http/header_map_impl.cc

… See envoyproxy#9593 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123

Awesome! Really great to see this happening. Love the red lines. Flushing out some comments from a first round and then I will take another pas. Thank you!

/wait

mattklein123 · 2020-02-07T21:55:52Z

include/envoy/http/header_map.h

@@ -89,12 +90,12 @@ using LowerCaseStrPairVector =
 * This is a string implementation for use in header processing. It is heavily optimized for
 * performance. It supports 3 different types of storage and can switch between them:


nit: 2 different types

mattklein123 · 2020-02-07T21:57:02Z

include/envoy/http/header_map.h

   * @return the type of backing storage for the string.
   */
-  Type type() const { return type_; }
+  Type type() const { return Type(buffer_.index()); }


I thought you mentioned that we can get rid of this now?

Sorry, I meant that we can get rid of the "Dynamic" type.
I'll look more closely if we can remove the type() method, as it seems to only be used at:

envoy/source/common/http/http2/codec_impl.cc

Lines 64 to 71 in d1db564

static void insertHeader(std::vector<nghttp2_nv>& headers, const HeaderEntry& header) {

uint8_t flags = 0;

if (header.key().type() == HeaderString::Type::Reference) {

flags |= NGHTTP2_NV_FLAG_NO_COPY_NAME;

}

if (header.value().type() == HeaderString::Type::Reference) {

flags |= NGHTTP2_NV_FLAG_NO_COPY_VALUE;

}

Removed access to type() and the Type enum is now private.
Added the isReference method that is used by the codec.

mattklein123 · 2020-02-07T21:58:28Z

source/common/http/header_map_impl.cc

-  static_assert(sizeof(inline_buffer_) >= MaxIntegerLength, "");
+// Initialize as a Type::Inline
+HeaderString::HeaderString() : buffer_(absl::InlinedVector<char, 128>()), string_length_(0) {
+  static_assert(sizeof(buffer_) >= MaxIntegerLength, "");


Does this make sense anymore? Should you be checking the size of the inline storage somehow? Effectively the minimum size of the vector backing data?

mattklein123 · 2020-02-07T22:00:24Z

source/common/http/header_map_impl.cc

-  }
-  }
+HeaderString::HeaderString(HeaderString&& move_value) noexcept
+    : buffer_(move_value.buffer_), string_length_(move_value.string_length_) {


I think this might copy. I would store the right hand side string length in a local variable and then do an explicit std::move?

Also removed string_length_, as it is not really necessary anymore.

mattklein123 · 2020-02-07T22:01:08Z

source/common/http/header_map_impl.cc

+#define BUFFER_STR_VIEW_GET absl::get<absl::string_view>(buffer_)
+#define BUFFER_IN_VEC_GET absl::get<absl::InlinedVector<char, 128>>(buffer_)


nit: prefer functions over macros.

mattklein123 · 2020-02-07T22:01:54Z

source/common/http/header_map_impl.cc

+  uint64_t new_capacity = static_cast<uint64_t>(size) + string_length_;
+  if (new_capacity < MinDynamicCapacity) {
+    new_capacity = MinDynamicCapacity;
  }
+  validateCapacity(new_capacity);


I don't think any of this should be needed anymore?

Kept validateCapacity to avoid OOM when appending.
Let me know if you think it is unnecessary.

mattklein123 · 2020-02-07T22:02:25Z

source/common/http/header_map_impl.cc


-void HeaderString::clear() {
-  switch (type_) {
+char* HeaderString::buffer() {


+1 please kill it if possible.

mattklein123 · 2020-02-07T22:03:36Z

source/common/http/header_map_impl.cc

-    buffer_.dynamic_ = inline_buffer_;
+  const uint32_t max_buffer_length = 32;
+  char inner_buffer[max_buffer_length];
+  string_length_ = StringUtil::itoa(inner_buffer, max_buffer_length, value);


You should be able to itoa directly into the buffer after reserving it and avoid the copy?

I've attempted the following:

BUFFER_IN_VEC_GET.resize(MaxIntegerLength); const uint32_t int_length = StringUtil::itoa(BUFFER_IN_VEC_GET.data(), MaxIntegerLength, value); BUFFER_IN_VEC_GET.resize(int_length);

but unfortunately it has ~100ns perf penalty (the first resize also initializes the 32 cells, and the second one "destroys" the unused ones).
If you think of a better way, let me know and I'll implement it and test it out.

… header_map_simplify Signed-off-by: Adi Suissa-Peleg <adip@google.com>

… See envoyproxy#9593 Cleaning up unneeded members, hiding Type enum, removing type() and data() methods, and introducing the inlineTransform method. Signed-off-by: Adi Suissa-Peleg <adip@google.com>

adisuissa · 2020-02-11T17:00:51Z

After removing some unnecessary things, fixing some small issues, and running the performance tests (test/common/http:header_map_impl_speed_test), we've got the following results (new version called "w/ absl::variant 2"):

Seems that most of the functions are a bit faster than the original implementation.
There's some penalty for the SetInteger tests and the HeaderMapImplPopulate test.

mattklein123 · 2020-02-12T20:17:04Z

Sorry do you mind merging master and I will take another pass?

/wait

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123 · 2020-02-13T19:07:02Z

Let me know when you are done addressing @asraa comments and I will take a look. Thank you!

/wait

adisuissa · 2020-02-13T19:11:29Z

@mattklein123 I think it would be better to remove to_lower_table and call ascii_tolower instead, in a different PR, as it has implications on other parts of the code.

mattklein123 · 2020-02-13T19:15:42Z

Sounds good I will review.

mattklein123

Thanks this is really awesome. LGTM with some small nits. I will defer to @asraa for further review and merge. Can you also talk to @asraa about running the fuzzer locally a bit just to get ahead of any super obvious issues before we merge to master? Thank you!

mattklein123 · 2020-02-13T19:23:19Z

include/envoy/http/header_map.h

+  /**
+   * @return the type of backing storage for the string.
+   */
+  Type type() const { return Type(buffer_.index()); }


Can you add a comment above near Type/buffer_ that this function relies on the order of the enum matching the variant order? Potentially fragile? Potentially add some assert around this? Up to you on the assert.

I like the idea of the assert, should make it more robust. Will add it.

mattklein123 · 2020-02-13T19:24:00Z

source/common/http/header_map_impl.cc


-} // namespace
+absl::string_view
+get_str_view(const absl::variant<absl::string_view, absl::InlinedVector<char, 128>>& buffer) {


nit: maybe add a using declaration to alias absl::variant<absl::string_view, absl::InlinedVector<char, 128>>? You could also sub alias absl::InlinedVector<char, 128>

I used a macro before. Do you suggest a typedef or something else?

using InlineHeaderVector = absl::InlinedVector<char, 128>;
using VariantHeader = absl::variant<absl::string_view, InlineHeaderVector>;

Etc.

Will do. Thanks for the guidance.

mattklein123 · 2020-02-13T19:25:27Z

source/common/http/header_map_impl.cc

+  ASSERT(valid());
 }

+HeaderString::~HeaderString() {}


mattklein123 · 2020-02-13T19:35:09Z

source/common/http/header_map_impl.cc

-    buffer_.dynamic_ = buf;
+    // Rather than be too clever and optimize this uncommon case, we switch to
+    // Inline mode and copy.
+    absl::string_view prev = get_str_view(buffer_);


nit: const, elsewhere where applicable if possible.

mattklein123 · 2020-02-13T19:37:02Z

source/common/http/header_map_impl.cc

+  // Initialize the size to the max length, copy the actual data, and then
+  // reduce the size (but not the capacity) as needed


Can you add a small note here about your perf experiment so that someone doesn't come along and ask the same question as I had? It's not intuitive to me that this is after versus an inline write, but I certainly believe your results.

mattklein123 · 2020-02-13T19:38:02Z

source/common/http/http1/codec_impl.cc


  if (!current_header_field_.empty()) {
-    toLowerTable().toLowerCase(current_header_field_.buffer(), current_header_field_.size());
+    current_header_field_.inlineTransform([](char c) { return static_cast<uint8_t>(tolower(c)); });


Does this change have a perf implication? Should we do the absl thing here now?

Changed to abseil's tolower version (ascii_tolower) now.
Compared performance of absl::to_lower to toLowerCase (not going to commit the test, just adding the results, to save these datapoints). absl is a bit faster, probably due to optimizations, so we should use it.

ToLowerTableEnvoyLowerCase/0 2.11 ns 2.11 ns 280473610 ToLowerTableEnvoyLowerCase/1 4.17 ns 4.17 ns 122310238 ToLowerTableEnvoyLowerCase/10 8.98 ns 8.98 ns 78490730 ToLowerTableEnvoyLowerCase/50 26.3 ns 26.3 ns 26710790 ToLowerTableEnvoyLowerCase/200 81.3 ns 81.3 ns 8631624 ToLowerTableAbseilLowerCase/0 1.91 ns 1.91 ns 365400798 ToLowerTableAbseilLowerCase/1 3.68 ns 3.68 ns 190285175 ToLowerTableAbseilLowerCase/10 8.94 ns 8.94 ns 77216159 ToLowerTableAbseilLowerCase/50 24.1 ns 24.1 ns 28907364 ToLowerTableAbseilLowerCase/200 72.6 ns 72.6 ns 9664473

include/envoy/http/header_map.h

…stead of tolower Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

asraa · 2020-02-14T20:52:20Z

Fuzz approved, thanks for adding coverage for both inline/reference header fields and inlineTransform.

mattklein123

Awesome work!!! Love the simplification here.

moderation · 2020-02-18T09:22:27Z

Looks like a side effect of this PR is the building on RHEL 7 now breaks. Error is below building on RHEL 7 with gcc 8 and bazel build -c opt --copt=-DENVOY_IGNORE_GLIBCXX_USE_CXX11_ABI_ERROR=1 //source/exe:envoy-static.stripped. Any workarounds possible? /cc @asraa

ERROR: /home/<USERNAME>/Library/envoyproxy/envoy/source/common/http/BUILD:256:1: C++ compilation of rule '//source/common/http:header_map_lib' failed (Exit 1) gcc failed: error executing command /opt/rh/devtoolset-8/root/usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g0 -O2 '-D_FORTIFY_SOURCE=1' -DNDEBUG ... (remaining 86 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
In file included from external/com_google_absl/absl/container/inlined_vector.h:53,
                 from bazel-out/k8-opt/bin/include/envoy/http/_virtual_includes/header_map_interface/envoy/http/header_map.h:18,
                 from bazel-out/k8-opt/bin/source/common/http/_virtual_includes/header_map_lib/common/http/header_map_impl.h:9,
                 from source/common/http/header_map_impl.cc:1:
external/com_google_absl/absl/container/internal/inlined_vector.h: In constructor 'Envoy::Http::HeaderString::HeaderString()':
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' is used uninitialized in this function [-Werror=uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::setInteger(uint64_t)':
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::setCopy(const char*, uint32_t)':
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::append(const char*, uint32_t)':
external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     data_ = other_storage.data_;
     ^~~~~
cc1plus: all warnings being treated as errors
Target //source/exe:envoy-static.stripped failed to build

adisuissa · 2020-02-18T12:01:23Z

Thanks. Interesting, it seems that the field might not be initialized in these settings. I'll try to see if the error can be reproduced or if the member can be explicitly initialized.

…

On Tue, Feb 18, 2020, 04:22 moderation ***@***.***> wrote: Looks like a side effect of this PR is the building on RHEL 7 now breaks. Error is below building on RHEL 7 with gcc 8 and bazel build -c opt --copt=-DENVOY_IGNORE_GLIBCXX_USE_CXX11_ABI_ERROR=1 //source/exe:envoy-static.stripped. Any workarounds possible? /cc @asraa <https://github.com/asraa> ERROR: /home/<USERNAME>/Library/envoyproxy/envoy/source/common/http/BUILD:256:1: C++ compilation of rule '//source/common/http:header_map_lib' failed (Exit 1) gcc failed: error executing command /opt/rh/devtoolset-8/root/usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -g0 -O2 '-D_FORTIFY_SOURCE=1' -DNDEBUG ... (remaining 86 argument(s) skipped) Use --sandbox_debug to see verbose messages from the sandbox In file included from external/com_google_absl/absl/container/inlined_vector.h:53, from bazel-out/k8-opt/bin/include/envoy/http/_virtual_includes/header_map_interface/envoy/http/header_map.h:18, from bazel-out/k8-opt/bin/source/common/http/_virtual_includes/header_map_lib/common/http/header_map_impl.h:9, from source/common/http/header_map_impl.cc:1: external/com_google_absl/absl/container/internal/inlined_vector.h: In constructor 'Envoy::Http::HeaderString::HeaderString()': external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' is used uninitialized in this function [-Werror=uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::setInteger(uint64_t)': external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::setCopy(const char*, uint32_t)': external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ external/com_google_absl/absl/container/internal/inlined_vector.h: In member function 'void Envoy::Http::HeaderString::append(const char*, uint32_t)': external/com_google_absl/absl/container/internal/inlined_vector.h:437:5: error: '<anonymous>.absl::inlined_vector_internal::Storage<char, 128, std::allocator<char> >::data_' may be used uninitialized in this function [-Werror=maybe-uninitialized] data_ = other_storage.data_; ^~~~~ cc1plus: all warnings being treated as errors Target //source/exe:envoy-static.stripped failed to build — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9952?email_source=notifications&email_token=ABEQZZZCRXCTQN5NPXCC3V3RDOSFHA5CNFSM4KRETFU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMBGL6Y#issuecomment-587359739>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABEQZZYVSMI4GTUSR5O4NWDRDOSFHANCNFSM4KRETFUQ> .

asraa · 2020-02-18T14:23:40Z

Looks like a side effect of this PR is the building on RHEL 7 now breaks. Error is below building on RHEL 7 with gcc 8 and bazel build -c opt --copt=-DENVOY_IGNORE_GLIBCXX_USE_CXX11_ABI_ERROR=1 //source/exe:envoy-static.stripped. Any workarounds possible? /cc @asraa

Do you get the same error running external/com_google_absl/absl/container/inlined_vector_test.cc?

adisuissa · 2020-02-19T03:38:40Z

@moderation I've attempted to compile it with GCC, but couldn't reproduce the problem.
The errors seem to be coming from abseil. If you can provide me with a docker image to test this on, I may be able to help with this.

moderation · 2020-02-21T09:54:28Z

@adisuissa I think the issue is specific to RHEL 7 (and possibly Centos) due to the ancient glibc. I suspect using GCC 8 on something like Ubuntu will trigger the issue.

@asraa can you give me a hint on how to run that specific test? What is the Bazel label that I would use as outlined at https://github.com/envoyproxy/envoy/tree/master/bazel#testing-envoy-with-bazel ?

asraa · 2020-02-21T13:41:57Z

It might be hackish, but I cd'd in to my bazel cache, in to the external/com_google_absl directory, I copied the WORKSPACE from https://github.com/abseil/abseil-cpp/blob/master/WORKSPACE because I guess pulling external deps writes over their WORKSPACE, and ran bazel test absl/container:inlined_vector_test and all was well

moderation · 2020-02-21T15:39:32Z

That worked @asraa ! I confirmed the test passed on my Xubuntu 19.10 with Clang 9. Output from work RHEL 7 is below:

INFO: Analyzed target //absl/container:inlined_vector_test (31 packages loaded, 478 targets configured).
INFO: Found 1 test target...
ERROR: /home/<USERNAME>/.cache/bazel/_bazel_<USERNAME>@<DOMAIN>/3cccb42b5a473b17439089ce85d34673/external/com_google_absl/absl/container/BUILD.bazel:147:1: Linking of rule '//absl/container:inlined_vector_test' failed (Exit 1) gcc failed: error executing command /opt/rh/devtoolset-8
/root/usr/bin/gcc @bazel-out/k8-fastbuild/bin/absl/container/inlined_vector_test-2.params

Use --sandbox_debug to see verbose messages from the sandbox
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::logic_error::logic_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::invalid_argument::invalid_argument(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::domain_error::domain_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::length_error::length_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::out_of_range::out_of_range(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::runtime_error::runtime_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::range_error::range_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::overflow_error::overflow_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libabsl_Sbase_Slibthrow_Udelegate.so: error: undefined reference to 'std::underflow_error::underflow_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libexternal_Scom_Ugoogle_Ugoogletest_Slibgtest.so: error: undefined reference to 'std::runtime_error::runtime_error(char const*)'
bazel-out/k8-fastbuild/bin/_solib_k8/libexternal_Scom_Ugoogle_Ugoogletest_Slibgtest.so: error: undefined reference to 'std::__throw_out_of_range_fmt(char const*, ...)'
bazel-out/k8-fastbuild/bin/absl/container/_objs/inlined_vector_test/inlined_vector_test.pic.o:inlined_vector_test.cc:function std::istream_iterator<int, char, std::char_traits<char>, long>::_M_read(): error: undefined reference to 'std::basic_ios<char, std::char_traits<char$ >::operator bool() const'
bazel-out/k8-fastbuild/bin/absl/container/_objs/inlined_vector_test/inlined_vector_test.pic.o:inlined_vector_test.cc:function std::istream_iterator<int, char, std::char_traits<char>, long>::_M_read(): error: undefined reference to 'std::basic_ios<char, std::char_traits<char$ >::operator bool() const'
collect2: error: ld returned 1 exit status
Target //absl/container:inlined_vector_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 33.227s, Critical Path: 9.28s
INFO: 69 processes: 69 linux-sandbox.
FAILED: Build did NOT complete successfully
//absl/container:inlined_vector_test                            FAILED TO BUILD

FAILED: Build did NOT complete successfully

asraa · 2020-02-24T15:02:23Z

Oh jeez. I was hoping to reproduce the same error frm the test, but it looks like building the absl test fails in itself. Maybe @adisuissa can write a simple inlinedvector test (no headermap stuff, just replicating the initialization and usage of variant and inlined vector in headermap) for you to add to an Envoy test and run in your environment?

moderation · 2020-02-25T04:59:48Z

Big thanks to @asraa and @adisuissa for helping out on this one. I was able to install GCC 9.1.1 in my Red Hat 7 environment and now I'm able to compile without errors using bazel build -c opt --copt=-DENVOY_IGNORE_GLIBCXX_USE_CXX11_ABI_ERROR=1 //source/exe:envoy-static.stripped and the resultant binary runs on RHEL7.

surki · 2020-03-23T06:10:42Z

FWIW: I am still getting this error in debian-bullseye (gcc 9.x), I have to use the following options:

--copt="-Wno-redundant-move"

and in debian-buster (gcc 8.x):

--copt="-Wno-maybe-uninitialized" --copt="-Wno-uninitialized"

Rewriting HeaderString to use stdl::variant. See envoyproxy#9593

46fdbe7

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123 assigned mattklein123 and asraa Feb 7, 2020

Rewriting HeaderString to use absl::variant. See envoyproxy#9593

4ded70e

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

asraa reviewed Feb 7, 2020

View reviewed changes

source/common/http/header_map_impl.cc Outdated Show resolved Hide resolved

Rewriting HeaderString to use absl::variant, and absl::InlinedVector.…

94c587a

… See envoyproxy#9593 Signed-off-by: Adi Suissa-Peleg <adip@google.com>

mattklein123 requested changes Feb 7, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 7, 2020

Merge branch 'header_map_simplify' of github.com:adisuissa/envoy into…

6a600d8

… header_map_simplify Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Feb 10, 2020

adisuissa added 2 commits February 11, 2020 11:05

Rewriting HeaderString to use absl::variant, and absl::InlinedVector.…

0b73c42

… See envoyproxy#9593 Cleaning up unneeded members, hiding Type enum, removing type() and data() methods, and introducing the inlineTransform method. Signed-off-by: Adi Suissa-Peleg <adip@google.com>

Rewriting HeaderString to use absl::variant, and absl::InlinedVector.…

6639fa0

… See envoyproxy#9593 Cleaning up unneeded members, hiding Type enum, removing type() and data() methods, and introducing the inlineTransform method. Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the waiting label Feb 12, 2020

Removing an unneeded code comment

b315f28

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Feb 12, 2020

Merging upstream/master

b848392

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot added the waiting label Feb 13, 2020

mattklein123 removed the waiting label Feb 13, 2020

mattklein123 requested changes Feb 13, 2020

View reviewed changes

mattklein123 added the waiting label Feb 13, 2020

asraa reviewed Feb 13, 2020

View reviewed changes

include/envoy/http/header_map.h Show resolved Hide resolved

adisuissa added 2 commits February 14, 2020 10:59

HeaderString rewrite - code cleanup, and using absl::ascii_tolower in…

aa29021

…stead of tolower Signed-off-by: Adi Suissa-Peleg <adip@google.com>

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

e73e1b9

Signed-off-by: Adi Suissa-Peleg <adip@google.com>

repokitteh-read-only bot removed the waiting label Feb 14, 2020

mattklein123 approved these changes Feb 17, 2020

View reviewed changes

mattklein123 merged commit b71eae0 into envoyproxy:master Feb 17, 2020

junr03 mentioned this pull request Feb 18, 2020

envoy: update ref 021820 envoyproxy/envoy-mobile#690

Merged

curiouserrandy mentioned this pull request Feb 24, 2020

References to @com_google_absl should go through bazel_respositories.bzl #10147

Closed

curiouserrandy mentioned this pull request Feb 25, 2020

include/envoy/http::header_map_interface has an implicit dependency on source/common/http:header_map_lib #10166

Closed

		@@ -89,12 +90,12 @@ using LowerCaseStrPairVector =
		* This is a string implementation for use in header processing. It is heavily optimized for
		* performance. It supports 3 different types of storage and can switch between them:

	static void insertHeader(std::vector<nghttp2_nv>& headers, const HeaderEntry& header) {
	uint8_t flags = 0;
	if (header.key().type() == HeaderString::Type::Reference) {
	flags \|= NGHTTP2_NV_FLAG_NO_COPY_NAME;
	}
	if (header.value().type() == HeaderString::Type::Reference) {
	flags \|= NGHTTP2_NV_FLAG_NO_COPY_VALUE;
	}

		#define BUFFER_STR_VIEW_GET absl::get<absl::string_view>(buffer_)
		#define BUFFER_IN_VEC_GET absl::get<absl::InlinedVector<char, 128>>(buffer_)

		// Initialize the size to the max length, copy the actual data, and then
		// reduce the size (but not the capacity) as needed

Conversation

adisuissa commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adisuissa commented Feb 6, 2020

Uh oh!

mattklein123 commented Feb 7, 2020

Uh oh!

adisuissa commented Feb 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa Feb 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa commented Feb 11, 2020

Uh oh!

mattklein123 commented Feb 12, 2020

Uh oh!

mattklein123 commented Feb 13, 2020

Uh oh!

adisuissa commented Feb 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Feb 13, 2020

Uh oh!

mattklein123 left a comment

adisuissa commented Feb 6, 2020 •

edited

Loading

adisuissa Feb 7, 2020 •

edited

Loading

adisuissa Feb 11, 2020 •

edited

Loading

adisuissa commented Feb 13, 2020 •

edited

Loading