[interpreter] Handle custom sections and annotations

[yuri91]

This is the work developed in https://github.com/WebAssembly/branch-hinting/pull/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]

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

[rossberg]

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

[alexp-sssup]

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

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.

[yuri91]

@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, 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]

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]

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

[alexp-sssup]

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

My analysis above seems confirmed on the CI

[rossberg]

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]

@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 <rossberg@mpi-sws.org>
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]

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]

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]

@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