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

Fix the GenericTest.Vector*Boolean #47070

Merged
merged 1 commit into from
Jan 16, 2021
Merged

Conversation

sandreenko
Copy link
Contributor

Bool need special marshaling attribute, the test was failing on arm64 apple
where C++ was expecting them to be packed as 1 byte.

Thanks, @jkoritzinsky for helping with this.

Bool need special marshalling attribute, the test was failing on arm64 apple
where C++ was expecting them to be packed as 1 byte.
@sandreenko sandreenko added arch-arm64 test-bug Problem in test source code (most likely) os-ios Apple iOS labels Jan 16, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@sandreenko
Copy link
Contributor Author

cc @sdmaclea, @janvorli , @mangod9

@sandreenko
Copy link
Contributor Author

I think it was the lest test that was failing constantly in my local runs (with #46665 applied)

@sdmaclea
Copy link
Contributor

sdmaclea commented Jan 16, 2021

I am not sure this is the right fix. As this requires all C# code to be changed to work with Apple Silicon. Maybe the default marshalling of C# bool on Apple Silicon needs to be changed instead?

It seems like we should have a stable cross platform expectation C# <-> C++ mapping across platforms. Although in general C++ types do not have defined sized, there are newer types with well defined and stable size across platforms. i.e int8_t ... int64_t and uint8_t ... uint64_t

@jkoritzinsky
Copy link
Member

This fix fixes a bug that was on all platforms that we just hadn’t noticed. The previous code was semantically wrong in all cases and just happened to work.

@tannergooding
Copy link
Member

The previous code was semantically wrong in all cases and just happened to work.

Yep, my bad 😄. I always forget that bool is int by default, even on non-Windows (this also why I explicitly use byte and int in my own P/Invoke code >.>).

@sandreenko sandreenko merged commit caeba7d into dotnet:master Jan 16, 2021
@sandreenko sandreenko deleted the fixPinvokeTest branch January 16, 2021 04:28
@ghost ghost locked as resolved and limited conversation to collaborators Feb 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-Interop-coreclr os-ios Apple iOS test-bug Problem in test source code (most likely)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants