[Build] fix file_data_loader.cpp build issues for windows

[python3kgae]

Two Windows 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 file.h, enabling Windows builds to utilize io.h and implement pread.

For #4661

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4899

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[kirklandsign]

Thank you @python3kgae !

I would prefer that we use a single file.h, and check for ifdef _WIN32. When we use #include please use the full path starting from executorch/... , not adding a include_dir in cmake. Or at least, using a file.h which selects the correct file_posix.h and file_windows.h based on compiler macro. We also maintain an internal build system and it's easier that way.

cc @dbort

[python3kgae]

Thank you @python3kgae !

I would prefer that we use a single file.h, and check for ifdef _WIN32. When we use #include please use the full path starting from executorch/... , not adding a include_dir in cmake. Or at least, using a file.h which selects the correct file_posix.h and file_windows.h based on compiler macro. We also maintain an internal build system and it's easier that way.

cc @dbort

Changed into single file.h.

[kirklandsign]

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line

headers = ["file.h"],

[python3kgae]

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line

headers = ["file.h"],

Like this?

@@ -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/...",

[kirklandsign]

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line

headers = ["file.h"],

Like this?

@@ -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/...",

Yes

[python3kgae]

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line

headers = ["file.h"],

Like this?

@@ -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/...",

Yes

Done.

[dbort]

Normally we try to avoid using platform-based #ifdefs for implementations, and will create separate .cpp files that the build system brings in as necessary. But this is pretty light-weight and self-contained, so it seems fine.

[dbort]

Please call this file pread.h to make its purpose more clear.

[python3kgae]

Updated.

[dbort]

Please add a comment describing the purpose of this header: i.e., it ensures that a pread-compatible function is defined in the global namespace for windows and posix environments.

[python3kgae]

Updated.

[dbort]

Please remove the leading underscores from the param names. Since this is our code and not part of the standard library, we shouldn't define any names beginning with underscore.

[python3kgae]

Updated.

[dbort]

Will this do the right thing on both 32-bit and 64-bit architectures? Seems like it should, but I don't know enough about the definition of DWORD on different architectures

[python3kgae]

DWORD will always be 32 bits on both 32-bit and 64-bit Windows systems.

It's important to note that the nNumberOfBytesToRead parameter for ReadFile is a DWORD. Therefore, if we need to read more than 4GB of data at once, the current implementation needs to be updated to handle larger file sizes.

[dbort]

Please move this to the top of the file, closer to the header that defined it. If you know, please mention which header defined it, since on its own it's not clear why this file should bother undefining a macro that it didn't define.

[python3kgae]

Done.

[shoumikhin]

@tarun292, @kirklandsign can you take a look please?