Isolate the include of Windows.h to the windows internals rather than bleeding out through error.h

This introduces a shim header and functions in the core/platform/windows folder
to remove this from the larger surface area of core/platform.

PiperOrigin-RevId: 293264226
Change-Id: I941492f5abf33827fbf52a4485fe7f0f3c7bb9d8

tensorflow/core/platform/error.cc

@@ -178,21 +178,4 @@ Status IOError(const string& context, int err_number) {
   return Status(code, strings::StrCat(context, "; ", strerror(err_number)));
 }
 
-#if defined(_WIN32)
-namespace internal {
-
-std::string GetWindowsErrorMessage(DWORD err) {
-  LPSTR buffer = NULL;
-  DWORD flags = FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM |
-                FORMAT_MESSAGE_IGNORE_INSERTS;
-  FormatMessageA(flags, NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
-                 reinterpret_cast<LPSTR>(&buffer), 0, NULL);
-  std::string message = buffer;
-  LocalFree(buffer);
-  return message;
-}
-
-}  // namespace internal
-#endif
-
 }  // namespace tensorflow

tensorflow/core/platform/error.h

@@ -18,7 +18,6 @@ limitations under the License.
 
 #include <string>
 
-#include "tensorflow/core/platform/platform.h"
 #include "tensorflow/core/platform/status.h"
 
 namespace tensorflow {
@@ -27,20 +26,4 @@ Status IOError(const string& context, int err_number);
 
 }  // namespace tensorflow
 
-#if defined(PLATFORM_WINDOWS)
-
-#include <Windows.h>
-// Windows.h #defines ERROR, but it is also used in
-// tensorflow/core/util/event.proto
-#undef ERROR
-
-namespace tensorflow {
-namespace internal {
-
-std::string GetWindowsErrorMessage(DWORD err);
-}
-}  // namespace tensorflow
-
-#endif
-
 #endif  // TENSORFLOW_CORE_PLATFORM_ERROR_H_

tensorflow/core/platform/windows/BUILD

