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

[PostEmscripten] Fix calcSegmentOffsets for large offsets #6260

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

sbc100
Copy link
Member

@sbc100 sbc100 commented Jan 31, 2024

Specifically offsets larger than 2^32 which were being interpreted misinterpreted here as very large int64_t values.

@sbc100 sbc100 requested a review from kripken January 31, 2024 02:21
@sbc100
Copy link
Member Author

sbc100 commented Jan 31, 2024

I'm not sure it worth writing a test for this.. maybe you can suggest a way if you feel like it needs one.

This bug is currently preventing some of the "2gb" tests in emscripten from running (where static data segments are >2Gb in the 32-bit space)

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.

In wasm64 don't these need to be 64-bit?

@sbc100
Copy link
Member Author

sbc100 commented Jan 31, 2024

In wasm64 don't these need to be 64-bit?

The Address being assigned to here is 64-bit or 32-bit accordingly, that is not the problem..

The bug being fixed here only occurs when on wasm32 when the value being read is is > 2&32.

Prior to this change the getInteger() would interpret this larger 32-bit value as a negative 32-bit value, which is wrong. Data segment offsets should be interpreted as unsigned values.. which I think is exactly what this change does.

@@ -317,7 +316,7 @@ struct PostEmscripten : public Pass {
// The first operand is the function pointer index, which must be
// constant if we are to optimize it statically.
if (auto* index = curr->operands[0]->dynCast<Const>()) {
size_t indexValue = index->value.getInteger();
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just a drive by fix, that I noticed while fixing the line above.

Function pointer are also unsigned values.

@sbc100 sbc100 force-pushed the post_emscripten_high_offsets branch from c5a7893 to 512738f Compare January 31, 2024 18:04
@sbc100
Copy link
Member Author

sbc100 commented Jan 31, 2024

I added a test!

@sbc100 sbc100 requested a review from kripken January 31, 2024 18:07
@sbc100 sbc100 force-pushed the post_emscripten_high_offsets branch from 512738f to 94c15eb Compare January 31, 2024 18:23
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.

I see now, thanks. lgtm % question about the test.

test/lit/passes/post-emscripten.wast Show resolved Hide resolved
Specifically offsets larger than 2^32 which were being interpreted
misinterpreted here as very large int64_t values.
@sbc100 sbc100 force-pushed the post_emscripten_high_offsets branch from 94c15eb to 715513b Compare January 31, 2024 18:32
@sbc100 sbc100 enabled auto-merge (squash) January 31, 2024 18:33
@sbc100 sbc100 disabled auto-merge January 31, 2024 19:08
@sbc100 sbc100 merged commit 396a826 into main Jan 31, 2024
14 checks passed
@sbc100 sbc100 deleted the post_emscripten_high_offsets branch January 31, 2024 19:08
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
…y#6260)

Specifically offsets larger than 2^32 which were being interpreted
misinterpreted here as very large int64_t values.
@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

2 participants