Skip to content

[stdlib] fix return type of getNumRuntimeFunctionCounters #24182

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

Conversation

zhuowei
Copy link
Contributor

@zhuowei zhuowei commented Apr 20, 2019

The return type of these functions are uint64_t in Stubs.cpp but UInt in the Swift code; this changes the Swift code to match the C++ return type.

Found when compiling the stdlib for WebAssembly, which requires that all return types match: UInt maps to i32 while uint64_t maps to i64, so functions calling these functions fail the validation.

@zhuowei zhuowei force-pushed the fix-getNumRuntimeFunctionCounters-return-type branch from f401981 to 3a43aa2 Compare April 20, 2019 07:57
@compnerd
Copy link
Member

CC: @airspeedswift
Can we possibly let the UInt64 propagate through?

@zhuowei
Copy link
Contributor Author

zhuowei commented Apr 20, 2019

@compnerd It didn't compile without the Int() casts, so I decided to just add the casts to get it working. Should I try keeping it as UInt64, then?

@compnerd
Copy link
Member

I think that if it doesn't bleed into any public interfaces, we should try to switch over the types since that would prevent truncation.

@zhuowei
Copy link
Contributor Author

zhuowei commented Apr 30, 2019

@compnerd The number of runtime counters is used to index into an array. Array indices must be Int, so there's no point keeping it UInt64.

Here's the compile error if I remove the Int() casts:
https://gist.github.com/zhuowei/6d694894da292607c8a382afcf7cf9cc

@compnerd
Copy link
Member

compnerd commented May 7, 2019

Sounds good

@compnerd
Copy link
Member

compnerd commented May 7, 2019

@swift-ci please test

The return type of getNumRuntimeFunctionCounters is defined as
uint64_t in RuntimeInvocationsTracking.cpp, but it has return type
Int in RuntimeFunctionCounters.swift.

Found when compiling the stdlib for WebAssembly, as WebAssembly validates
return types. uint64_t corresponds to i64, but Int is i32,
so the program fails validation.
@MaxDesiatov MaxDesiatov force-pushed the fix-getNumRuntimeFunctionCounters-return-type branch from 3a43aa2 to c94e876 Compare November 16, 2019 14:11
@MaxDesiatov
Copy link
Contributor

@compnerd conflicts have been fixed, would you be able to have another look please?

@stephentyrone
Copy link
Contributor

@swift-ci please test

@stephentyrone
Copy link
Contributor

@swiftix would we rather fix this by having the cpp funds return long? Can these ever overflow 32b, and if they do is it desired that the conversion to Int to trap in the Swift source?

@MaxDesiatov
Copy link
Contributor

@stephentyrone are there any changes needed here based on the review of #24181?

@atrick
Copy link
Contributor

atrick commented Nov 22, 2019

These are indices, not counts, so trapping Int is the correct choice.

@stephentyrone stephentyrone merged commit b9c1def into swiftlang:master Nov 22, 2019
@MaxDesiatov MaxDesiatov deleted the fix-getNumRuntimeFunctionCounters-return-type branch November 22, 2019 20:53
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

Successfully merging this pull request may close these issues.

5 participants