Skip to content

[embind] Reuse signature codes from em_asm. NFC #24611

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
merged 1 commit into from
Jul 8, 2025

Conversation

RReverser
Copy link
Collaborator

@RReverser RReverser commented Jun 19, 2025

Aside from a minor discrepancy, there is no point in maintaining two separate sets of type pack to signature conversions, especially now that they've grown with wasm64 conditionals. Keeping them in one place reduces complexity in bind.h and ensures the signatures stay in sync.

@RReverser RReverser requested review from brendandahl and sbc100 June 19, 2025 17:47
@RReverser RReverser marked this pull request as draft June 19, 2025 17:57
Aside from a minor discrepancy, there is no point in maintaining two separate sets of type pack to signature conversions, especially now that they've grown with wasm64 conditionals. Keeping them in one place reduces complexity in bind.h and ensures the signatures stay in sync.
@RReverser RReverser marked this pull request as ready for review June 20, 2025 16:11

// TODO: should we add this override to em_asm?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sbc100 Would love your thoughts on this one. In other places we represent size_t aka unsigned long as number, even though it's 32 bit on wasm32 and 64 on wasm64, so it's lossy on wasm64.

Should we change EM_ASM behaviour to match that, or should we change those other places to prevent precision loss for unsigned long, or is it expected that they handle it differently?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm maybe "most" is an exaggeration. Embind itself also encodes size_t aka unsigned long as a BigInt on wasm64 and number otherwise.

Shame that you can't distinguish between size_t and "plain" longs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Without looking at this specific instance yet) I think the only times we want to use i53 numbers for 64-bit values is:

  1. When we know that thing is a pointer type (e.g. when p is used in signature of a JS function)
  2. When there is an explicit opt-in (i.e. __i53abi tag on JS functions).

Otherwise 64-bit values should be preserved either via bigint (or pair-of-numbers)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, but in that case sounds like we should instead remove this override from Embind, as it will currently use i53 for any unsigned long.

Does that sound right / should I do that in the same PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this PR is NFC than lets keep that way,.. is it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing the behavior in another PR to be consistent sounds good to me. FWIW, I thought size_t was already a bigint on memory64, but I'm guessing we left it int32 to avoid users having to deal with BigInt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We handle size_t like we handle pointers yes. They have both have pointer-like behaviour in that they have a different size on wasm32 and wasm64.

Other types such as uint64 have the same size on both wasm32 and wasm64.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They have both have pointer-like behaviour in that they have a different size on wasm32 and wasm64.

But for pointers we translate them to int53 aka plain numbers in JS code, including in Embind, whereas size_t is different, as it will become bigint on wasm64.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is, I suppose, a limitation of type system. I assume its not possible to distinguish between unsigned long and size_t? If it is possible we should treat size_t like we do pointers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is, I suppose, a limitation of type system. I assume its not possible to distinguish between unsigned long and size_t?

Indeed. That's what I said above too:

Shame that you can't distinguish between size_t and "plain" longs.

@RReverser
Copy link
Collaborator Author

@sbc100 @brendandahl Ping.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 8, 2025

Sorry, I've been away on vacation.

@RReverser
Copy link
Collaborator Author

I know, all good. Just pinged to get some attention now that you're back :)

@RReverser RReverser changed the title [embind] Reuse signature codes from em_asm [embind] Reuse signature codes from em_asm. NFC Jul 8, 2025
@RReverser
Copy link
Collaborator Author

Ok changed the title to NFC as per discussion, please review as-is.

@RReverser RReverser merged commit 25c1806 into emscripten-core:main Jul 8, 2025
30 checks passed
@RReverser RReverser deleted the reuse-sig branch July 8, 2025 22:54
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.

3 participants