search

ornladios / ADIOS2

Next generation of ADIOS developed in the Exascale Computing Program
https://adios2.readthedocs.io/en/latest/index.html
Apache License 2.0
266 stars 122 forks source link

`master` fails on 32bit Windows in BP5 Serializer #4105

Open ax3l opened 3 months ago

ax3l commented 3 months ago

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):

Additional context Building new wheels and this worked in the past: https://github.com/openPMD/openPMD-api/pull/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 commented 3 months ago

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

eisenhauer commented 3 months ago

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 commented 3 months ago

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

openPMD-api deployment variants

eisenhauer commented 3 months ago

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

ax3l commented 3 months ago

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

eisenhauer commented 3 months ago

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.