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

This patch makes consistent the type for ssize_t as used on Windows. We will favor describing it as a ptrdiff_t, which is standard C. Patches to tclap and nghttp2 are needed for this exercise. Linux is a 64ILP architecture, so there's no difference between int and [s]size_t and pointer widths. Windows is a 64P architecture, where only long long maps to the width of the size_t and C99 standard ptrdiff_t types. This leads to a number of places where the Envoy API has presented an explicit int or int32_t which is smaller than the ptrdiff_t on Windows. In some places we must cast to or from the API defined width. Several other misassumptions on the width of int are also addressed. Windows does not support alternately-named VA_ARGS which is not a standard C++ language feature in our baseline c++ 17 expectation. Other minor adjustments reflect other quirks of the MSVC compilier (some more correct and some simply buggy behavior). Windows MSVC needs various guards on unrecognized #pragmas, and will not support any preprocessor operations within the arguments to a macro; this is not stdc or stdc++. This patch largely ignores changes which are required for windows, and do not impact the linux/os-x compilation path. This patch is also missing the IoHandle abstraction of 'fd' arguments, storage and return codes, win32 error handling and win32 specific #include's handling. Risk Level: Moderate Testing: Passed locally on Windows (with additional patches) and Linux Signed-off-by: William A Rowe Jr <[email protected]> Signed-off-by: Yechiel Kalmenson <[email protected]>
envoyproxy · Nov 4, 2019 · 85e2038 · 85e2038
1 parent 9dd7af9
commit 85e2038
Show file tree

Hide file tree

Showing 99 changed files with 270 additions and 253 deletions.
diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl
@@ -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({

diff --git a/bazel/foreign_cc/nghttp2.patch b/bazel/foreign_cc/nghttp2.patch
@@ -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)
 +  else()
 +    set(ssize_t int)
 +  endif()

diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl
@@ -237,6 +237,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",
@@ -334,6 +340,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
     )

diff --git a/bazel/tclap-win64-ull-sizet.patch b/bazel/tclap-win64-ull-sizet.patch
@@ -0,0 +1,16 @@
+diff --git a/include/tclap/StandardTraits.h b/include/tclap/StandardTraits.h
+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.
+  */
diff --git a/include/envoy/api/os_sys_calls.h b/include/envoy/api/os_sys_calls.h
@@ -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 {

diff --git a/include/envoy/api/os_sys_calls_common.h b/include/envoy/api/os_sys_calls_common.h
@@ -3,6 +3,8 @@
 #include <memory>
 #include <string>
 
+#include "envoy/common/platform.h"
+
 namespace Envoy {
 namespace Api {
 /**

diff --git a/include/envoy/buffer/buffer.h b/include/envoy/buffer/buffer.h
@@ -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"
 

diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h
@@ -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
diff --git a/include/envoy/network/address.h b/include/envoy/network/address.h
@@ -1,6 +1,5 @@
 #pragma once
 
-#include <sys/socket.h>
 #include <sys/types.h>
 
 #include <array>
@@ -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"
 

diff --git a/include/envoy/network/io_handle.h b/include/envoy/network/io_handle.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include "envoy/api/io_error.h"
+#include "envoy/common/platform.h"
 #include "envoy/common/pure.h"
 
 namespace Envoy {

diff --git a/source/common/common/byte_order.h b/source/common/common/byte_order.h
@@ -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)

diff --git a/source/common/common/fmt.h b/source/common/common/fmt.h
@@ -1,5 +1,7 @@
 #pragma once
 
+#include "envoy/common/platform.h" // Avert format.h including windows.h
+
 #include "absl/strings/string_view.h"
 #include "fmt/format.h"
 #include "fmt/ostream.h"

diff --git a/source/common/common/perf_annotation.cc b/source/common/common/perf_annotation.cc
@@ -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"
 

diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc
@@ -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 =
@@ -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) {

diff --git a/source/common/common/version.cc b/source/common/common/version.cc
@@ -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[];
 
@@ -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",
+  static const absl::string_view release_type = "RELEASE";
 #else
-                                                  "DEBUG",
+  static const absl::string_view release_type = "DEBUG";
 #endif
 #ifdef ENVOY_SSL_VERSION
-                                                  ENVOY_SSL_VERSION
+  static const absl::string_view ssl_version = ENVOY_SSL_VERSION;
 #else
-                                                  "no-ssl"
+  static 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
diff --git a/source/common/common/win32/thread_impl.h b/source/common/common/win32/thread_impl.h
@@ -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 {

diff --git a/source/common/filesystem/win32/directory_iterator_impl.h b/source/common/filesystem/win32/directory_iterator_impl.h
@@ -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 {

diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc
@@ -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>
@@ -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 {

diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc
@@ -1,7 +1,5 @@
 #include "common/grpc/common.h"
 
-#include <arpa/inet.h>
-
 #include <atomic>
 #include <cstdint>
 #include <cstring>

diff --git a/source/common/grpc/common.h b/source/common/grpc/common.h
@@ -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"

diff --git a/source/common/grpc/google_async_client_impl.h b/source/common/grpc/google_async_client_impl.h
@@ -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"

diff --git a/source/common/grpc/google_grpc_creds_impl.h b/source/common/grpc/google_grpc_creds_impl.h
@@ -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"
 

diff --git a/source/common/grpc/google_grpc_utils.h b/source/common/grpc/google_grpc_utils.h
@@ -4,6 +4,7 @@
 #include <string>
 
 #include "envoy/buffer/buffer.h"
+#include "envoy/common/platform.h"
 
 #include "grpcpp/grpcpp.h"