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.

Sign up for GitHub

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

Jump to bottom

Correct stdc++ language and size issues for MSVC #8572

Merged
merged 15 commits into from
Nov 4, 2019
Merged

Correct stdc++ language and size issues for MSVC #8572

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d5ba81c
Correct stdc++ language and size issues for MSVC
wrowe Oct 10, 2019
5b5312d
Merge branch 'master' into fix-msvc-incompatibilties
Oct 23, 2019
7ea3522
This cleanup was misplaced; relocating to platform.h
wrowe Oct 23, 2019
3505e99
Note open ticket for format exception
achasveachas Oct 23, 2019
8172478
Ensure one-time variables per lizan's review
wrowe Oct 23, 2019
d62c39b
Merge remote-tracking branch 'origin/master' into fix-msvc-incompatib…
Oct 28, 2019
01da2c7
Merge branch 'fix-msvc-incompatibilties' of github.com:greenhouse-org…
Oct 28, 2019
55a7360
Refactor platform-specific includes into platform.h
achasveachas Oct 29, 2019
bbf2f05
Use '!' expression syntax, rather than 'not'
wrowe Oct 29, 2019
586fbbf
Small corrections for check_format.py
achasveachas Oct 29, 2019
e5fb28d
Add patch comments, set aside specific patch for later per htuch
wrowe Oct 30, 2019
5e98c83
Minor corrections identified by htuch
achasveachas Nov 1, 2019
ac0ee9e
Withdraw patch for a later fix using absl::Time
wrowe Nov 1, 2019
94070c5
Pedantic spelling fix
achasveachas Nov 1, 2019
74cee40
static const constants per lizan
wrowe Nov 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions bazel/envoy_internal.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,22 @@ def envoy_copts(repository, test = False):
"-std=c++14",
]

# Windows options for cleanest service compilation;
# General MSVC C++ options
# Streamline windows.h behavior for Win8+ API (for ntohll, see;
# https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx )
# Minimize Win32 API, dropping GUI-oriented features
msvc_options = [
"-WX",
"-Zc:__cplusplus",
"-std:c++14",
"-DWIN32",
"-DWIN32_LEAN_AND_MEAN",
# need win8 for ntohll
# https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745(v=vs.85).aspx
"-D_WIN32_WINNT=0x0602",
"-DNTDDI_VERSION=0x06020000",
"-DWIN32_LEAN_AND_MEAN",
"-DNOUSER",
"-DNOMCX",
"-DNOIME",
]

