Skip to content

Commit

Permalink
[Build] fix file_data_loader.cpp build issues for windows
Browse files Browse the repository at this point in the history
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 pytorch#4661
  • Loading branch information
python3kgae committed Aug 28, 2024
1 parent a79b1a6 commit c709a7d
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 3 deletions.
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
5 changes: 5 additions & 0 deletions extension/data_loader/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions extension/data_loader/Unix/file.h
Original file line number Diff line number Diff line change
@@ -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 <unistd.h>
53 changes: 53 additions & 0 deletions extension/data_loader/Windows/file.h
Original file line number Diff line number Diff line change
@@ -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 <executorch/runtime/platform/compiler.h> // For ssize_t.
#include <io.h>

#include <windows.h>

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<int32_t>::max() in
// file_data_loader.cpp.
#undef max
8 changes: 5 additions & 3 deletions extension/data_loader/file_data_loader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

#include <executorch/extension/data_loader/file_data_loader.h>
#include <file.h>

#include <algorithm>
#include <cerrno>
Expand All @@ -17,7 +18,6 @@
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#include <executorch/runtime/core/error.h>
#include <executorch/runtime/core/result.h>
Expand Down Expand Up @@ -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<char*>(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_);
}

Expand Down

0 comments on commit c709a7d

Please sign in to comment.