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

Few small fixes to allow envoy to compile on ARM32 #1781

Closed
wants to merge 31 commits into from

Conversation

costinm
Copy link
Contributor

@costinm costinm commented Sep 29, 2017

Tested compilation for rasbperry pi and android (the 'endian' change is
required for android NDK - recent androids can compile with 64 bit).

Signed-off-by: Costin Manolache [email protected]

@costinm
Copy link
Contributor Author

costinm commented Sep 29, 2017

Note that I compiled/tested using a custom cmake, not bazel.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks alright to me.

@@ -24,9 +24,9 @@ OptionsImpl::OptionsImpl(int argc, char** argv, const std::string& hot_restart_v
log_levels_string += "\n[trace] and [debug] are only available on debug builds";

TCLAP::CmdLine cmd("envoy", ' ', VersionInfo::version());
TCLAP::ValueArg<uint64_t> base_id(
TCLAP::ValueArg<uint32_t> base_id(
Copy link
Member

Choose a reason for hiding this comment

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

Why are these necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get this error with the pi cross-compiler. Not sure if there is some other fix, not an expert in c++...

In file included from /ws/istio-master/src/tclap/include/tclap/Arg.h:54:0,
from /ws/istio-master/src/tclap/include/tclap/SwitchArg.h:30,
from /ws/istio-master/src/tclap/include/tclap/CmdLine.h:27,
from /ws/istio-master/envoy/source/server/options_impl.cc:13:
/ws/istio-master/src/tclap/include/tclap/ArgTraits.h: In instantiation of 'struct TCLAP::ArgTraits':
/ws/istio-master/src/tclap/include/tclap/ValueArg.h:403:66: required from 'void TCLAP::ValueArg::_extractValue(const string&) [with T = long long unsigned int; std::string = std::basic_string]'
/ws/istio-master/src/tclap/include/tclap/ValueArg.h:363:29: required from 'bool TCLAP::ValueArg::processArg(int*, std::vector<std::basic_string >&) [with T = long long unsigned int]'
/ws/istio-master/envoy/source/server/options_impl.cc:120:1: required from here
/ws/istio-master/src/tclap/include/tclap/ArgTraits.h:80:39: error: 'long long unsigned int' is not a class, struct, or union type
typedef typename T::ValueCategory ValueCategory;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also - I don't think any of the options would need 64 bit parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's with arm-rpi-4.9.3-linux-gnueabihf

Copy link
Member

Choose a reason for hiding this comment

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

Might be an issue in converting from the literal to the type. I think in general, for clean 32/64-bit support, we should have some macros for writing fixed width literals, e.g. CONST64(0), since we're going to hit this elswhere. I do agree that these are fine as uint32_t though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems specific to tclap macros - so far everything else seems to work on 32 bit arm, I'll do more testing and try to get it into a continuous build.

@mattklein123
Copy link
Member

In general this seems fine to me but please fix DCO and format.

@@ -197,7 +197,7 @@ struct Http2Settings {
// initial value from HTTP/2 spec, same as NGHTTP2_DEFAULT_HEADER_TABLE_SIZE from nghttp2
static const uint32_t DEFAULT_HPACK_TABLE_SIZE = (1 << 12);
// no maximum from HTTP/2 spec, use unsigned 32-bit maximum
static const uint32_t MAX_HPACK_TABLE_SIZE = (1UL << 32) - 1;
static const uint32_t MAX_HPACK_TABLE_SIZE = 0xFFFFFFFF;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use the C++ numeric_limits for this:

#include <limits>
static const uint32_t MAX_HPACK_TABLE_SIZE = std::numeric_limits<uint32_t>::max();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

mattklein123 and others added 26 commits October 2, 2017 15:11
)

The v2 API supports using the downstream (source) IP address to route HTTP requests to
upstream hosts. This PR implements that functionality, as specified in envoyproxy#1296.

Signed-off-by: Alex Konradi <[email protected]>
There is no minimal language and due to travis-ci/travis-ci#4895, it will fallback to ruby. shell should serve our usecase

Signed-off-by: bndw <[email protected]>
Tested compilation for rasbperry pi and android (the 'endian' change is
required for android NDK - recent androids can compile with 64 bit).
Signed-off-by: Costin Manolache <[email protected]>
This PR adds per-host counters to support UpstreamLocalityStats. At load report time, these will be
aggregated on a per-locality basis.

Signed-off-by: Harvey Tuch [email protected]
…yproxy#1786)

This is helpful for Google import and shouldn't affect anybody else.

Signed-off-by: Piotr Sikora <[email protected]>
tdmackey and others added 4 commits October 2, 2017 15:11
See ci/README.md for instructions.

Signed-off-by: David Mackey [email protected]
This makes it appopriate to use the OsSysCalls abstraction
in other areas of the code-base, such as hot-restart.

Signed-off-by: Greg Greenway <[email protected]>
Tested compilation for rasbperry pi and android (the 'endian' change is
required for android NDK - recent androids can compile with 64 bit).

Signed-off-by: Costin Manolache <[email protected]>
@costinm
Copy link
Contributor Author

costinm commented Oct 2, 2017

Git mixups attempting to edit commits to add DCO - will upload a clean PR

@costinm costinm closed this Oct 2, 2017
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: Move tsan workflow to Engflow's remote execution

Same as envoyproxy/envoy-mobile#1745 but using a couple of tsan flags instead of asan. Using a separate config for modularity.

Risk Level: Low
Testing: See tsan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: Move tsan workflow to Engflow's remote execution

Same as envoyproxy/envoy-mobile#1745 but using a couple of tsan flags instead of asan. Using a separate config for modularity.

Risk Level: Low
Testing: See tsan workflow
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Luis Fernando Pino Duque <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.