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 ref.null semantics #478

Merged
merged 6 commits into from
Nov 14, 2023
Merged

Fix ref.null semantics #478

merged 6 commits into from
Nov 14, 2023

Conversation

ShinWonho
Copy link
Contributor

@ShinWonho ShinWonho commented Nov 10, 2023

Problem

Screenshot 2023-11-10 at 5 47 05 PM

This is the semantics of ref.null ht in the current specification.
But I think simply pushing ref.null ht is problematic.

Example

Consider the following example, which is simplified version of "test-sub" in ref_test.wast.

(module
  (type $t0 (sub (struct)))

  (func (export "test-sub")
    ;; must hold
    (ref.test (ref null $t0) (ref.null $t0))
    (drop)))

(assert_return (invoke "test-sub"))
Screenshot 2023-11-10 at 6 08 14 PM

According to the semantics of ref.test, we first need to get the reference type of ref.null $t0.

Screenshot 2023-11-10 at 6 10 50 PM

However, the typeindex $t0 is not ok under empty context.

Suggested change

In the reference interpreter, type index of ref.null $t0 is substituted before pushed.

let rec step (c : config) : config =

      ....

      | RefNull t, vs' ->
        Ref (NullRef (subst_heap_type (subst_of c.frame.inst) t)) :: vs', []

Therefore, I fixed the semantics according to the reference interpreter.

Screenshot 2023-11-10 at 5 46 24 PM

Copy link
Member

@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.

Ah, good catch! This is rather annoying, if only ref.null didn't have this dreaded type annotation...

Unfortunately, I think the fix isn't quite as easy as that. Because (ref.null ht) is defined to be a value, and values must never reduce, otherwise the Progress statement is compromised.

So we need to syntactically distinguish reduced null refs from non-reduced ones. We could do it based on the shape of the ht, which is probably fine for now (you'll need to add a side condition to the grammar of reference values), but is a bit hacky and not very robust.

Long-term, I think the cleaner fix is to make the type annotation optional. Then the reduction rule would remove it. And the typing rule for valid-ref would only allow unannotated ref.null and assign it type (ref null bot).

But changing the syntax of reference values like that requires more adjustments throughout the spec, and even the JS API spec. The interpreter should be adjusted as well then. That's quite a bit of work, so perhaps for later.

@zapashcanon
Copy link
Contributor

zapashcanon commented Nov 10, 2023

Because (ref.null ht) is defined to be a value, and values must never reduce, otherwise the Progress statement is compromised.

Wouldn't having null.new $t to be an instruction and ref.null $t a value fix the issue ?

@rossberg
Copy link
Member

See my reply on the other issue. Also, ref.null is already standardised as part of Wasm 2.0 (as is ref.func, btw), so we cannot change it anymore.

@ShinWonho
Copy link
Contributor Author

Screenshot 2023-11-13 at 5 33 26 PM Screenshot 2023-11-13 at 5 33 56 PM

I changed the syntax of ref.null value so that ref.null with type index cannot be a value.
Is this right fix?

@@ -47,7 +47,7 @@ Any of the aformentioned references can furthermore be wrapped up as an *externa
\production{vector} & \vecc &::=&
\V128.\CONST~\i128 \\
\production{reference} & \reff &::=&
\REFNULL~t \\&&|&
\REFNULL~t & (\iff t \neq \typeidx) \\&&|&
Copy link
Member

Choose a reason for hiding this comment

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

This is correct in spirit, but technically, since meta variables are existentially quantified, a negation like this doesn't do the right thing – you can always take a different type index to make it true. I think you need to say some like "t = absheaptype ∨ t = deftype". Or perhaps spell it out as \REFNULL~\absheaptype~|~\REFNULL~\deftype.

Copy link
Contributor Author

@ShinWonho ShinWonho Nov 13, 2023

Choose a reason for hiding this comment

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

Screenshot 2023-11-13 at 6 34 43 PM

Thank you for the comment! I fixed this.


.. note::
No formal reduction rule is required for this instruction, since the |REFNULL| instruction is already a :ref:`value <syntax-val>`.
Copy link
Member

Choose a reason for hiding this comment

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

I think a variation of this note still applies, since we only reduce some ref.null's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2023-11-14 at 8 13 01 PM I added a note like this. If you think there are better words, feel free to change it.

document/core/exec/runtime.rst Outdated Show resolved Hide resolved
ShinWonho and others added 2 commits November 14, 2023 19:51
Co-authored-by: Andreas Rossberg <rossberg@mpi-sws.org>
Copy link
Member

@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!

@rossberg rossberg merged commit 026ffea into WebAssembly:main Nov 14, 2023
3 checks passed
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.

3 participants