Skip to content
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.

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

master fails on 32bit Windows in BP5 Serializer #4105

Open
ax3l opened this issue Mar 25, 2024 · 6 comments
Open

master fails on 32bit Windows in BP5 Serializer #4105

ax3l opened this issue Mar 25, 2024 · 6 comments

Comments

@ax3l
Copy link
Contributor

ax3l commented Mar 25, 2024

Describe the bug

The current master does not build due to a uint64_t / size_t mismatch on ILP32 platforms:

2024-03-25T06:36:25.2420671Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1600,68): error C2397: conversion from 'uint64_t' to 'size_t' requires a narrowing conversion [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2425984Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1600,68): warning C4244: 'initializing': conversion from 'uint64_t' to 'size_t', possible loss of data [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2429288Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1601,32): warning C4244: '+=': conversion from 'uint64_t' to 'size_t', possible loss of data [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]
2024-03-25T06:36:25.2431866Z D:\a\openPMD-api\openPMD-api\src\dep-adios2\ADIOS2-fix-static-blosc2-build\source\adios2\toolkit\format\bp5\BP5Serializer.cpp(1608,69): error C2397: conversion from 'uint64_t' to 'size_t' requires a narrowing conversion [D:\a\openPMD-api\openPMD-api\src\build-adios2\source\adios2\adios2_core.vcxproj]

To Reproduce
Build on Visual Studio / w/ MSVC in 32bit mode ("x86").

Expected behavior
Build on LP32, ILP32, LLP64 and LP64 platforms.

Desktop (please complete the following information):

  • OS/Platform: Windows Visual Studi 16 2019, 32bit (cross-compiled from 64bit)
  • Build: MSVC 19.29.30154.0 (x86) aka 32bit

Additional context
Building new wheels and this worked in the past: openPMD/openPMD-api#1554

You might wonder: what is wrong with you, why 32bit Windows?
You are right, this is to get an in with experimental physicists. In labs, some hardware has very weird control software and limited/old driver support, which only runs on 32bit Windows.

I am ok to ditch this eventually, but since it is easy to fix and keep general, we should do it.

@ax3l
Copy link
Contributor Author

ax3l commented Mar 25, 2024

Should size_t Position = 0; simply be a uint64_t Position = 0; @eisenhauer ?

@eisenhauer
Copy link
Member

Ugh. Making this stuff build and run cleanly on 32- and 64-bit was something I advocated for years ago, but couldn't get off the ground. (Doing it properly really requires saving output files from different platforms, making sure what's written on one is readable on another, etc. FFS does this, but we've not done it for ADIOS.) So, to the extent that we can tweak something and it gets us where we need to be, I'm completely supportive. I think the question is how far we go down the road from "it compiled once" through "it's in CI so we don't break it accidentally" to "we support and test 32/64 bit interoperability to the extent possible".

WRT the specific question, yeah, Position is probably uint64_t, but given the pickiness of VS, making that change may require a bunch more casts somewhere. If you've go this setup and can sort out the compile issues, I'm happy to look at the PR, but the number of background code projects I've got on my plate is a little daunting at the moment, so tackling it myself might be slow.

@ax3l
Copy link
Contributor Author

ax3l commented Mar 25, 2024

Generally, yes I would expect a file format (H5, BP4, BP5) to handle for me the concerns of how to encode a file so I can read it back between any system (little/big endian, 32bit to 64bit, platform specific sizes of ints and floats). Otherwise it is not a portable format.

That said, at the current point (this issue), I am happy if things compile and in/out consistently on the same platform...

This is what we deploy on downstream:

openPMD-api deployment targets

  • Linux 32bit (i686) - GH actions, cross-compiled
    • Note: mostly to avoid hard-coding platform assumptions
  • Linux 64bit (x86_64) - GH actions
  • Linux aarch64 - CircleCI
  • Linux ppc64le (little endian) - TravisCI using its free partner queue
    • Note: Lassen (LLNL) and Summit (OLCF) are the last machines we need it on.
  • Windows 32bit (x86) - GH actions, cross-compiled from 64bit
    • Note: mostly for easy install on experimentalist hardware and to avoid hard-coding platform assumptions
  • Windows 64bit (x86_64) - GH actions from 64bit
  • MacOS 64bit (x86_64) - GH actions from 64bit
    • Note: Apple is fading this out mid-term
  • MacOS 64bit (aarch64/arm64) - GH actions, cross-compiled from x86_64
    • GH actions has now official aarch64 hardware, too

openPMD-api deployment variants

  • all-shared: conda, Spack, etc.
  • all-static (besides Python module, which is a shared lib): Pip

@eisenhauer
Copy link
Member

Even coming up with a big-endian platform to do CI on is a challenge. Heterogeneity isn't what it used to be...

@ax3l
Copy link
Contributor Author

ax3l commented Mar 25, 2024

(that said, I am personally not needing big endian support, luckily HPC has no such machine atm :D)

@eisenhauer
Copy link
Member

Last BE machine I knew of was a Sparc-based machine at JAXA... Before it was activated I assumed that as a PowerPC machine Summit would be BE, but they put it in LE mode. Lots of BP5 code is untested for BE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants