From c709a7d8bba9373cb735658598e5bd6197473dbd Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Sun, 25 Aug 2024 23:43:25 -0400 Subject: [PATCH 1/4] [Build] fix file_data_loader.cpp build issues for windows Two build issues have been addressed in this pull request: 1. Avoid closing when fd_ is -1, as this would cause a crash on Windows. 2. Introduce separate headers for Windows and Unix, enabling Windows builds to utilize a distinct header and implement pread. For #4661 --- CMakeLists.txt | 5 ++ extension/data_loader/CMakeLists.txt | 5 ++ extension/data_loader/Unix/file.h | 11 +++++ extension/data_loader/Windows/file.h | 53 ++++++++++++++++++++++ extension/data_loader/file_data_loader.cpp | 8 ++-- 5 files changed, 79 insertions(+), 3 deletions(-) create mode 100644 extension/data_loader/Unix/file.h create mode 100644 extension/data_loader/Windows/file.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 99c8b7f69f..72464c282b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -785,6 +785,11 @@ if(EXECUTORCH_BUILD_EXECUTOR_RUNNER) endif() add_executable(executor_runner ${_executor_runner__srcs}) + if(WIN32) + target_include_directories(executor_runner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/extension/data_loader/Windows) + else() + target_include_directories(executor_runner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/extension/data_loader/Unix) + endif() if(CMAKE_BUILD_TYPE STREQUAL "Release") if(APPLE) target_link_options(executor_runner PRIVATE "LINKER:-dead_strip") diff --git a/extension/data_loader/CMakeLists.txt b/extension/data_loader/CMakeLists.txt index c9503e78e2..c978d98f61 100644 --- a/extension/data_loader/CMakeLists.txt +++ b/extension/data_loader/CMakeLists.txt @@ -20,6 +20,11 @@ list(TRANSFORM _extension_data_loader__srcs PREPEND "${EXECUTORCH_ROOT}/") add_library(extension_data_loader ${_extension_data_loader__srcs}) target_link_libraries(extension_data_loader executorch) target_include_directories(extension_data_loader PUBLIC ${EXECUTORCH_ROOT}/..) +if(WIN32) + target_include_directories(extension_data_loader PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/Windows) +else() + target_include_directories(extension_data_loader PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/Unix) +endif() target_compile_options(extension_data_loader PUBLIC ${_common_compile_options}) # Install libraries diff --git a/extension/data_loader/Unix/file.h b/extension/data_loader/Unix/file.h new file mode 100644 index 0000000000..270794748f --- /dev/null +++ b/extension/data_loader/Unix/file.h @@ -0,0 +1,11 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include diff --git a/extension/data_loader/Windows/file.h b/extension/data_loader/Windows/file.h new file mode 100644 index 0000000000..3a9699f1eb --- /dev/null +++ b/extension/data_loader/Windows/file.h @@ -0,0 +1,53 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include // For ssize_t. +#include + +#include + +inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) { + OVERLAPPED overlapped; /* The offset for ReadFile. */ + memset(&overlapped, 0, sizeof(overlapped)); + overlapped.Offset = __offset; + overlapped.OffsetHigh = __offset >> 32; + + BOOL result; /* The result of ReadFile. */ + DWORD bytes_read; /* The number of bytes read. */ + HANDLE file = (HANDLE)_get_osfhandle(__fd); + + result = ReadFile(file, __buf, __nbytes, &bytes_read, &overlapped); + DWORD error = GetLastError(); + if (!result) { + if (error == ERROR_IO_PENDING) { + result = GetOverlappedResult(file, &overlapped, &bytes_read, TRUE); + if (!result) { + error = GetLastError(); + } + } + } + if (!result) { + // Translate error into errno. + switch (error) { + case ERROR_HANDLE_EOF: + errno = 0; + break; + default: + errno = EIO; + break; + } + return -1; + } + return bytes_read; +} + +// To avoid conflicts with std::numeric_limits::max() in +// file_data_loader.cpp. +#undef max diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp index 1d097cfd98..cae27d944f 100644 --- a/extension/data_loader/file_data_loader.cpp +++ b/extension/data_loader/file_data_loader.cpp @@ -7,6 +7,7 @@ */ #include +#include #include #include @@ -17,7 +18,6 @@ #include #include #include -#include #include #include @@ -69,8 +69,10 @@ FileDataLoader::~FileDataLoader() { // file_name_ can be nullptr if this instance was moved from, but freeing a // null pointer is safe. std::free(const_cast(file_name_)); - // fd_ can be -1 if this instance was moved from, but closing a negative fd is - // safe (though it will return an error). + // fd_ can be -1 if this instance was moved from. + if (fd_ == -1) { + return; + } ::close(fd_); } From be77d20ad9c4bedcda35ccf1f1693bd83658f028 Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Tue, 27 Aug 2024 23:00:42 -0400 Subject: [PATCH 2/4] Use single file.h --- CMakeLists.txt | 5 ----- extension/data_loader/CMakeLists.txt | 5 ----- extension/data_loader/Unix/file.h | 11 ----------- extension/data_loader/{Windows => }/file.h | 4 ++++ extension/data_loader/file_data_loader.cpp | 2 +- 5 files changed, 5 insertions(+), 22 deletions(-) delete mode 100644 extension/data_loader/Unix/file.h rename extension/data_loader/{Windows => }/file.h (96%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 72464c282b..99c8b7f69f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -785,11 +785,6 @@ if(EXECUTORCH_BUILD_EXECUTOR_RUNNER) endif() add_executable(executor_runner ${_executor_runner__srcs}) - if(WIN32) - target_include_directories(executor_runner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/extension/data_loader/Windows) - else() - target_include_directories(executor_runner PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/extension/data_loader/Unix) - endif() if(CMAKE_BUILD_TYPE STREQUAL "Release") if(APPLE) target_link_options(executor_runner PRIVATE "LINKER:-dead_strip") diff --git a/extension/data_loader/CMakeLists.txt b/extension/data_loader/CMakeLists.txt index c978d98f61..c9503e78e2 100644 --- a/extension/data_loader/CMakeLists.txt +++ b/extension/data_loader/CMakeLists.txt @@ -20,11 +20,6 @@ list(TRANSFORM _extension_data_loader__srcs PREPEND "${EXECUTORCH_ROOT}/") add_library(extension_data_loader ${_extension_data_loader__srcs}) target_link_libraries(extension_data_loader executorch) target_include_directories(extension_data_loader PUBLIC ${EXECUTORCH_ROOT}/..) -if(WIN32) - target_include_directories(extension_data_loader PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/Windows) -else() - target_include_directories(extension_data_loader PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/Unix) -endif() target_compile_options(extension_data_loader PUBLIC ${_common_compile_options}) # Install libraries diff --git a/extension/data_loader/Unix/file.h b/extension/data_loader/Unix/file.h deleted file mode 100644 index 270794748f..0000000000 --- a/extension/data_loader/Unix/file.h +++ /dev/null @@ -1,11 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * All rights reserved. - * - * This source code is licensed under the BSD-style license found in the - * LICENSE file in the root directory of this source tree. - */ - -#pragma once - -#include diff --git a/extension/data_loader/Windows/file.h b/extension/data_loader/file.h similarity index 96% rename from extension/data_loader/Windows/file.h rename to extension/data_loader/file.h index 3a9699f1eb..1d380bea32 100644 --- a/extension/data_loader/Windows/file.h +++ b/extension/data_loader/file.h @@ -8,6 +8,9 @@ #pragma once +#ifndef _WIN32 +#include +#else #include // For ssize_t. #include @@ -51,3 +54,4 @@ inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) { // To avoid conflicts with std::numeric_limits::max() in // file_data_loader.cpp. #undef max +#endif diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp index cae27d944f..7cc40a99ab 100644 --- a/extension/data_loader/file_data_loader.cpp +++ b/extension/data_loader/file_data_loader.cpp @@ -6,8 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +#include #include -#include #include #include From 394f372ced9c718e37494689c822913b3f7cfe0f Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Wed, 28 Aug 2024 13:48:06 -0400 Subject: [PATCH 3/4] Add file.h to targets.bzl --- extension/data_loader/targets.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/extension/data_loader/targets.bzl b/extension/data_loader/targets.bzl index 4886df03a7..81269cfef5 100644 --- a/extension/data_loader/targets.bzl +++ b/extension/data_loader/targets.bzl @@ -40,6 +40,7 @@ def define_common_targets(): runtime.cxx_library( name = "file_data_loader", srcs = ["file_data_loader.cpp"], + headers = ["file.h"], exported_headers = ["file_data_loader.h"], visibility = [ "//executorch/test/...", From bc600ce9c3daaaae950d6edddbaea090956995cf Mon Sep 17 00:00:00 2001 From: Xiang Li Date: Thu, 29 Aug 2024 20:00:09 -0400 Subject: [PATCH 4/4] Update per comment. --- extension/data_loader/file_data_loader.cpp | 2 +- extension/data_loader/{file.h => pread.h} | 18 ++++++++++-------- extension/data_loader/targets.bzl | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) rename extension/data_loader/{file.h => pread.h} (75%) diff --git a/extension/data_loader/file_data_loader.cpp b/extension/data_loader/file_data_loader.cpp index 7cc40a99ab..ffc45ecc96 100644 --- a/extension/data_loader/file_data_loader.cpp +++ b/extension/data_loader/file_data_loader.cpp @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -#include +#include #include #include diff --git a/extension/data_loader/file.h b/extension/data_loader/pread.h similarity index 75% rename from extension/data_loader/file.h rename to extension/data_loader/pread.h index 1d380bea32..765397f3e8 100644 --- a/extension/data_loader/file.h +++ b/extension/data_loader/pread.h @@ -6,6 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +// This file ensures that a pread-compatible function is defined in the global namespace for windows and posix environments. + #pragma once #ifndef _WIN32 @@ -15,18 +17,21 @@ #include #include +// To avoid conflicts with std::numeric_limits::max() in +// file_data_loader.cpp. +#undef max -inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) { +inline ssize_t pread(int fd, void* buf, size_t nbytes, size_t offset) { OVERLAPPED overlapped; /* The offset for ReadFile. */ memset(&overlapped, 0, sizeof(overlapped)); - overlapped.Offset = __offset; - overlapped.OffsetHigh = __offset >> 32; + overlapped.Offset = offset; + overlapped.OffsetHigh = offset >> 32; BOOL result; /* The result of ReadFile. */ DWORD bytes_read; /* The number of bytes read. */ - HANDLE file = (HANDLE)_get_osfhandle(__fd); + HANDLE file = (HANDLE)_get_osfhandle(fd); - result = ReadFile(file, __buf, __nbytes, &bytes_read, &overlapped); + result = ReadFile(file, buf, nbytes, &bytes_read, &overlapped); DWORD error = GetLastError(); if (!result) { if (error == ERROR_IO_PENDING) { @@ -51,7 +56,4 @@ inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) { return bytes_read; } -// To avoid conflicts with std::numeric_limits::max() in -// file_data_loader.cpp. -#undef max #endif diff --git a/extension/data_loader/targets.bzl b/extension/data_loader/targets.bzl index 81269cfef5..ceb92d7ec7 100644 --- a/extension/data_loader/targets.bzl +++ b/extension/data_loader/targets.bzl @@ -40,7 +40,7 @@ def define_common_targets(): runtime.cxx_library( name = "file_data_loader", srcs = ["file_data_loader.cpp"], - headers = ["file.h"], + headers = ["pread.h"], exported_headers = ["file_data_loader.h"], visibility = [ "//executorch/test/...",