[interpreter] Handle custom sections and annotations #17

yuri91 · 2023-03-17T12:48:30Z

This is the work developed in WebAssembly/branch-hinting#19 , but without the branch hinting stuff, and rebased on the current main branch.

I tested the result of the rebase, but there are a couple of things I was unsure about :

in the original unignore branch that I based my changes on, there were some modifications to the Makefile that seem unrelated (?) , like adding quiettest as a PHONY target and some changes related to the smallint test. I noticed those because during the rebase they conflicted with the dune changes.
speaking of dune, I guess I should add the custom tests to the dune tests too?

@rossberg suggestions?

yuri91 · 2023-03-27T08:18:50Z

@rossberg let me know if there is anything left that I should do here.

rossberg · 2023-03-27T11:54:05Z

Hi @yuri91, sorry, I didn't get around to reviewing this yet.

alexp-sssup · 2023-04-12T09:54:56Z

@rossberg Do you have an ETA for the review of this PR? It is our understanding that it will allow the Annotation proposal to move forward to Phase 4.

Our main interest is actually moving branch hinting to Phase 4. We have invested significant effort on this tangential line of work for the purpose of (eventually) satisfying the "testing" requirement, which is the main remaining obstacle.

In case you do not have the opportunity of reviewing the work, is there anybody else that has the authority to do so?

Thanks in advance for your help on the matter.

rossberg · 2023-04-12T10:17:47Z

Sorry about that, I've been in deadline and travel hell for the past weeks, and will remain so until end of next week. After that, I'll try to review it ASAP.

rossberg

Thanks a lot for looking into this, and sorry again for the long delay.

I don't remember where the Makefile changes were coming from, but they look fine.

If you can add the custom tests to the dune tests that would be great.

interpreter/binary/encode.ml

interpreter/binary/decode.ml

interpreter/text/lexer.mll

rossberg · 2023-04-22T10:23:24Z

interpreter/text/parser.mly

+    { let at = at () in
+      (* Hack for empty modules *)
+      let at = if at.left <> at.right then at else
+        {at with right = {at.right with line = Stdlib.Int.max_int}} in


If only I remembered what this was needed for...

tests like this one, with an empty module (just an annotation) fail without those lines:

(module quote "(@a \09)") ;; \t

test/core/annotations.wast

Co-authored-by: Yuri Iozzelli <[email protected]>

yuri91 · 2023-04-24T09:25:07Z

@rossberg I addressed your comments.

About the "hack for empty modules" I am not entirely sure, but I guess that the source info would not play nice with the regular case without it. Before this PR nothing really cares about the source lines except for error messages.

About adding the custom tests to dune: I am not familiar with it, and I am not sure if there is a way to add the required parameters automatically like the Makefile does:

CUSTOMTESTDIR =	../test/custom
CUSTOMTESTDIRS =	$(shell cd $(CUSTOMTESTDIR); ls -d [a-z]*)
CUSTOMTESTFILES =	$(shell cd $(CUSTOMTESTDIR); ls [a-z]*/*.wast)
CUSTOMTESTS =		$(CUSTOMTESTFILES:%.wast=%)
CUSTOMOPTS = -c custom $(CUSTOMTESTDIRS:%=-c %)

Otherwise we need to spell all the tests and enabled sections in the dune file

rossberg

@yuri91, please never force-push to a branch under review. That breaks the Github review history and workflow. E.g., reviewers can no longer see what changed since their last comments.

I think we can ignore the dune test question for now. I'll have a look later.

rossberg · 2023-04-25T08:47:32Z

@yuri91, I merged upstream into main to fix the spec build failure. I then resolved the merge conflicts with main here, but either I made a mistake or something else is causing test annotations.wast to fail now. Can you please have a look?

rossberg · 2023-04-25T08:51:25Z

Hm, in fact, I just saw that annotations.wast is already failing on main in CI. That is really odd, since it works fine locally.

alexp-sssup · 2023-04-25T10:54:36Z

@rossberg Yuri is out-of-office for a couple of weeks, I am available to try and get this PR merged

alexp-sssup · 2023-04-25T11:28:41Z

@rossberg By my analysis the problem appears to be JS codegen issue. In particular code in this pattern is emitted. Please notice the let $m statement being duplicated. This is an error in JavaScript, at least for nodejs. I am unsure if it applies to all versions.

// annotations.wast:89
let $5 = instance("\x00\x61\x73\x6d\x01\x00\x00\x00\x01\x86\x80\x80\x80\x00\x01\x60\x02\x7f\x7d\x00\x02\xd9\x80\x80\x80\x00\x04\x08\x73\x70\x65\x63\x74\x65\x73\x74\x0a\x67\x6c\x6f\x62\x61\x6c\x5f\x69\x33\x32\x03\x7f\x00\x08\x73\x70\x65\x63\x74\x65\x73\x74\x05\x74\x61\x62\x6c\x65\x01\x70\x01\x0a\x14\x08\x73\x70\x65\x63\x74\x65\x73\x74\x06\x6d\x65\x6d\x6f\x72\x79\x02\x01\x01\x02\x08\x73\x70\x65\x63\x74\x65\x73\x74\x0d\x70\x72\x69\x6e\x74\x5f\x69\x33\x32\x5f\x66\x33\x32\x00\x00\x07\x91\x80\x80\x80\x00\x04\x01\x67\x03\x00\x01\x74\x01\x00\x01\x6d\x02\x00\x01\x66\x00\x00");
let $m = $5;

// annotations.wast:120
let $6 = instance("\x00\x61\x73\x6d\x01\x00\x00\x00\x01\x86\x80\x80\x80\x00\x01\x60\x02\x7f\x7d\x00\x02\xd9\x80\x80\x80\x00\x04\x08\x73\x70\x65\x63\x74\x65\x73\x74\x0a\x67\x6c\x6f\x62\x61\x6c\x5f\x69\x33\x32\x03\x7f\x00\x08\x73\x70\x65\x63\x74\x65\x73\x74\x05\x74\x61\x62\x6c\x65\x01\x70\x01\x0a\x14\x08\x73\x70\x65\x63\x74\x65\x73\x74\x06\x6d\x65\x6d\x6f\x72\x79\x02\x01\x01\x02\x08\x73\x70\x65\x63\x74\x65\x73\x74\x0d\x70\x72\x69\x6e\x74\x5f\x69\x33\x32\x5f\x66\x33\x32\x00\x00\x07\x91\x80\x80\x80\x00\x04\x01\x67\x03\x00\x01\x74\x01\x00\x01\x6d\x02\x00\x01\x66\x00\x00");
let $m = $6;

What is the purpose of the $m variables here, which are never used anyway at least for this example?

alexp-sssup · 2023-04-25T12:29:25Z

My analysis above seems confirmed on the CI

rossberg · 2023-04-25T13:42:06Z

Thanks @alexp-sssup, that's indeed the problem. I fixed the test case to avoid duplicate name. I also made that an error in the wast runner, so that it will be rejected in the future.

Now I just need to resolve the latest conflicts on this branch. However, GH UI seems to freeze on me every time I try to view test/core/run.py. :(

alexp-sssup · 2023-04-25T13:45:36Z

@rossberg I suspect the conflict is spurious and due to an unintended indentation change. This diff does not look right.

If that is the case reverting the change should remove the last conflict

Author: Andreas Rossberg <[email protected]>
Date:   Tue Apr 25 14:30:45 2023 +0200

    Avoid duplicate module name in test and improve diagnostics

diff --git a/test/core/run.py b/test/core/run.py
index 62a99c6..fa80ea9 100755
--- a/test/core/run.py
+++ b/test/core/run.py
@@ -64,7 +63,7 @@ class RunTests(unittest.TestCase):
         with open(actualFile) as actual:
           expectText = expect.read()
           actualText = actual.read()
-          self.assertEqual(expectText, actualText)
+      self.assertEqual(expectText, actualText)
 
   def _runTestFile(self, inputPath):
     dir, inputFile = os.path.split(inputPath)

rossberg · 2023-04-25T15:24:56Z

Actually, the indentation change was intended (though not critical). It narrows the life time of the open files.

I still can't resolve the conflict through GH UI. Would need to checkout Yuri's repo locally. Do you happen to have a checkout and can fix it?

rossberg · 2023-04-25T15:25:43Z

PS: I missed this question before:

What is the purpose of the $m variables here, which are never used anyway at least for this example?

They just come directly from the wast file, as the declared module names.

alexp-sssup · 2023-04-25T15:32:11Z

@rossberg The language is alien to me, but I have done my best here: https://github.com/alexp-sssup/annotations/tree/annot_tests_pr_rebased

* [interpreter] Handle custom sections and annotations Co-authored-by: Yuri Iozzelli <[email protected]> * Fix merge conflict * Fix lexer priorities * Fix wast.ml * Oops * Update wast.ml --------- Co-authored-by: Andreas Rossberg <[email protected]>

rossberg reviewed Apr 22, 2023

View reviewed changes

[interpreter] Handle custom sections and annotations

eb670ba

Co-authored-by: Yuri Iozzelli <[email protected]>

yuri91 force-pushed the annot_tests_pr branch from 64db71e to eb670ba Compare April 24, 2023 09:16

rossberg approved these changes Apr 25, 2023

View reviewed changes

Merge branch 'main' into annot_tests_pr

0c77457

rossberg added 6 commits April 25, 2023 17:48

Fix merge conflict

88a6bcb

Merge branch 'main' into annot_tests_pr

c656de9

Fix lexer priorities

45a3e4d

Fix wast.ml

6b72ecb

Oops

e58678f

Update wast.ml

39fc5a4

rossberg merged commit b2fa786 into WebAssembly:main Apr 25, 2023

rossberg mentioned this pull request Apr 25, 2023

No test cases for @custom sections #15

Closed

yuri91 mentioned this pull request Jun 14, 2023

Status and path to Phase 4 #18

Open

yuri91 mentioned this pull request Jun 21, 2023

Update CG-08-15.md - branch hinting WebAssembly/meetings#1304

Merged

[interpreter] Handle custom sections and annotations #17

[interpreter] Handle custom sections and annotations #17

Uh oh!

Conversation

yuri91 commented Mar 17, 2023

Uh oh!

yuri91 commented Mar 27, 2023

Uh oh!

rossberg commented Mar 27, 2023

Uh oh!

alexp-sssup commented Apr 12, 2023

Uh oh!

rossberg commented Apr 12, 2023

Uh oh!

rossberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rossberg Apr 22, 2023

Choose a reason for hiding this comment

Uh oh!

yuri91 Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuri91 commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rossberg left a comment

Choose a reason for hiding this comment

Uh oh!

rossberg commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rossberg commented Apr 25, 2023

Uh oh!

alexp-sssup commented Apr 25, 2023

Uh oh!

alexp-sssup commented Apr 25, 2023

Uh oh!

alexp-sssup commented Apr 25, 2023

Uh oh!

rossberg commented Apr 25, 2023

Uh oh!

alexp-sssup commented Apr 25, 2023

Uh oh!

rossberg commented Apr 25, 2023

Uh oh!

rossberg commented Apr 25, 2023

Uh oh!

alexp-sssup commented Apr 25, 2023

Uh oh!

Uh oh!

yuri91 commented Apr 24, 2023 •

edited

Loading

rossberg commented Apr 25, 2023 •

edited

Loading