Skip to content

[Build] fix file_data_loader.cpp build issues for windows #4899

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

Draft
python3kgae wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[Build] fix file_data_loader.cpp build issues for windows #4899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c709a7d
[Build] fix file_data_loader.cpp build issues for windows
python3kgae Aug 26, 2024
be77d20
Use single file.h
python3kgae Aug 28, 2024
394f372
Add file.h to targets.bzl
python3kgae Aug 28, 2024
bc600ce
Update per comment.
python3kgae Aug 30, 2024
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
8 changes: 5 additions & 3 deletions extension/data_loader/file_data_loader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* LICENSE file in the root directory of this source tree.
*/

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

#include <algorithm>
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
59 changes: 59 additions & 0 deletions extension/data_loader/pread.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* 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.
*/

// This file ensures that a pread-compatible function is defined in the global namespace for windows and posix environments.

#pragma once

#ifndef _WIN32
#include <unistd.h>
#else
#include <executorch/runtime/platform/compiler.h> // For ssize_t.
#include <io.h>

#include <windows.h>
// To avoid conflicts with std::numeric_limits<int32_t>::max() in
// file_data_loader.cpp.
Comment on lines +19 to +21
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

On Windows, including <windows.h> without NOMINMAX typically defines both min and max macros. This header only #undef max, but file_data_loader.cpp uses std::min, which can be broken by the min macro. Consider defining NOMINMAX before including <windows.h> and/or #undef min alongside #undef max here.

Suggested change
#include <windows.h>
// To avoid conflicts with std::numeric_limits<int32_t>::max() in
// file_data_loader.cpp.
#ifndef NOMINMAX
#define NOMINMAX
#endif
#include <windows.h>
// To avoid conflicts with std::min/std::max and std::numeric_limits<int32_t>::max()
// in file_data_loader.cpp.
#undef min

Copilot uses AI. Check for mistakes.
#undef max

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));
Comment on lines +16 to +26
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

pread.h isn't self-contained on Windows: it uses memset, errno, and EIO but doesn't include headers that declare/define them. Since file_data_loader.cpp includes this header before <cstring>/<cerrno>, this can cause Windows compile errors. Add the appropriate standard headers (e.g., <cstring> and <cerrno>/<errno.h>).

Copilot uses AI. Check for mistakes.
overlapped.Offset = offset;
overlapped.OffsetHigh = offset >> 32;
Comment on lines +27 to +28
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

overlapped.OffsetHigh = offset >> 32; can be undefined behavior on 32-bit builds because offset is a size_t (likely 32 bits) and shifting by 32 is UB. Consider first widening to uint64_t and then assigning the low/high 32-bit parts with explicit casts to DWORD to avoid UB and narrowing warnings.

Suggested change
overlapped.Offset = offset;
overlapped.OffsetHigh = offset >> 32;
ULONGLONG offset64 = (ULONGLONG)offset;
overlapped.Offset = (DWORD)offset64;
overlapped.OffsetHigh = (DWORD)(offset64 >> 32);

Copilot uses AI. Check for mistakes.

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();
Comment on lines +30 to +35
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

ReadFile takes a DWORD byte count, but this wrapper passes size_t nbytes directly. On 64-bit this can silently truncate (and may warn/error under -Wconversion/-Werror). Add an explicit bounds check (e.g., nbytes <= MAXDWORD) and cast to DWORD before calling ReadFile; return -1 with an appropriate errno if the request is too large.

Copilot uses AI. Check for mistakes.
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;
Comment on lines +49 to +54
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

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

The ERROR_HANDLE_EOF case currently sets errno = 0 and returns -1. POSIX pread signals EOF by returning 0 (not an error). Returning -1 here can lead to misleading logs like strerror(0) ("Success") and incorrect error handling. Return 0 for this case to match pread semantics.

Suggested change
break;
default:
errno = EIO;
break;
}
return -1;
return 0;
default:
errno = EIO;
return -1;
}

Copilot uses AI. Check for mistakes.
}
return bytes_read;
}

#endif
1 change: 1 addition & 0 deletions extension/data_loader/targets.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def define_common_targets():
runtime.cxx_library(
name = "file_data_loader",
srcs = ["file_data_loader.cpp"],
headers = ["pread.h"],
exported_headers = ["file_data_loader.h"],
visibility = [
"//executorch/test/...",
Expand Down