Skip to content

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 8, 2024

No description provided.

@kripken kripken requested review from aheejin and tlively February 8, 2024 17:59
Comment on lines 519 to 520
// segment has an offset at an address larger than kMaxSize, which traps at
// startup but would be a non-validating module for us to emit.
Copy link
Member

Choose a reason for hiding this comment

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

I am confused by this comment. Would the segment fail to validate or trap? It can't do both at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

A segment with a huge but valid offset that is larger than kMaxSize will trap. But we fail to validate if we raise initial to such sizes. This is due to the max size being 32 bits but Address being 64 bits. Perhaps another way to fix this is to refactor Address to be 32 bits in wasm32, but I think we had a reason for the current approach.

Copy link
Member

Choose a reason for hiding this comment

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

I see, so big segment offsets lead to traps and big tables lead to validation errors. Makes sense. I guess I don't understand the connection between them, though. Is the fuzzer generating segment offsets and table sizes based off of each other?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the code adjacent to this (right above it) increases initial based on the segments it sees, that is, it is trying to avoid a trap by ensuring a big-enough initial size.

@kripken
Copy link
Member Author

kripken commented Feb 10, 2024

Any other thoughts here or is this ok to land?

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

I think it would be good to clarify the language in the comment, but otherwise LGTM.

@kripken
Copy link
Member Author

kripken commented Feb 12, 2024

Ok, I rewrote the comment.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Thanks, this makes sense to me!

@kripken kripken merged commit 34d35ae into WebAssembly:main Feb 12, 2024
@kripken kripken deleted the val.big.table branch February 12, 2024 21:09
@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

2 participants