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.

Sign up for GitHub

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

Jump to bottom

Automatically convert i32 pointer args to unsigned when CAN_ADDRESS_2GB is set #19740

Merged
merged 1 commit into from
Jun 30, 2023
Merged

Automatically convert i32 pointer args to unsigned when CAN_ADDRESS_2GB is set #19740

merged 1 commit into from
Jun 30, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jun 28, 2023

This extends the use automatically-generated JS wrappers to handle conversion of incoming i32 pointer to u32 JS numbers. This is only needed when CAN_ADDRESS_2GB is set.

See #19737

@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch from 6ca2414 to a84657f Compare June 28, 2023 19:26
@sbc100 sbc100 mentioned this pull request Jun 28, 2023
Merged
@sbc100 sbc100 requested a review from kripken June 28, 2023 19:28
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 28, 2023

I think this change might also allows us to completely remove the unsignPointers pass.. but I'm not sure.

@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch 2 times, most recently from 864860e to b09ddf0 Compare June 28, 2023 19:55
kripken
kripken reviewed Jun 28, 2023
View reviewed changes
emcc.py Outdated Show resolved Hide resolved
@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch 2 times, most recently from 8eb5546 to 1206ca2 Compare June 28, 2023 21:05
kripken
kripken reviewed Jun 28, 2023
View reviewed changes
emcc.py Outdated Show resolved Hide resolved
emcc.py Outdated Show resolved Hide resolved
emcc.py Outdated Show resolved Hide resolved
src/library.js Outdated Show resolved Hide resolved
@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch 5 times, most recently from 34d58ac to bb88913 Compare June 28, 2023 22:22
@sbc100 sbc100 requested a review from kripken June 28, 2023 22:27
kripken
kripken reviewed Jun 29, 2023
View reviewed changes
src/library.js
requestedSize = requestedSize >>> 0;
#if !MEMORY64 && !CAN_ADDRESS_2GB
// With CAN_ADDRESS_2GB or MEMORY64, pointers are already unsigned.
requestedSize >>>= 0;
Copy link
Member

Choose a reason for hiding this comment

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

Why do this? The code is only run if requestedSize === (requestedSize >>> 0).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason for this special case is that we still want emscripten_resize_heap(MAX_INT32) to fail here even if CAN_ADDRESS_2GB is not set.. at least the tests expect that. So this function in particular has to force its parameter to unsigned even when CAN_ADDRESS_2GB is not set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact we want to fail especially if CAN_ADDRESS_2GB is not set.

Without this line the below assert(requestedSize > oldSize); will fail because requestedSize would come through as -1.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right... this is for proper error handling. Higher values are not valid and we error on them.

kripken
kripken reviewed Jun 29, 2023
View reviewed changes
src/library.js
requestedSize = requestedSize >>> 0;
#if !MEMORY64 && !CAN_ADDRESS_2GB
// With CAN_ADDRESS_2GB or MEMORY64, pointers are already unsigned.
requestedSize >>>= 0;
Copy link
Member

Choose a reason for hiding this comment

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

Oh, right... this is for proper error handling. Higher values are not valid and we error on them.

src/library.js Outdated Show resolved Hide resolved
src/library_fs.js Show resolved Hide resolved
src/library_strings.js Outdated Show resolved Hide resolved
@sbc100 sbc100 mentioned this pull request Jun 29, 2023
Merged
@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 29, 2023

I split out #19752

sbc100 added a commit that referenced this pull request Jun 29, 2023
@sbc100
Avoid setting CAN_ADDRESS_2GB with MEMORY64 (#19752)
c043654 
Split out from #19740.
@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch from bb88913 to e221930 Compare June 29, 2023 22:42
sbc100
sbc100 commented Jun 29, 2023
View reviewed changes
Copy link
Collaborator Author

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

I reverted the potentially problematic parts of this change in src/library_strings.js.. so hopefully what remains is more conservative.

@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch from e221930 to 3e11c0c Compare June 30, 2023 00:32
@sbc100
Automatically convert i32 pointer args to unsigned when CAN_ADDRESS_2…
96e9b41 
…GB is set

This extends the use automatically-generated JS wrappers to handle
conversion of incoming i32 pointer to u32 JS numbers.  This is only
needed when CAN_ADDRESS_2GB is set.

See #19737
@sbc100 sbc100 force-pushed the automate_unsigned_pointer_args branch from 3e11c0c to 96e9b41 Compare June 30, 2023 00:34
@sbc100 sbc100 requested a review from kripken June 30, 2023 02:48
kripken
kripken approved these changes Jun 30, 2023
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks, and sorry for insisting on those assertions, maybe I'm overly cautious but I think they might help someone.

@sbc100 sbc100 merged commit 1b3533e into main Jun 30, 2023
@sbc100 sbc100 deleted the automate_unsigned_pointer_args branch June 30, 2023 21:58
@sbc100 sbc100 mentioned this pull request Jan 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @kripken