@@ -1,7 +1,6 @@
 # Tensorflow windows-specific implementations of tensorflow/core/platform libraries.
 load(
     "//tensorflow:tensorflow.bzl",
-    "if_windows",
     "tf_copts",
 )
 load(
@@ -39,6 +38,7 @@ cc_library(
         "nobuilder",
     ],
     deps = [
+        ":error_windows",
         ":wide_char",
         "//tensorflow/core/lib/core:blocking_counter",
         "//tensorflow/core/lib/core:error_codes_proto_cc",
@@ -85,6 +85,21 @@ cc_library(
     deps = ["//tensorflow/core/platform:types"],
 )
 
+cc_library(
+    name = "error_windows",
+    srcs = ["error_windows.cc"],
+    hdrs = ["error_windows.h"],
+    linkopts = ["-DEFAULTLIB:ws2_32.lib"],
+    tags = [
+        "manual",
+        "no_oss",
+        "nobuilder",
+    ],
+    # This code is highly windows specific and should only be used with care
+    # from this package.
+    visibility = ["//visibility:private"],
+)
+
 cc_library(
     name = "intrinsics_port",
     srcs = ["intrinsics_port.h"],
@@ -116,13 +131,14 @@ cc_library(
     name = "net",
     srcs = ["net.cc"],
     hdrs = ["//tensorflow/core/platform:net.h"],
-    linkopts = if_windows(["-DEFAULTLIB:ws2_32.lib"]),
+    linkopts = ["-DEFAULTLIB:ws2_32.lib"],
     tags = [
         "manual",
         "no_oss",
         "nobuilder",
     ],
     deps = [
+        ":error_windows",
         "//tensorflow/core/platform:error",
         "//tensorflow/core/platform:logging",
     ],

tensorflow/core/platform/windows/error_windows.cc

@@ -0,0 +1,53 @@
+/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+==============================================================================*/
+
+#include "tensorflow/core/platform/windows/error_windows.h"
+
+#include <Windows.h>
+#include <Winsock2.h>
+
+#include <string>
+
+namespace tensorflow {
+namespace internal {
+namespace {
+
+std::string GetWindowsErrorMessage(DWORD err) {
+  LPSTR buffer = NULL;
+  DWORD flags = FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM |
+                FORMAT_MESSAGE_IGNORE_INSERTS;
+  DWORD length = FormatMessageA(flags, NULL, err,
+                                MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT),
+                                reinterpret_cast<LPSTR>(&buffer), 0, NULL);
+  std::string message(buffer, length);
+  LocalFree(buffer);
+  if (length == 0) {
+    message = "Failed to FormatMessage for error";
+  }
+  return message;
+}
+
+}  // namespace
+
+std::string WindowsGetLastErrorMessage() {
+  return GetWindowsErrorMessage(::GetLastError());
+}
+
+std::string WindowsWSAGetLastErrorMessage() {
+  return GetWindowsErrorMessage(::WSAGetLastError());
+}
+
+}  // namespace internal
+}  // namespace tensorflow

tensorflow/core/platform/windows/error_windows.h

@@ -0,0 +1,40 @@
+/* Copyright 2020 The TensorFlow Authors. All Rights Reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+==============================================================================*/
+
+#ifndef TENSORFLOW_CORE_PLATFORM_WINDOWS_ERROR_WINDOWS_H_
+#define TENSORFLOW_CORE_PLATFORM_WINDOWS_ERROR_WINDOWS_H_
+
+// This file is here to provide a windows specific interface to error functions
+// without needing to include any windows specific headers. This is intended to
+// reduce conflicts induced by code needing to run on multiple operating
+// systems.
+
+#include <string>
+
+namespace tensorflow {
+namespace internal {
+
+// WindowsGetLastErrorMessage calls GetLastError() and then formats the error
+// message for reporting.
+std::string WindowsGetLastErrorMessage();
+
+// WindowsWSLGetLastErrorMessage calls GetLastError() and then formats the error
+// message for reporting.
+std::string WindowsWSAGetLastErrorMessage();
+
+}  // namespace internal
+}  // namespace tensorflow
+
+#endif  // TENSORFLOW_CORE_PLATFORM_WINDOWS_ERROR_WINDOWS_H_

tensorflow/core/platform/windows/net.cc

@@ -23,6 +23,7 @@ limitations under the License.
 
 #include "tensorflow/core/platform/error.h"
 #include "tensorflow/core/platform/logging.h"
+#include "tensorflow/core/platform/windows/error_windows.h"
 
 #undef ERROR
 
@@ -42,8 +43,7 @@ bool IsPortAvailable(int* port, bool is_tcp) {
   CHECK_GE(*port, 0);
   CHECK_LE(*port, 65535);
   if (sock == INVALID_SOCKET) {
-    LOG(ERROR) << "socket() failed: "
-               << GetWindowsErrorMessage(WSAGetLastError());
+    LOG(ERROR) << "socket() failed: " << WindowsWSAGetLastErrorMessage();
     return false;
   }
 
@@ -52,8 +52,7 @@ bool IsPortAvailable(int* port, bool is_tcp) {
   int result = setsockopt(sock, SOL_SOCKET, SO_REUSEADDR,
                           reinterpret_cast<const char*>(&one), sizeof(one));
   if (result == SOCKET_ERROR) {
-    LOG(ERROR) << "setsockopt() failed: "
-               << GetWindowsErrorMessage(WSAGetLastError());
+    LOG(ERROR) << "setsockopt() failed: " << WindowsWSAGetLastErrorMessage();
     closesocket(sock);
     return false;
   }
@@ -65,7 +64,7 @@ bool IsPortAvailable(int* port, bool is_tcp) {
   result = bind(sock, (struct sockaddr*)&addr, sizeof(addr));
   if (result == SOCKET_ERROR) {
     LOG(WARNING) << "bind(port=" << *port
-                 << ") failed: " << GetWindowsErrorMessage(WSAGetLastError());
+                 << ") failed: " << WindowsWSAGetLastErrorMessage();
     closesocket(sock);
     return false;
   }
@@ -73,8 +72,7 @@ bool IsPortAvailable(int* port, bool is_tcp) {
   // Get the bound port number.
   result = getsockname(sock, (struct sockaddr*)&addr, &addr_len);
   if (result == SOCKET_ERROR) {
-    LOG(WARNING) << "getsockname() failed: "
-                 << GetWindowsErrorMessage(WSAGetLastError());
+    LOG(WARNING) << "getsockname() failed: " << WindowsWSAGetLastErrorMessage();
     closesocket(sock);
     return false;
   }

tensorflow/core/platform/windows/windows_file_system.cc

@@ -13,6 +13,8 @@ See the License for the specific language governing permissions and
 limitations under the License.
 ==============================================================================*/
 
+#include "tensorflow/core/platform/windows/windows_file_system.h"
+
 #include <Shlwapi.h>
 #include <Windows.h>
 #include <direct.h>
@@ -30,8 +32,8 @@ limitations under the License.
 #include "tensorflow/core/platform/file_system_helper.h"
 #include "tensorflow/core/platform/logging.h"
 #include "tensorflow/core/platform/strcat.h"
+#include "tensorflow/core/platform/windows/error_windows.h"
 #include "tensorflow/core/platform/windows/wide_char.h"
-#include "tensorflow/core/platform/windows/windows_file_system.h"
 #include "tensorflow/core/protobuf/error_codes.pb.h"
 
 // TODO(mrry): Prevent this Windows.h #define from leaking out of our headers.
@@ -45,9 +47,11 @@ namespace {
 const auto CloseHandleFunc = [](HANDLE h) { ::CloseHandle(h); };
 typedef std::unique_ptr<void, decltype(CloseHandleFunc)> UniqueCloseHandlePtr;
 
-inline Status IOErrorFromWindowsError(const string& context, DWORD err) {
+inline Status IOErrorFromWindowsError(const string& context) {
+  auto last_error = ::GetLastError();
   return IOError(
-      context + string(" : ") + internal::GetWindowsErrorMessage(err), err);
+      context + string(" : ") + internal::WindowsGetLastErrorMessage(),
+      last_error);
 }
 
 // PLEASE NOTE: hfile is expected to be an async handle
@@ -166,8 +170,7 @@ class WindowsWritableFile : public WritableFile {
     BOOL write_result =
         ::WriteFile(hfile_, data.data(), data_size, &bytes_written, NULL);
     if (FALSE == write_result) {
-      return IOErrorFromWindowsError("Failed to WriteFile: " + filename_,
-                                     ::GetLastError());
+      return IOErrorFromWindowsError("Failed to WriteFile: " + filename_);
     }
 
     assert(size_t(bytes_written) == data.size());
@@ -183,8 +186,8 @@ class WindowsWritableFile : public WritableFile {
     *position = SetFilePointer(hfile_, 0, NULL, FILE_CURRENT);
 
     if (*position == INVALID_SET_FILE_POINTER) {
-      return IOErrorFromWindowsError(
-          "Tell(SetFilePointer) failed for: " + filename_, ::GetLastError());
+      return IOErrorFromWindowsError("Tell(SetFilePointer) failed for: " +
+                                     filename_);
     }
 
     return Status::OK();
@@ -199,8 +202,7 @@ class WindowsWritableFile : public WritableFile {
     }
 
     if (FALSE == ::CloseHandle(hfile_)) {
-      return IOErrorFromWindowsError("CloseHandle failed for: " + filename_,
-                                     ::GetLastError());
+      return IOErrorFromWindowsError("CloseHandle failed for: " + filename_);
     }
 
     hfile_ = INVALID_HANDLE_VALUE;
@@ -209,8 +211,8 @@ class WindowsWritableFile : public WritableFile {
 
   Status Flush() override {
     if (FALSE == ::FlushFileBuffers(hfile_)) {
-      return IOErrorFromWindowsError(
-          "FlushFileBuffers failed for: " + filename_, ::GetLastError());
+      return IOErrorFromWindowsError("FlushFileBuffers failed for: " +
+                                     filename_);
     }
     return Status::OK();
   }
@@ -278,7 +280,7 @@ Status WindowsFileSystem::NewRandomAccessFile(
 
   if (INVALID_HANDLE_VALUE == hfile) {
     string context = "NewRandomAccessFile failed to Create/Open: " + fname;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   result->reset(new WindowsRandomAccessFile(translated_fname, hfile));
@@ -298,7 +300,7 @@ Status WindowsFileSystem::NewWritableFile(
 
   if (INVALID_HANDLE_VALUE == hfile) {
     string context = "Failed to create a NewWriteableFile: " + fname;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   result->reset(new WindowsWritableFile(translated_fname, hfile));
@@ -318,7 +320,7 @@ Status WindowsFileSystem::NewAppendableFile(
 
   if (INVALID_HANDLE_VALUE == hfile) {
     string context = "Failed to create a NewAppendableFile: " + fname;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   UniqueCloseHandlePtr file_guard(hfile, CloseHandleFunc);
@@ -326,7 +328,7 @@ Status WindowsFileSystem::NewAppendableFile(
   DWORD file_ptr = ::SetFilePointer(hfile, NULL, NULL, FILE_END);
   if (INVALID_SET_FILE_POINTER == file_ptr) {
     string context = "Failed to create a NewAppendableFile: " + fname;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   result->reset(new WindowsWritableFile(translated_fname, hfile));
@@ -356,8 +358,7 @@ Status WindowsFileSystem::NewReadOnlyMemoryRegionFromFile(
 
   if (INVALID_HANDLE_VALUE == hfile) {
     return IOErrorFromWindowsError(
-        "NewReadOnlyMemoryRegionFromFile failed to Create/Open: " + fname,
-        ::GetLastError());
+        "NewReadOnlyMemoryRegionFromFile failed to Create/Open: " + fname);
   }
 
   UniqueCloseHandlePtr file_guard(hfile, CloseHandleFunc);
@@ -383,7 +384,7 @@ Status WindowsFileSystem::NewReadOnlyMemoryRegionFromFile(
           "Failed to create file mapping for "
           "NewReadOnlyMemoryRegionFromFile: " +
           fname;
-      return IOErrorFromWindowsError(context, ::GetLastError());
+      return IOErrorFromWindowsError(context);
     }
 
     UniqueCloseHandlePtr map_guard(hmap, CloseHandleFunc);
@@ -400,7 +401,7 @@ Status WindowsFileSystem::NewReadOnlyMemoryRegionFromFile(
           "Failed to MapViewOfFile for "
           "NewReadOnlyMemoryRegionFromFile: " +
           fname;
-      return IOErrorFromWindowsError(context, ::GetLastError());
+      return IOErrorFromWindowsError(context);
     }
 
     result->reset(new WinReadOnlyMemoryRegion(fname, hfile, hmap, mapped_region,
@@ -439,7 +440,7 @@ Status WindowsFileSystem::GetChildren(const string& dir,
   HANDLE find_handle = ::FindFirstFileW(pattern.c_str(), &find_data);
   if (find_handle == INVALID_HANDLE_VALUE) {
     string context = "FindFirstFile failed for: " + translated_dir;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   do {
@@ -452,7 +453,7 @@ Status WindowsFileSystem::GetChildren(const string& dir,
 
   if (!::FindClose(find_handle)) {
     string context = "FindClose failed for: " + translated_dir;
-    return IOErrorFromWindowsError(context, ::GetLastError());
+    return IOErrorFromWindowsError(context);
   }
 
   return Status::OK();
@@ -501,7 +502,7 @@ Status WindowsFileSystem::GetFileSize(const string& fname, uint64* size) {
     *size = file_size.QuadPart;
   } else {
     string context = "Can not get size for: " + fname;
-    result = IOErrorFromWindowsError(context, ::GetLastError());
+    result = IOErrorFromWindowsError(context);
   }
   return result;
 }
@@ -524,7 +525,7 @@ Status WindowsFileSystem::RenameFile(const string& src, const string& target) {
   if (!::MoveFileExW(ws_translated_src.c_str(), ws_translated_target.c_str(),
                      MOVEFILE_REPLACE_EXISTING)) {
     string context(strings::StrCat("Failed to rename: ", src, " to: ", target));
-    result = IOErrorFromWindowsError(context, ::GetLastError());
+    result = IOErrorFromWindowsError(context);
   }
   return result;
 }