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

[cxx-interop][stdlib] windows - use new hash inline functions like other platforms #78161

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

hyp
Copy link
Contributor

@hyp hyp commented Dec 13, 2024

…her platforms

The PR #77857 added windows-specific workaround for #77856, that happened after #77843. Unfortunately this caused a new issue on windows - #78119. It looks like windows is suffering from a similar serialization issue as libstdc++, although its even more complex as the callAsFunction is not only a derived function from a base class, the base class also has a static call operator. In any case, the libstdc++ callAsFunction deserialization fix should align with the static operator () deserialization too, so for now make windows use the same workaround as other platforms to avoid the deserialization crash (#78119).

This change was tested on i686 windows too, ensuring that IR verifier crash no longer happens

…her platforms

The PR #77857 added windows-specific workaround for #77856, that happened after #77843. Unfortunately this caused a new issue on windows - #78119. It looks like windows is suffering from a similar serialization issue as libstdc++, although its even more complex as the callAsFunction is not only a derived function from a base class, the base class although has a static call operator. In any case, the libstdc++ callAsFunction deserialization fix should align with the static operator () deserialization too, so for now make windows use the same workaround as other platforms to avoid the deserialization crash (77856).

This change was tested on i686 windows too, ensuring that IR verifier crash no longer happens
@hyp
Copy link
Contributor Author

hyp commented Dec 13, 2024

@swift-ci please test

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Dec 13, 2024
Copy link
Contributor

@egorzhdan egorzhdan left a comment

Choose a reason for hiding this comment

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

LGTM, just one question regarding the IR verifier issue we saw earlier

@@ -4,19 +4,19 @@

/// Used for std::string conformance to Swift.Hashable
typedef std::hash<std::string> __swift_interopHashOfString;
inline std::size_t __swift_interopComputeHashOfString(std::string str) {
inline std::size_t __swift_interopComputeHashOfString(const std::string &str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I guess making the parameter a const reference avoids the IR verifier failure?

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!

@hyp hyp merged commit 978e72c into main Dec 13, 2024
5 checks passed
@hyp hyp deleted the hyp/hash-bless-you-and-merry-xmas branch December 13, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants