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

[Parser] Fix Table64 parsing #7298

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 14, 2025

We had some hardcoded i32 indexes, but the table might be table64.

@kripken kripken requested a review from tlively February 14, 2025 23:33
// No table specified, so this is the default.
assert(!wasm.tables.empty());
return wasm.tables[0].get();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@tlively why is this necessary for tables but not memories? That is, when parsing some table.get instructions there is no name for the table. That is the case for e.g.

 (func $table-get
  i32.const 0
  table.get
  drop
)

where there is no table specified. But this does not seem to be the case for memories, where the memory name is always non-null? (see notePointer, above, which does not need to be careful like noteTableIndex)

Copy link
Member

Choose a reason for hiding this comment

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

Oops, just saw this question. Will take a look.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so when the table or memory index is left implicit in the parsed text, ParseDefsCtx::getTable and ParseDefsCtx::getMemory should be responsible for looking up the internal name of table or memory 0. So the table name should always exist here, and if it doesn't, the bug is earlier in the parser.

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.

Tables should always exist at this point, just like memories, so the bug must be earlier.

@kripken
Copy link
Member Author

kripken commented Feb 19, 2025

Ok, I found the bug - tables need to be explicitly set in the temporary objects in IRBuilder, and a few cases were missing. There was no way to notice that before this PR (and hence no way to test it). Fixed.

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