build: minor fix for 32-bit archs by goaway · Pull Request #11726 · envoyproxy/envoy

goaway · 2020-06-24T00:49:29Z

Commit Message: build: minor fix for 32-bit archs
Additional Description: Fixes an implicit cast error building for 32-bit Android here.
Risk Level: Low
Testing: Builds locally

Signed-off-by: Mike Schore [email protected]

Signed-off-by: Mike Schore <[email protected]>

wrowe · 2020-06-24T16:50:23Z

source/common/stats/symbol_table_impl.h

  absl::string_view dataAsStringView() const {
-    return {reinterpret_cast<const char*>(data()), dataSize()};
+    return {reinterpret_cast<const char*>(data()),
+            static_cast<absl::string_view::size_type>(dataSize())};


Isn't the underlying problem the abuse of uint64_t in line 409 above for dataSize()?

There's a significant penalty for using a 64 bit int in 32 bit contexts.

@jmarantz ?

I don't mind changing the underlying type of dataSize, but I don't really have any context on the original choice.

Personally I'd prefer size_t for this; IIUC Envoy style has veered toward using uint64_t. @mattklein123 may have more context.

I'm fine with either:

this change, which makes sense in the context of returning a string_view

changing dataSize() et al to return size_t, which makes sense generally IMO

I slightly prefer the latter but I'm OK with this. @mattklein123 what do you prefer?

Sure we can use size_t. Typically we have used explicit types for things that can wrap, but for something like this size_t seems fine.

/wait

uint64_t seems to be used extensively here to represent a number of bytes. dataSize() is also called in a number of locations. Just to clarify, is the proposal to switch size_t for all of these cases?

Yes size_t seems appropriate for number of bytes.

Despite updating the type used for representing byte counts throughout the file, I left this cast in; I don't know what sort of contract absl::string_view::size_type has with respect to size_t.

If folks feel it's redundant though now, I can remove it.

jmarantz

this is OK with me but I'd like @mattklein123 to be the final approver.

jmarantz · 2020-06-27T17:21:38Z

source/common/stats/symbol_table_impl.h

  absl::string_view dataAsStringView() const {
-    return {reinterpret_cast<const char*>(data()), dataSize()};
+    return {reinterpret_cast<const char*>(data()),
+            static_cast<absl::string_view::size_type>(dataSize())};


Personally I'd prefer size_t for this; IIUC Envoy style has veered toward using uint64_t. @mattklein123 may have more context.

I'm fine with either:

this change, which makes sense in the context of returning a string_view

changing dataSize() et al to return size_t, which makes sense generally IMO

I slightly prefer the latter but I'm OK with this. @mattklein123 what do you prefer?

Signed-off-by: Mike Schore <[email protected]>

goaway · 2020-06-30T03:10:13Z

source/common/stats/symbol_table_impl.cc

-  const uint64_t size = src.size();
-  MemBlockBuilder<uint8_t> storage(size);
+  const size_t size = src.size();
+  MemBlockBuilder<uint8_t> storage(size); // Note: MemBlockBuilder takes uint64_t.


In theory MemBlockBuilder could be updated to use size_t, too, and I suspect there may be other places we could switch over as well. My feeling is that that's a bit outside the scope of this change, however.

agreed; outside the scope for now.

Signed-off-by: Mike Schore <[email protected]>

jmarantz

basically looks good, modulo a naming issue you need to fix to pass CI. Also merge master.

jmarantz · 2020-07-02T21:39:18Z

source/common/stats/symbol_table_impl.cc

-  const uint64_t size = src.size();
-  MemBlockBuilder<uint8_t> storage(size);
+  const size_t size = src.size();
+  MemBlockBuilder<uint8_t> storage(size); // Note: MemBlockBuilder takes uint64_t.


agreed; outside the scope for now.

jmarantz · 2020-07-02T21:43:31Z

source/common/stats/symbol_table_impl.cc

 void SymbolTableImpl::Encoding::decodeTokens(
-    const SymbolTable::Storage array, uint64_t size,
-    const std::function<void(Symbol)>& symbolTokenFn,
+    const SymbolTable::Storage array, size_t size, const std::function<void(Symbol)>& symbolTokenFn,


not your fault, but clang-tidy is dinging you for a pre-existing naming mistake here.

s/symbolTokenFn/symbol_token_fn/ here and in the header. Sorry about that.

That will help you pass CI.

jmarantz · 2020-07-02T21:44:49Z

/wait

Signed-off-by: Mike Schore <[email protected]>

jmarantz

@envoyproxy/senior-maintainers

junr03 · 2020-07-08T21:13:43Z

@ggreenway do you mind doing a last pass here?

ggreenway

LGTM

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: envoyproxy/envoy#10030 - Fixes for 32 bit archs: envoyproxy/envoy#11726 - Fix for missing posix call on Android: envoyproxy/envoy#12081 - Additional zlib stats: envoyproxy/envoy#11782 Signed-off-by: Mike Schore <[email protected]>

Fixes an implicit cast error building for 32-bit Android here. Signed-off-by: Mike Schore <[email protected]> Signed-off-by: scheler <[email protected]>

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: #10030 - Fixes for 32 bit archs: #11726 - Fix for missing posix call on Android: #12081 - Additional zlib stats: #11782 Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>

build: minor fix for 32-bit archs

96bc163

Signed-off-by: Mike Schore <[email protected]>

asraa assigned junr03 Jun 24, 2020

wrowe reviewed Jun 24, 2020

View reviewed changes

asraa assigned jmarantz and unassigned junr03 Jun 26, 2020

jmarantz previously approved these changes Jun 27, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 28, 2020

overhaul SymbolTableImpl to use size_t for byte sizes

2bbb403

Signed-off-by: Mike Schore <[email protected]>

goaway dismissed jmarantz’s stale review via 2bbb403 June 30, 2020 03:01

repokitteh-read-only bot removed the waiting label Jun 30, 2020

goaway commented Jun 30, 2020

View reviewed changes

goaway added 2 commits July 1, 2020 06:05

fix format

df37ac4

Signed-off-by: Mike Schore <[email protected]>

another format fix

f06247c

Signed-off-by: Mike Schore <[email protected]>

jmarantz previously approved these changes Jul 2, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 2, 2020

fix a couple names

80497bc

Signed-off-by: Mike Schore <[email protected]>

goaway dismissed jmarantz’s stale review via 80497bc July 6, 2020 08:15

repokitteh-read-only bot removed the waiting label Jul 6, 2020

goaway added 4 commits July 6, 2020 16:15

Merge branch 'master' into ms/fix32

6398bce

another name fix

27b7027

Signed-off-by: Mike Schore <[email protected]>

format fix

6d56ecd

Signed-off-by: Mike Schore <[email protected]>

retry test

6dbb02f

Signed-off-by: Mike Schore <[email protected]>

jmarantz approved these changes Jul 7, 2020

View reviewed changes

ggreenway approved these changes Jul 9, 2020

View reviewed changes

ggreenway merged commit ec431a0 into envoyproxy:master Jul 9, 2020

rebello95 mentioned this pull request Jul 14, 2020

upstream: update Envoy upstream sha envoyproxy/envoy-mobile#951

Closed

goaway mentioned this pull request Jul 15, 2020

envoy: update upstream for multiple fixes envoyproxy/envoy-mobile#923

Merged

Conversation

goaway commented Jun 24, 2020

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!

jmarantz 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!

jmarantz 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!

jmarantz commented Jul 2, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jul 8, 2020

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants