Skip to content

[stdlib] Fix return type of swift_{uint64,int64,float*}ToString #24181

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

Merged

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.

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.
@compnerd
Copy link
Member

CC: @airspeedswift

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Nov 16, 2019

@airspeedswift would you be able to have a look please? Is there anyone else who could be pinged about reviewing this PR?

@stephentyrone stephentyrone self-requested a review November 19, 2019 14:13
@stephentyrone
Copy link
Contributor

@swift-ci please smoke test

@stephentyrone
Copy link
Contributor

Thanks @zhuowei. Sorry that we didn't see this earlier. FWIW, your best bet is to tag me or @tbkka for this stuff.

@stephentyrone
Copy link
Contributor

stephentyrone commented Nov 19, 2019

N.B. for anyone playing along at home: this was a "safe" return type mismatch because the actual implementation has a signature compatible with the signature used; the high 32b of the result were simply ignored by the caller on 32b targets (both i386 and armv7 return 64b results in register, with the low part in the register where a 32b result would go), but they are semantically guaranteed to be zero anyway.

@tbkka
Copy link
Contributor

tbkka commented Nov 19, 2019

The return value here is the length of the formatted string, so a 32-bit value would be preferable to avoid overhead on 32-bit platforms. I believe this is all internal, so changing the type won't break the ABI.

Of course, that might mean touching more places to get the types to line up all the way down.

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Nov 19, 2019

Does it mean that this PR needs any more changes to pass the review and be merged? I'm a co-maintainer of @swiftwasm, and as far as I'm aware @zhuowei is not actively maintaining it currently, but I have access to the PR branch and I'm ready to push it forward and to apply any changes if needed.

@stephentyrone
Copy link
Contributor

stephentyrone commented Nov 21, 2019

I agree with @tbkka that it would be better if these just returned int32. But, the cpp functions are marked SWIFT_RUNTIME_STDLIB_API, so we formally can't modify them (they are API), and the difference in efficiency is minimal.

@MaxDesiatov since we know that the result is always in-range, can you replace the Int inits (line 172 and similar) with Int(truncatingIfNeeded: ...)? This will eliminate the check so that the only overhead on 32b platforms is zeroing out a register, which is sufficiently cheap to not worry about.

Copy link
Contributor

@tbkka tbkka left a comment

Choose a reason for hiding this comment

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

But, the cpp functions are marked SWIFT_RUNTIME_STDLIB_API, so we formally can't modify them...

Phooey. In that case, this is the right fix.

@stephentyrone
Copy link
Contributor

Actually, I'll just merge this and make the other change myself as a follow on. Thanks all!

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