return select({
Expand Down
2 changes: 1 addition & 1 deletion bazel/foreign_cc/nghttp2.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ index 35c77d1d..47bd63f5 100644
# Set it to "int" to match the behavior of AC_TYPE_SSIZE_T (autotools).
- set(ssize_t int)
+ if(WIN32 AND CMAKE_SIZEOF_VOID_P EQUAL 8)
+ set(ssize_t int64_t)
+ set(ssize_t ptrdiff_t)
Copy link
Member

Choose a reason for hiding this comment

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

Can you upstream this? Patches aren't sustainable for us long term.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will not be accepted upstream until a version major bump (note it is our patch - of envoy's present patch.) PR and discussion item here; nghttp2/nghttp2#1395

Copy link
Member

Choose a reason for hiding this comment

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

Ack, please leave a reference to this thread in the patches arg of the _repository_impl .

Copy link
Contributor

Choose a reason for hiding this comment

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

Added footnote of ABI compat/next major release, discussion pointer to nghttp2/nghttp2#1395

+ else()
+ set(ssize_t int)
+ endif()
Expand Down
10 changes: 10 additions & 0 deletions bazel/repositories.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,12 @@ def _com_github_eile_tclap():
_repository_impl(
name = "com_github_eile_tclap",
build_file = "@envoy//bazel/external:tclap.BUILD",
patch_args = ["-p1"],
# If and when we pick up tclap 1.4 or later release,
# this entire issue was refactored away 6 years ago;
# https://sourceforge.net/p/tclap/code/ci/5d4ffbf2db794af799b8c5727fb6c65c079195ac/
# https://github.com/envoyproxy/envoy/pull/8572#discussion_r337554195
patches = ["@envoy//bazel:tclap-win64-ull-sizet.patch"],
)
native.bind(
name = "tclap",
Expand Down Expand Up @@ -344,6 +350,10 @@ def _com_github_nghttp2_nghttp2():
name = "com_github_nghttp2_nghttp2",
build_file_content = BUILD_ALL_CONTENT,
patch_args = ["-p1"],
# This patch cannot be picked up due to ABI rules. Better
# solve is likely at the next version-major. Discussion at;
# https://github.com/nghttp2/nghttp2/pull/1395
# https://github.com/envoyproxy/envoy/pull/8572#discussion_r334067786
patches = ["@envoy//bazel/foreign_cc:nghttp2.patch"],
**location
)
Expand Down
16 changes: 16 additions & 0 deletions bazel/tclap-win64-ull-sizet.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
diff --git a/include/tclap/StandardTraits.h b/include/tclap/StandardTraits.h
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix this one upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

We will submit it, but it is needed locally short term.

Copy link
Member

Choose a reason for hiding this comment

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

It seems small enough that we could kick this into the pipeline in parallel. We really dislike patches, they are only a solution when all else fails when working with an upstream project.

Copy link
Contributor

Choose a reason for hiding this comment

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

If and when we ever see a tclap 1.4 release, this patch and the defect go away, since the entire issue was refactored away 6 years ago;
https://sourceforge.net/p/tclap/code/ci/5d4ffbf2db794af799b8c5727fb6c65c079195ac/
Please consider this patch acceptable short-term (or permanently given the stable/no-releases state) for this component and we'll treat this as resolved for modern windows 64 compilers.

Copy link
Member

Choose a reason for hiding this comment

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

Ack, please leave a reference to this thread in the patches arg of the _repository_impl .

Copy link
Contributor

Choose a reason for hiding this comment

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

Added short note + refactor link

index 46d7f6f..117057b 100644
--- a/include/tclap/StandardTraits.h
+++ b/include/tclap/StandardTraits.h
@@ -123,8 +123,9 @@ struct ArgTraits<unsigned char> {
typedef ValueLike ValueCategory;
};

-// Microsoft implements size_t awkwardly.
-#if defined(_MSC_VER) && defined(_M_X64)
+// Microsoft implements size_t awkwardly.
+// Studio 2005 introduces unsigned long long, which conflicts with the size_t template
+#if defined(_MSC_VER) && (_MSC_VER < 1400) && defined(_M_X64)
/**
* size_ts have value-like semantics.
*/
9 changes: 1 addition & 8 deletions include/envoy/api/os_sys_calls.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
#pragma once

#ifndef WIN32
#include <sys/ioctl.h>
#include <sys/mman.h> // for mode_t
#include <sys/socket.h> // for sockaddr
#include <sys/stat.h>
#include <sys/uio.h> // for iovec

#endif

#include <memory>
#include <string>

#include "envoy/api/os_sys_calls_common.h"
#include "envoy/common/pure.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"

namespace Envoy {
namespace Api {
Expand Down
2 changes: 2 additions & 0 deletions include/envoy/api/os_sys_calls_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <memory>
#include <string>

#include "envoy/common/platform.h"

namespace Envoy {
namespace Api {
/**
Expand Down
1 change: 1 addition & 0 deletions include/envoy/buffer/buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "envoy/api/os_sys_calls.h"
#include "envoy/common/exception.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"
#include "envoy/network/io_handle.h"

Expand Down
56 changes: 50 additions & 6 deletions include/envoy/common/platform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,64 @@
#pragma once

// NOLINT(namespace-envoy)

// This common "platform.h" header exists to simplify the most common references
// to non-ANSI C/C++ headers, required on Windows, Posix, Linux, BSD etc,
// and to provide substitute definitions when absolutely required.
//
// The goal is to eventually not require this file of envoy header declarations,
// but limit the use of these architecture-specific types and declarations
// to the corresponding .cc implementation files.

#ifdef _MSC_VER

#include <windows.h>
#include <winsock2.h>

// These must follow afterwards
#include <mswsock.h>
#include <ws2tcpip.h>

// <windows.h> defines some frequently used symbols, so we need to undef these interfering symbols.
#undef DELETE
#undef ERROR
#undef GetMessage
#undef interface
#undef TRUE

#include <io.h>
#include <stdint.h>

#define PACKED_STRUCT(definition, ...) \
__pragma(pack(push, 1)) definition, ##__VA_ARGS__; \
__pragma(pack(pop))

#ifdef _M_X64
using ssize_t = int64_t;
#else
#error Envoy is not supported on 32-bit Windows
using ssize_t = ptrdiff_t;

typedef unsigned int sa_family_t;

#else // POSIX

#include <arpa/inet.h>
#include <ifaddrs.h>
#include <netdb.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <sys/ioctl.h>
#include <sys/mman.h> // for mode_t
#include <sys/socket.h>
#include <sys/uio.h> // for iovec
#include <sys/un.h>
#include <unistd.h>

#if defined(__linux__)
#include <linux/netfilter_ipv4.h>
#endif

#else
#define PACKED_STRUCT(definition, ...) definition, ##__VA_ARGS__ __attribute__((packed))

#ifndef IP6T_SO_ORIGINAL_DST
// From linux/netfilter_ipv6/ip6_tables.h
#define IP6T_SO_ORIGINAL_DST 80
#endif

#endif
2 changes: 1 addition & 1 deletion include/envoy/network/address.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#pragma once

#include <sys/socket.h>
#include <sys/types.h>

#include <array>
Expand All @@ -9,6 +8,7 @@
#include <string>

#include "envoy/api/os_sys_calls.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"
#include "envoy/network/io_handle.h"

Expand Down
1 change: 1 addition & 0 deletions include/envoy/network/io_handle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include "envoy/api/io_error.h"
#include "envoy/common/platform.h"
#include "envoy/common/pure.h"

namespace Envoy {
Expand Down
5 changes: 1 addition & 4 deletions source/common/common/byte_order.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,7 @@

#elif WIN32

#include <WinSock2.h>
// <winsock2.h> includes <windows.h>, so undef some interfering symbols
#undef DELETE
#undef GetMessage
#include "envoy/common/platform.h"

#define htole16(x) (x)
#define htole32(x) (x)
Expand Down
2 changes: 2 additions & 0 deletions source/common/common/fmt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#pragma once

#include "envoy/common/platform.h" // Avert format.h including windows.h
Copy link
Member

Choose a reason for hiding this comment

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

Can we fix fmtlib upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a practical matter? Any of our 3 dozen dependencies could introduce an #include <window.h> and disturb our source compilation, that header is not meant for general C++ language consumption. By this, we mean that MS heavily leverages #define and C++ heavily leverages multiple legible tokens. The logic in C language might introduce add_interface() but in C++ we are going to introduce interface.add(), which collides with windows.h defines.

We've introduced platform.h to work around this every time we encounter the issue. And if it is fixed upstream, then we could drop our #include, but it isn't critical. The answer at envoy is to eventually stop using Posix or Windows types in our own .h interface definitions, and localize the Posix and Windows implementations to our .cc sources (which will always include platform.h.) Exceptions would include our .h interface stubs to third party libraries such as grpc, nghttp2, etc.

I don't believe an upstream fix is really a priority, since I'm reading on #envoy-dev that fmtlib is being phased out for absl? It certainly shouldn't block this patch.

Copy link
Member

Choose a reason for hiding this comment

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

I feel that any 1st class support for Windows in Envoy doesn't end at Envoy. With my maintainer hat on I'm labeling this technical debt.. It's just a single line, but one day someone will come along and spend a day or so chasing down some obscure issue that is masked by this.

At the same time, we're pragmatic about tech debt and want to see Windows support land.

I would like to see tracking issues filed with the respective communities or in Envoy GH and referenced in these comments. Even if you folks aren't going to own this, we need to recognize these problems and provide the opportunity for other folks to do so.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we will live with fmtlib for a long time. It's deprecated on the data path, but it's a key part of logging which won't be going away.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we will live with fmtlib for a long time. It's deprecated on the data path, but it's a key part of logging which won't be going away.

Fair point, and researching, it turns out they have already responded.

fmtlib/fmt#171

We have the option of disabling fmt.h's use of windows.h, we just need to define FMT_USE_WINDOWS_H=0 as appropriate. This simply deprives us of the optimizations present in bazel-envoy/external/com_github_fmtlib_fmt/include/fmt/format-inl.h

It's an open question whether we want to define this symbol prior to including format.h in our
source/common/common/fmt.h header or include windows.h before they can, these have the same effect. If we want to explore this further, perhaps an issue is in order for further discussions, but we are reasonably happy with the proposed solution.


#include "absl/strings/string_view.h"
#include "fmt/format.h"
#include "fmt/ostream.h"
Expand Down
4 changes: 2 additions & 2 deletions source/common/common/perf_annotation.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@

#include "common/common/perf_annotation.h"

#include <unistd.h>

#include <chrono>
#include <iostream>
#include <string>

#include "envoy/common/platform.h"

#include "common/common/lock_guard.h"
#include "common/common/utility.h"

Expand Down
4 changes: 2 additions & 2 deletions source/common/common/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ DateFormatter::fromTimeAndPrepareSpecifierOffsets(time_t time, SpecifierOffsets&
const std::string& seconds_str) const {
std::string formatted_time;

size_t previous = 0;
int32_t previous = 0;
specifier_offsets.reserve(specifiers_.size());
for (const auto& specifier : specifiers_) {
std::string current_format =
Expand Down Expand Up @@ -353,7 +353,7 @@ uint32_t StringUtil::itoa(char* out, size_t buffer_size, uint64_t i) {
}

*current = 0;
return current - out;
return static_cast<uint32_t>(current - out);
}

size_t StringUtil::strlcpy(char* dst, const char* src, size_t size) {
Expand Down
16 changes: 9 additions & 7 deletions source/common/common/version.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include "common/common/macros.h"
#include "common/common/version_linkstamp.h"

#include "absl/strings/string_view.h"

extern const char build_scm_revision[];
extern const char build_scm_status[];

Expand All @@ -19,18 +21,18 @@ const std::string& VersionInfo::revisionStatus() {
}

const std::string& VersionInfo::version() {
CONSTRUCT_ON_FIRST_USE(std::string, fmt::format("{}/{}/{}/{}/{}", revision(),
BUILD_VERSION_NUMBER, revisionStatus(),
#ifdef NDEBUG
"RELEASE",
const absl::string_view release_type = "RELEASE";
htuch marked this conversation as resolved.
Show resolved Hide resolved
#else
"DEBUG",
const absl::string_view release_type = "DEBUG";
#endif
#ifdef ENVOY_SSL_VERSION
ENVOY_SSL_VERSION
const absl::string_view ssl_version = ENVOY_SSL_VERSION;
#else
"no-ssl"
const absl::string_view ssl_version = "no-ssl";
#endif
));
CONSTRUCT_ON_FIRST_USE(std::string,
fmt::format("{}/{}/{}/{}/{}", revision(), BUILD_VERSION_NUMBER,
revisionStatus(), release_type, ssl_version));
}
} // namespace Envoy
7 changes: 1 addition & 6 deletions source/common/common/win32/thread_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,8 @@
#pragma once

#include <windows.h>

// <windows.h> defines some macros that interfere with our code, so undef them
#undef DELETE
#undef GetMessage

#include <functional>

#include "envoy/common/platform.h"
#include "envoy/thread/thread.h"

namespace Envoy {
Expand Down
10 changes: 0 additions & 10 deletions source/common/filesystem/win32/directory_iterator_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,5 @@
#pragma once

#include <windows.h>

// <windows.h> uses macros to #define a ton of symbols, two of which (DELETE and GetMessage)
// interfere with our code. DELETE shows up in the base.pb.h header generated from
// api/envoy/api/core/base.proto. Since it's a generated header, we can't #undef DELETE at
// the top of that header to avoid the collision. Similarly, GetMessage shows up in generated
// protobuf code so we can't #undef the symbol there.
#undef DELETE
#undef GetMessage

#include "envoy/filesystem/filesystem.h"

namespace Envoy {
Expand Down
14 changes: 2 additions & 12 deletions source/common/filesystem/win32/filesystem_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,6 @@
#include <fcntl.h>
#include <io.h>
#include <sys/stat.h>
#include <windows.h>

// <windows.h> uses macros to #define a ton of symbols, two of which (DELETE and GetMessage)
// interfere with our code. DELETE shows up in the base.pb.h header generated from
// api/envoy/api/core/base.proto. Since it's a generated header, we can't #undef DELETE at
// the top of that header to avoid the collision. Similarly, GetMessage shows up in generated
// protobuf code so we can't #undef the symbol there.
#undef DELETE
#undef GetMessage

#include "common/common/assert.h"
#include "common/filesystem/filesystem_impl.h"

#include <fstream>
#include <iostream>
Expand All @@ -21,7 +9,9 @@

#include "envoy/common/exception.h"

#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/filesystem/filesystem_impl.h"

namespace Envoy {
namespace Filesystem {
Expand Down
2 changes: 0 additions & 2 deletions source/common/grpc/common.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include "common/grpc/common.h"

#include <arpa/inet.h>

#include <atomic>
#include <cstdint>
#include <cstring>
Expand Down
1 change: 1 addition & 0 deletions source/common/grpc/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <string>

#include "envoy/common/exception.h"
#include "envoy/common/platform.h"
#include "envoy/grpc/status.h"
#include "envoy/http/filter.h"
#include "envoy/http/header_map.h"
Expand Down
1 change: 1 addition & 0 deletions source/common/grpc/google_async_client_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <queue>

#include "envoy/api/api.h"
#include "envoy/common/platform.h"
#include "envoy/grpc/async_client.h"
#include "envoy/stats/scope.h"
#include "envoy/thread/thread.h"
Expand Down
1 change: 1 addition & 0 deletions source/common/grpc/google_grpc_creds_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "envoy/api/api.h"
#include "envoy/api/v2/core/grpc_service.pb.h"
#include "envoy/common/platform.h"

#include "grpcpp/grpcpp.h"

Expand Down
1 change: 1 addition & 0 deletions source/common/grpc/google_grpc_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <string>

#include "envoy/buffer/buffer.h"
#include "envoy/common/platform.h"

#include "grpcpp/grpcpp.h"

Expand Down
Loading