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

Fix spec #50

Merged
merged 3 commits into from
Nov 30, 2023
Merged

Fix spec #50

merged 3 commits into from
Nov 30, 2023

Conversation

f52985
Copy link
Collaborator

@f52985 f52985 commented Nov 30, 2023

This PR fixes spec for Wasm 1 and 2, especially regarding module instantiation and call_indirect.

@f52985 f52985 requested a review from rossberg November 30, 2023 05:25
@@ -116,8 +116,7 @@ rule Step_read/call:
rule Step_read/call_indirect-call:
z; (CONST I32 i) (CALL_INDIRECT x) ~> (CALL_ADDR a)
-- if $table(z, 0).ELEM[i] = a
-- if $funcinst(z)[a].CODE = FUNC x' (LOCAL t)* instr*
-- if $type(z, x) = $type(z, x')
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 problematic when the function instance is an imported function.
In that case, x' is a type index for the imported module, but $type(z, x') returns a x'th type in the current module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, yes.


def $instantiate(store, module, externval*) : config
def $instantiate(s, module, externval*) = s'; f; (CALL x')?
def $instantiate(s, module, externval*) = s'''; f; (CALL x')?
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 store should be propagated through initializing elems and datas, thus s''' here.
(Perhaps s_3 will be better?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with changing this to s_N.

-- if f_init = { LOCAL epsilon, MODULE mm_init }
-- if z = s; f_init
-- (Eval_expr : z; expr_G ~>* z; val)*
-- (Eval_expr : z; expr_E ~>* z; (CONST I32 i_E))*
-- (Eval_expr : z; expr_D ~>* z; (CONST I32 i_D))*
-- if (s', mm) = $allocmodule(s, module, externval*, val*)
-- if s'' = $initelem(s', mm.TABLE[0], i_E*, mm.FUNC[x]**)
-- if s''' = $initdata(s'', mm.MEM[0], i_D*, b**)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mm.TABLE[0] (or mm.MEM[0]) may result in out-of-index error, especially if elem* (or data*) is empty. I had to pass the entire module instance mm as an argument to the initelem (or initdata), but perhaps there might be a better way?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That was working as intended: it is an error to define a segment when no memory/table is present, even if the segment's length is 0. (And it's actually a validation error already.)

Copy link
Collaborator Author

@f52985 f52985 Nov 30, 2023

Choose a reason for hiding this comment

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

The problem here is that, the premise
-- if s'' = $initelem(s', mm.TABLE[0], i_E*, mm.FUNC[x]**)
implies the side condition |mm.TABLE| > 0, regardless of whether element is present or not.

In other words, the result of the function $instantiate would not be defined for the given module

(module)

In contrast, in the changed version, the side condition |mm.TABLE| > 0 is implied only if element is present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see! Yeah, I don't know how to do it more elegantly than your change then.

Copy link
Collaborator

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM except for the one comment below.

-- if f_init = { LOCAL epsilon, MODULE mm_init }
-- if z = s; f_init
-- (Eval_expr : z; expr_G ~>* z; val)*
-- (Eval_expr : z; expr_E ~>* z; (CONST I32 i_E))*
-- (Eval_expr : z; expr_D ~>* z; (CONST I32 i_D))*
-- if (s', mm) = $allocmodule(s, module, externval*, val*)
-- if s'' = $initelem(s', mm.TABLE[0], i_E*, mm.FUNC[x]**)
-- if s''' = $initdata(s'', mm.MEM[0], i_D*, b**)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That was working as intended: it is an error to define a segment when no memory/table is present, even if the segment's length is 0. (And it's actually a validation error already.)


def $instantiate(store, module, externval*) : config
def $instantiate(s, module, externval*) = s'; f; (CALL x')?
def $instantiate(s, module, externval*) = s'''; f; (CALL x')?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with changing this to s_N.

@@ -116,8 +116,7 @@ rule Step_read/call:
rule Step_read/call_indirect-call:
z; (CONST I32 i) (CALL_INDIRECT x) ~> (CALL_ADDR a)
-- if $table(z, 0).ELEM[i] = a
-- if $funcinst(z)[a].CODE = FUNC x' (LOCAL t)* instr*
-- if $type(z, x) = $type(z, x')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, yes.

@f52985 f52985 merged commit ec79d67 into main Nov 30, 2023
1 check passed
@f52985 f52985 deleted the main-spec branch November 30, 2023 07:59
Alan-Liang pushed a commit to Alan-Liang/spectec that referenced this pull request Sep 2, 2024
Update readme to link to the two active stack-switching proposals
rossberg pushed a commit that referenced this pull request Nov 7, 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