Skip to content

New attribute layout: Read from either int8_t or uint8_t#926

Merged
ax3l merged 6 commits intoopenPMD:devfrom
franzpoeschel:fix-new-layout-vecstring
Mar 9, 2021
Merged

New attribute layout: Read from either int8_t or uint8_t#926
ax3l merged 6 commits intoopenPMD:devfrom
franzpoeschel:fix-new-layout-vecstring

Conversation

@franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Feb 8, 2021

CppReference on char:

has the same representation and alignment as either signed char or unsigned char, but is always a distinct type

When doing IO::DefineVariable<char>() in ADIOS2, the resulting variable will be stored in a platform-dependent way:

  int8_t    /data/8/meshes/E/vector_of_string  {3, 7}
  uint8_t   /data/8/meshes/E/vector_of_string  {3, 7}

Both of the above options may be seen on different platforms.

This PR fixes reading char-based variables if they were written on a platform with the same representation for the char type.

I've done some cross-platform testing and come to the conclusion that char/string-based variables are currently not implemented in a portable way in ADIOS2.
Case: Writing on some local x86-based machine where char is an unsigned type, reading on Summit where char is a signed type:

  • Define a scalar string variable Variable<string>. The variable will not even occur in IO::AvailableVariables().
  • Define a char variable Variable<char>. IO::InquireVariable<char>() will fail.

Interestingly, writing on Summit and reading on the local machine works fine.

I'll write up a simple reproducer for a bug report, in the meantime this PR allows interaction within the same platform at least.

Also, I think our Datatype enum is also broken in this regard: We list CHAR and UCHAR, but not SCHAR. char can be either signed char or unsigned char, but our defintions imply that char is simply the signed variant. ADIOS2 gives us types with explicit signedness (uint8_t or int8_t) and I cannot represent this sensibly with our datatypes.

  • The test breaks when OPENPMD_NEW_ATTRIBUTE_LAYOUT is set manually, fix that
  • Pull ADIOS2: v2.7.0+ #927 first

@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch from 8545d4e to 493b70d Compare February 8, 2021 13:50
@ax3l
Copy link
Member

ax3l commented Feb 11, 2021

I'll write up a simple reproducer for a bug report, in the meantime this PR allows interaction within the same platform at least.

Thanks! Can you then cross-link the ADIOS2 bug report and this PR please?

Also, I think our Datatype enum is also broken in this regard: We list CHAR and UCHAR, but not SCHAR. char can be either signed char or unsigned char, but our defintions imply that char is simply the signed variant. ADIOS2 gives us types with explicit signedness (uint8_t or int8_t) and I cannot represent this sensibly with our datatypes.

That's correct - there are three distinct char types in C++. I think we used schar at some point or skipped it for using it for something else (like bools) but have not gotten back to it (also in part b/c adios never supported it). But yes, ideally that would just be another distinct type.

@franzpoeschel
Copy link
Contributor Author

Alright, the reproducer for ADIOS2 interestingly shows no broken behavior, other than the differing signed-ness cross-platform. I'll put it on the agenda for this evening/morning's VC whether this should be considered a bug or not.
I'll then need to investigate what is causing me the cross-platform issues.

@franzpoeschel franzpoeschel changed the title New attribute layout: Read from either int8_t or uint8_t (workaround until upstream fix) New attribute layout: Read from either int8_t or uint8_t (workaround until upstream fix ??) Feb 11, 2021
@franzpoeschel
Copy link
Contributor Author

I think that making things portable across platforms requires introducing a clear distinction between CHAR, SCHAR and UCHAR which I propose we do in a separate PR. For now, this PR ensures that things at least work within the same platform.

@ax3l ax3l self-assigned this Feb 21, 2021
@ax3l ax3l self-requested a review February 21, 2021 07:17
Comment on lines +162 to +163
std::cout << "location.dt=" << location.dt <<
"\tdetermineDatatype=" << determineDatatype< T > () << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a leftover debug message and should go into the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, will merge that with the error message

std::vector< std::string > res( height );
for( size_t i = 0; i < height; ++i )
std::vector< std::string > res( height );
if( std::is_signed< char >::value ==
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to add a bit more in-code comments for this part? Just to make it easier for our future selves, reading the workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch 2 times, most recently from 2bdb725 to ed5f462 Compare February 22, 2021 12:29
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Feb 22, 2021

Hm, the Windows error in the newly added test is exactly the same one that already made me deactivate the bp4_steps test under Windows. Think it's time to boot up the VM again.

3: C:\projects\openpmd-api\test\SerialIOTest.cpp(42): FAILED:
3: due to unexpected exception with message:
3:   Unknown iterationEncoding: �����

EDIT: I rebased this on your adios-2.7.0 branch now to see whether the error persists.

EDIT: It's not an ADIOS issue. MSVC doesn't like that I use placement new to put a string into the char array of PreloadAdiosVariable. Link to CI

@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch from ed5f462 to b6377dd Compare February 22, 2021 13:58
@ax3l ax3l changed the title New attribute layout: Read from either int8_t or uint8_t (workaround until upstream fix ??) New attribute layout: Read from either int8_t or uint8_t Feb 23, 2021
@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch 14 times, most recently from 55bfcb0 to 312e77d Compare March 1, 2021 14:05
@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch 2 times, most recently from 10b20b4 to f548f33 Compare March 2, 2021 10:03
@franzpoeschel
Copy link
Contributor Author

I think I've found out why the new ADIOS2 schema never worked on MSVC: It apparently has trouble understanding placement new for arrays. The recent commit replaces:

                 new( dest ) T[ numItems ]{};

by:

                for( size_t i = 0; i < numItems; ++i )
                {
                    new( dest + i ) T();
                }

And suddenly, things are working.

@ax3l ax3l mentioned this pull request Mar 4, 2021
3 tasks
@ax3l
Copy link
Member

ax3l commented Mar 4, 2021

Ouch, seems to be fixed in Visual Studio 2019: https://developercommunity.visualstudio.com/t/c-placement-new-is-incorrectly-compiled/206439

Do you want to add a comment?

@franzpoeschel
Copy link
Contributor Author

Ouch, seems to be fixed in Visual Studio 2019: https://developercommunity.visualstudio.com/t/c-placement-new-is-incorrectly-compiled/206439

Do you want to add a comment?

Ah, it's good to have a link for that, thanks for figuring that out.
I have added that in the code.

@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch 2 times, most recently from f3cb0a9 to b23ec1a Compare March 8, 2021 11:32
@franzpoeschel franzpoeschel force-pushed the fix-new-layout-vecstring branch from b23ec1a to d424b02 Compare March 9, 2021 15:45
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you! 🚀 ✨

@ax3l ax3l merged commit 6e3b8b2 into openPMD:dev Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants