Complete the support for try_table. #6814

sjrd · 2024-08-06T16:10:58Z

(Motivation: the Scala.js Wasm backend emits try_tables, and we would like to pipe our output to wasm-opt.)

This includes adding interpreter support for exnref values. Since they must carry a Name tag in addition to Literals payload, they did not fit into existing GCData. Therefore, we add another possible type in the big union for ExnData.

Alternatively, we could make a non-funcref Name value a valid choice for a single Literal, but that would require inventing a fake Type for such literals, such as Type::tag, which would have no spec equivalent.

Other than the above, the changes are straightforward. They are inspired from existing handling of Try and Break.

Currently WiP:

The new tests in local-subtyping.wast fail. Some pass somewhere is refining the types of blocks that are the target of catch clauses in an incorrect way. The obvious candidates--finalize, operateOnScopeNameUsesAndSentTypes--seem to be doing the right thing, so I am missing something somewhere.
More existing tests for legacy eh should be translated to the new eh. Is the approach taken for the tests translated so far appropriate?

I would appreciate some guidance about the above questions. ~~Can someone point me in the right direction for the first one, in particular?~~

Also, is the approach taken for ExnData acceptable? Should I do it differently?

Thanks in advance

kripken

Thanks for the PR!

Some initial comments on the code. I haven't read the tests yet.

src/literal.h

src/wasm/literal.cpp

kripken · 2024-08-06T17:06:53Z

Aside from those comments the code looks right to me - nice work!

sjrd · 2024-08-07T09:17:32Z

Ah ah! I found the issue with the refined block types. There was some code missing from ReFinalize::visitTryTable. All the existing tests now pass locally.

kripken

Nice work, looks good!

Let's also get feedback from @aheejin before landing.

@tlively may also want to look at the spec test here.

test/lit/passes/dce-eh.wast

test/lit/passes/local-subtyping.wast

test/spec/return_call_eh-legacy.wast

kripken · 2024-08-07T22:02:38Z

Oh, one thing to consider here is that any new tests that are added get quickly looked at by our fuzzer. Until we are certain the fuzzer is ready for try_table, please add the new tests here:

binaryen/scripts/fuzz_opt.py

Lines 355 to 358 in fb6ead8

    
           # New EH implementation is in progress 
        
           'exception-handling.wast', 
        
           'translate-to-new-eh.wast', 
        
           'rse-eh.wast',

(You can also run that script locally (instructions at the top) to see if it finds anything, to check if that is needed.)

tlively

Spec tests LGTM, thanks!

This includes adding interpreter support for `exnref` values. Since they must carry a `Name tag` in addition to `Literals payload`, they did not fit into existing `GCData`. Therefore, we add another possible type in the big `union` for `ExnData`. Alternatively, we could make a non-`funcref` `Name` value a valid choice for a single `Literal`, but that would require inventing a fake `Type` for such literals, such as `Type::tag`, which would have no spec equivalent. This highlighted the fact that using `Name`s will not work when cross module boundaries (for `funcref` nor `exnref` nor the thrown `WasmException`s). Since fixing this is beyond the scope of this commit, we leave TODO's in place. Other than the above, the changes are straightforward. They are inspired from existing handling of `Try` and `Break`.

sjrd · 2024-08-08T16:18:59Z

Thank you for the reviews. I believe I have addressed all the points that were raised.

I also completed the translation of -eh-legacy lit tests to the new EH under -eh tests. That uncovered one incorrect optimization in CodeFolding, and two missed opportunities in CodePushing and Vacuum. If fixed those in the process.

I don't consider this WiP anymore. If you're happy with it, I think it's complete.

To be fully transparent, there are two things that were not done here:

There is a TODO in Vacuum.cpp for an optimization that we do for try but not yet for try_catch. I left the translation of the tests for it in as "progression" tests, so it will be easy to adapt if/when we add that optimization.

Possibly more important: I discovered try_table is still not supported in Flatten.cpp. I did not address this. I don't think it's possible to address it and keep the restrictions on the target IR as described in ir/flat.h. The problem is that the target blocks for the catches must necessarily have result types to capture the exceptions' payloads and/or the exnrefs. There is no way to "configure" the try_table to store these in locals instead. I'm not sure how important the Flat IR is? This missing piece was not triggered when I applied wasm-opt to the Scala.js test suite, so I am not directly affected, but perhaps others will be?

sjrd · 2024-08-08T16:22:19Z

(You can also run that script locally (instructions at the top) to see if it finds anything, to check if that is needed.)

Looks like I found an issue but with f32.store_f16 instead 😄 . I'll see how I'm supposed to report it tomorrow or later this weekend.

kripken · 2024-08-08T17:15:10Z

@sjrd

I discovered try_table is still not supported in Flatten.cpp

Yeah, that is a larger issue that we need to think about. I agree with your analysis. We can land this without that.

Looks like I found an issue but with f32.store_f16 instead 😄 . I'll see how I'm supposed to report it tomorrow or later this weekend.

Ah, no need: that was a recent landing that has since been fixed by ignoring f16 in the fuzzer in #6822. Merging latest main should avoid it for you.

kripken

Nice work! Here are a few minor comments on the code. I'll look at the tests next.

Btw, please don't squash and force-push, if possible: This is a large PR, and seeing incremental small commits makes it easier to do incremental reviews. We will squash all the commits when landing anyhow.

scripts/fuzz_opt.py

src/ir/effects.h

src/passes/CodeFolding.cpp

src/passes/Vacuum.cpp

kripken · 2024-08-08T18:28:56Z

test/lit/passes/coalesce-locals-eh.wast

+
+ (tag $e)
+
+ (tag $any (param (ref any)))


Indentation here looks different than the function before. Also, please put the tags first, before functions.

test/lit/passes/code-folding-eh.wast

sjrd · 2024-08-09T08:12:37Z

Btw, please don't squash and force-push, if possible: This is a large PR, and seeing incremental small commits makes it easier to do incremental reviews. We will squash all the commits when landing anyhow.

Oops, sorry. Different habits in different repos. I should have asked first.

This time I added a commit addressing the latest batch of comments.

test/lit/passes/vacuum-eh.wast

kripken · 2024-08-12T20:58:22Z

No worries, yeah, there isn't really a standard way of doing these things on github...

Just one last minor comment from me. Otherwise, looks good, except for @aheejin taking a look, and also @sjrd please see that at least a few thousand fuzzer iterations pass without issue here.

aheejin

Thank you for the work! Haven't checked the tests yet, but the comments for the code part:

aheejin · 2024-08-12T22:56:40Z

src/ir/linear-execution.h

+      case Expression::Id::TryTableId: {
+        self->pushTask(SubType::doVisitTryTable, currp);
+        self->pushTask(SubType::doNoteNonLinear, currp);
+        break;
+      }


Don't we need to push subtasks for the try_table's body?

Ah yes, you're right. Good catch.

Does any test cover this?

Now yes: simplify-locals-eh.wast covers this.

src/passes/CodeFolding.cpp

src/passes/Vacuum.cpp

src/wasm-type.h

aheejin · 2024-08-13T07:59:19Z

src/wasm-interpreter.h

+            }
+          }
+          if (curr->catchRefs[i]) {
+            ret.values.push_back(self()->makeExnData(e.tag, e.values));


How about calling makeExnData in visitThrow and just reuse it when catching/rethrowing?

I did that in the branch that changes WasmException but in which I'm stuck:
sjrd/binaryen@full-support-try-table...sjrd:binaryen:exnref-in-wasmexception
I don't think I can apply this particular suggestion without changing WasmException, can I?

aheejin · 2024-08-13T08:04:06Z

src/wasm-interpreter.h

 struct WasmException {
+  // TODO: handle cross-module calls using something other than a Name here.
  Name tag;
  Literals values;
 };


Now that we have exnref, this can simply be

struct WasmException { WasmException(Literal exn) : exn(exn) {} Literal exn; };

I tried to something like that, but I'm running into issues that I have a hard time troubleshooting. I get the following errors when running the spec tests:

executing: /localhome/doeraene/projects/binaryen/bin/wasm-shell /localhome/doeraene/projects/binaryen/test/spec/exception-handling-legacy.wast ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n') Traceback (most recent call last): File "/localhome/doeraene/projects/binaryen/./check.py", line 214, in run_spec_tests actual = run_spec_test(wast) File "/localhome/doeraene/projects/binaryen/./check.py", line 190, in run_spec_test output = support.run_command(cmd, stderr=subprocess.PIPE) File "/localhome/doeraene/projects/binaryen/scripts/test/support.py", line 193, in run_command raise Exception(('run_command failed (%s)' % code, out + str(err or ''))) Exception: ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n') During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/localhome/doeraene/projects/binaryen/./check.py", line 408, in <module> sys.exit(main()) File "/localhome/doeraene/projects/binaryen/./check.py", line 391, in main TEST_SUITES[test]() File "/localhome/doeraene/projects/binaryen/./check.py", line 220, in run_spec_tests shared.fail_with_error(str(e)) File "/localhome/doeraene/projects/binaryen/scripts/test/shared.py", line 334, in fail_with_error raise Exception(msg) Exception: ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n')

I pushed my attempt so far at sjrd/binaryen@full-support-try-table...sjrd:binaryen:exnref-in-wasmexception

@sjrd You can run individual spec tests using e.g.

bin/wasm-shell test/spec/exception-handling-legacy.wast

I'm attending some seminar Tue-Wed, so I can't take a look now, but will take a look soon, unless it's solved by then. But I think this can be done by a follow-up too.

@sjrd You can run individual spec tests using e.g.

bin/wasm-shell test/spec/exception-handling-legacy.wast

Thanks, that helps a bit. At least now I know I'm running into a segfault during catch handling inside visitTry. It's a bit of a heisenbug: depending on how much printing I do, I get further or not. I think I'm out of my depth in terms of understanding how C++ handles exception structs that have a destructor. 😕

No worries @sjrd . Let's leave this part to a followup then, as @aheejin suggested. It does sound useful but it may not be trivial to do, and is not urgent IIUC.

(To be clear, I am not saying you should do the followup - could be anyone.)

aheejin · 2024-08-13T08:07:34Z

src/wasm-interpreter.h

+    WasmException exn;
+    exn.tag = exnData->tag;
+    for (auto item : exnData->payload) {
+      exn.values.push_back(item);
+    }
+    throwException(exn);


Suggested change

WasmException exn;

exn.tag = exnData->tag;

for (auto item : exnData->payload) {

exn.values.push_back(item);

}

throwException(exn);

throwException(exnData);

This can simplified if we change WasmException to take ExnData literal. See my comment on line 49-53.

aheejin

Haven't read the tests yet, but I wonder why the old -legacy tests have changed or deleted?

test/lit/passes/coalesce-locals-eh-legacy.wast

test/lit/passes/code-pushing-eh-legacy.wast

* Do not combine with `--flatten`. * Requires reference types for the `exnref` type.

sjrd

Thanks @aheejin for the review. I addressed everything except the change to WasmException. I haven't managed to make it work yet. I left a comment with a link to my attempts.

sjrd · 2024-08-13T09:37:07Z

src/ir/linear-execution.h

+      case Expression::Id::TryTableId: {
+        self->pushTask(SubType::doVisitTryTable, currp);
+        self->pushTask(SubType::doNoteNonLinear, currp);
+        break;
+      }


Ah yes, you're right. Good catch.

src/passes/Vacuum.cpp

src/wasm-type.h

test/lit/passes/coalesce-locals-eh-legacy.wast

test/lit/passes/code-pushing-eh-legacy.wast

sjrd · 2024-08-13T11:07:59Z

src/wasm-interpreter.h

 struct WasmException {
+  // TODO: handle cross-module calls using something other than a Name here.
  Name tag;
  Literals values;
 };


I tried to something like that, but I'm running into issues that I have a hard time troubleshooting. I get the following errors when running the spec tests:

executing: /localhome/doeraene/projects/binaryen/bin/wasm-shell /localhome/doeraene/projects/binaryen/test/spec/exception-handling-legacy.wast ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n') Traceback (most recent call last): File "/localhome/doeraene/projects/binaryen/./check.py", line 214, in run_spec_tests actual = run_spec_test(wast) File "/localhome/doeraene/projects/binaryen/./check.py", line 190, in run_spec_test output = support.run_command(cmd, stderr=subprocess.PIPE) File "/localhome/doeraene/projects/binaryen/scripts/test/support.py", line 193, in run_command raise Exception(('run_command failed (%s)' % code, out + str(err or ''))) Exception: ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n') During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/localhome/doeraene/projects/binaryen/./check.py", line 408, in <module> sys.exit(main()) File "/localhome/doeraene/projects/binaryen/./check.py", line 391, in main TEST_SUITES[test]() File "/localhome/doeraene/projects/binaryen/./check.py", line 220, in run_spec_tests shared.fail_with_error(str(e)) File "/localhome/doeraene/projects/binaryen/scripts/test/shared.py", line 334, in fail_with_error raise Exception(msg) Exception: ('run_command failed (-11)', '0 BUILDING MODULE [line: 1]\n1 CHECKING [line: 288]\n----------------------------------------\nCAUGHT EXCEPTION\n2 CHECKING [line: 289]\n----------------------------------------\nCAUGHT EXCEPTION\n3 CHECKING [line: 290]\n4 CHECKING [line: 291]\n')

I pushed my attempt so far at sjrd/binaryen@full-support-try-table...sjrd:binaryen:exnref-in-wasmexception

sjrd · 2024-08-13T13:28:01Z

please see that at least a few thousand fuzzer iterations pass without issue here.

I let it run for more than 12,000 iterations without issue.

scripts/fuzz_opt.py

src/passes/Vacuum.cpp

The previous implementation caused segmentation faults when exnrefs were manipulated a bit too much.

src/wasm/literal.cpp

sjrd · 2024-08-14T18:30:09Z

I also completed the translation of -eh-legacy lit tests to the new EH under -eh tests. That uncovered one incorrect optimization in CodeFolding, and two missed opportunities in CodePushing and Vacuum. If fixed those in the process.

Are the incorrect optimization and and missed opportunities about the old EH or the new EH? The new EH's optimizations have not really been implemented anyway prior to this PR so I guess they are about the old EH, but I can't find anything in the PR that fixes them. Can you elaborate?

Oh sorry for the confusion. This was about the new EH, as implemented in an early state of this PR. Porting more tests from old EH to new EH highlighted things I had not yet taken care of in the implementation. This comment is obsolete by now.

src/ir/possible-contents.h

aheejin

Mostly test names and comments nits. I think you can "Add suggestion to batch" on the ones you'd like to accept and batch-commit them, without manually fixing them.

test/lit/passes/coalesce-locals-eh.wast

test/lit/passes/code-folding-eh.wast

test/lit/exec/eh.wast

test/spec/exception-handling.wast

Co-authored-by: Heejin Ahn <[email protected]>

sjrd · 2024-08-15T09:08:54Z

Overnight I ran the fuzzer while allowing it to use the new EH sources as inputs. I did not discover any issue within wasm-opt, but I think I found a bug in the V8 validator (after ~6,000 iterations). It refuses to validate the following module:

(module
 (type $0 (func))
 (func $0
  (throw_ref
   (ref.null noexn)
  )
 )
)

with the error

/localhome/doeraene/projects/binaryen/scripts/fuzz_shell.js:155: CompileError: WebAssembly.Module(): Compiling function #0 failed: invalid type for throw_ref: expected exnref, found noexnref @+25
var module = new WebAssembly.Module(binary);
             ^
CompileError: WebAssembly.Module(): Compiling function #0 failed: invalid type for throw_ref: expected exnref, found noexnref @+25
    at /localhome/doeraene/projects/binaryen/scripts/fuzz_shell.js:155:14

I don't think this is correct, because noexnref is a subtype of exnref. It should allow to validate the throw_ref (even if it will inevitably trap at run-time).

It looks similar to https://issues.chromium.org/issues/332931390 which I had already reported some time ago.

kripken · 2024-08-15T19:04:29Z

invalid type for throw_ref: expected exnref, found noexnref @+25

That does look like a bug. Until v8 fixes it, we'll need to disable fuzzing of exnref. That means disabling all exceptions testing in v8 since we don't have separate feature flags, which is somewhat unfortunate, but we will still be fuzzing it in our interpreter so it's not too bad.

To disable it in V8 only, see

binaryen/scripts/fuzz_opt.py

Lines 840 to 844 in d567578

    
           def can_run(self, wasm): 
        
               # V8 does not support shared memories when running with 
        
               # shared-everything enabled, so do not fuzz shared-everything 
        
               # for now. 
        
               return all_disallowed(['shared-everything'])

kripken · 2024-08-15T20:05:02Z

src/ir/possible-contents.h

@@ -473,6 +473,12 @@ struct TagLocation {
  }
 };

+// The location of an exnref materialized by a catch_ref or catch_all_ref clause
+// of a try_table.


Suggested change

// of a try_table.

// of a try_table. No data is stored here (exnrefs contain a stack at runtime, but we don't track that), so this is the same as NullLocation in a way: we just need *a* source of contents for places that receive an exnref.

It's been a while since I read GUFA code so my understanding is bad, but why are all exnrefs treated the same? All nulls are actually the same value and can be used interchangeably, but each exnref contains a different payload and tag, no?

Isn't the payload separate from the exnref? When we catch with an exnref we jump to a block that gets the payload as multivalues + an exnref at the end IIUC.

GUFA doesn't do any abstract interpretation, AFAICT. It only tracks specific literals, and unknown things of certain types. So all unknown (ref exn) are the same anyway.

exnref conceptually contains the payload, even though it is opaque, no? I think we can treat it in the same way we treat externref or anyref, given that they also contain something opaque. How do we treat them in GUFA? Do we treat all of them as the same literal?

In the new EH (exnref) scheme, when we rethrow an exnref using throw_ref, we rethrow the whole package, including a tag and a payload. So exnref contains both.

Oh, I didn't realize that. So when we catch, we are sending the payload as values, and also sending it wrapped in the exnref object (which also has the stack and tag id)? So we send it both wrapped and unwrapped?

No I think, at least conceptually (because the actual implementation in the engines can differ), we just send a package (=exnref). And when caught, depending on the kind of catch clauses, we provide the exnref as is or extract values from it and provide the values.

I think we can treat it in the same way we treat externref or anyref, given that they also contain something opaque.

Makes sense, yeah. That is basically what the code does. E.g. if an import returns an anyref, then we know nothing about it but the type, so we fill that ResultLocation (result of a function) with addRoot, which just sets it to an unknown value of that type. The code here does the same, but we do need a Location to read the exnref from (it isn't a ResultLocation or anything else we have, yet), so we add that as well.

Still not sure why we need a separate location kind, given that externref/anyref doesn't have any. exnref here is just a return value from a block, so it can be treated as such, no? Do we need a separate Location kind for every kind of block return value? Or it can be treated the same way as a return value of a br, because it sends the info as the block return value.

The issue is, where does the block return value receive the value from? anyref arrives from e.g. a function result, which we already have a Location type for. But here we are not just adding a new type but also a new control flow construct that sends that value.

But maybe we could instead add a root on the target of the branch, though, is that what you mean? I suppose there is just one such target, and we know exactly what it is. I think that would be safe and correct to do here (that location can't be marked as a root for any other reason), but it does seem a little cleaner to me to keep the code as it is, since the branch target is not conceptually a "root" (e.g. it may receive values from other branches than this, so it needs to combine them all. We do know that this unknown value will "win", but conceptually we are still combining there).

Hmm I see. Then I guess even though all exnrefs are treated as the same constant it wouldn't matter in GUFA because it doesn't do anything with constants, right?

Yeah, we just move constants around, but don't do anything with the values otherwise. So this should be ok as is, I think.

Hmm I see. Then I guess even though all exnrefs are treated as the same constant it wouldn't matter in GUFA because it doesn't do anything with constants, right?

They're not treated like the same constant. They're all treated as some unknown value of type (ref exn). GUFA makes a difference between actual constants and unknown values of some type. If we did register them as the same constant, that would be bad, as GUFA would then propagate that constant at all destinations.

src/ir/possible-contents.h

src/ir/possible-contents.cpp

test/lit/passes/gufa-eh.wast

test/lit/passes/local-subtyping.wast

test/lit/passes/vacuum-eh.wast

test/spec/exception-handling.wast

sjrd · 2024-08-16T15:42:29Z

I won't have a computer until Tuesday, do I'll come back to this then.

For the conflicts, should I resolve them by merging main into this branch, or rebasing on top of main?

kripken · 2024-08-16T15:55:45Z

@sjrd Merging would be better, thanks.

sjrd · 2024-08-20T11:35:08Z

With the latest changes, and with disabling the following lines from the fuzzer (see comments here):

binaryen/src/tools/fuzzing/fuzzing.cpp

Lines 3304 to 3310 in 2c9c74d

    
           EqVecF16x8, 
        
           EqVecF16x8, 
        
           NeVecF16x8, 
        
           LtVecF16x8, 
        
           GtVecF16x8, 
        
           LeVecF16x8, 
        
           GeVecF16x8,

I could successfully run 20,000+ iterations of the fuzzer on this PR, when also allowing the fuzzer to take the new EH tests as inputs.

@kripken Do you think that's good enough to remove the following lines:
https://github.com/WebAssembly/binaryen/pull/6814/files#diff-3c641dc74f54559f1df2b85afcac1c8e199a33edfbbf80a481167ad2e1b01f90L355-R372
?

kripken · 2024-08-20T20:02:53Z

I could successfully run 20,000+ iterations of the fuzzer on this PR, when also allowing the fuzzer to take the new EH tests as inputs. Do you think that's good enough to remove the following lines

Nice! Yes, that seems enough. Likely the fuzzer will eventually find more issues but so long as there is nothing known and high-frequency that fails, that's what we want.

kripken

lgtm % any remaining questions @aheejin has, and to enable the fuzzer as just discussed.

sjrd · 2024-08-20T20:55:36Z

Thanks. Fuzzer enabled.

aheejin

Thank you for the work!

kripken · 2024-08-20T22:40:09Z

Thanks from me as well @sjrd , this is great!

kripken requested a review from aheejin August 6, 2024 16:41

kripken reviewed Aug 6, 2024

View reviewed changes

src/literal.h Show resolved Hide resolved

src/wasm/literal.cpp Outdated Show resolved Hide resolved

sjrd force-pushed the full-support-try-table branch 2 times, most recently from 0b235e4 to f38aa74 Compare August 7, 2024 09:15

kripken approved these changes Aug 7, 2024

View reviewed changes

kripken mentioned this pull request Aug 7, 2024

Support cross-module calls properly in the interpreter #6823

Open

tlively reviewed Aug 8, 2024

View reviewed changes

sjrd force-pushed the full-support-try-table branch from f38aa74 to 0f0f5a5 Compare August 8, 2024 16:10

sjrd changed the title ~~WiP: Complete the support for try_table. (asking for some guidance)~~ Complete the support for try_table. Aug 8, 2024

kripken reviewed Aug 8, 2024

View reviewed changes

scripts/fuzz_opt.py Outdated Show resolved Hide resolved

src/ir/effects.h Outdated Show resolved Hide resolved

src/ir/effects.h Outdated Show resolved Hide resolved

src/passes/CodeFolding.cpp Show resolved Hide resolved

src/passes/Vacuum.cpp Outdated Show resolved Hide resolved

kripken reviewed Aug 8, 2024

View reviewed changes

Address review comments.

fd6eda4

kripken reviewed Aug 12, 2024

View reviewed changes

test/lit/passes/vacuum-eh.wast Outdated Show resolved Hide resolved

aheejin reviewed Aug 13, 2024

View reviewed changes

test/lit/passes/coalesce-locals-eh-legacy.wast Show resolved Hide resolved

test/lit/passes/code-pushing-eh-legacy.wast Show resolved Hide resolved

sjrd added 3 commits August 13, 2024 11:26

Adapt the fuzz script not to generate exnref-incompatible option sets.

4f07f18

* Do not combine with `--flatten`. * Requires reference types for the `exnref` type.

Correctly connect tags and exnrefs in the GUFA analysis.

6868ba1

Address aheejin's review comments, except the WasmException changes.

cb49cbb

sjrd commented Aug 13, 2024

View reviewed changes

aheejin reviewed Aug 14, 2024

View reviewed changes

scripts/fuzz_opt.py Show resolved Hide resolved

src/passes/Vacuum.cpp Outdated Show resolved Hide resolved

sjrd added 2 commits August 14, 2024 17:36

Fix copy construction of (ref exn) literals.

70f36df

The previous implementation caused segmentation faults when exnrefs were manipulated a bit too much.

Store a literal exnref in WasmException.

4c752b3

sjrd commented Aug 14, 2024

View reviewed changes

src/wasm/literal.cpp Outdated Show resolved Hide resolved

kripken mentioned this pull request Aug 14, 2024

[NFC] Clean up Literal copy constructor #6841

Merged

kripken reviewed Aug 14, 2024

View reviewed changes

src/ir/possible-contents.h Show resolved Hide resolved

aheejin reviewed Aug 15, 2024

View reviewed changes

sjrd and others added 3 commits August 15, 2024 10:33

Apply suggestions from code review

202bc17

Co-authored-by: Heejin Ahn <[email protected]>

Re-update lit checks after test name changes.

7c14456

Address further review comments in the tests (more comments).

515b6ae

kripken reviewed Aug 15, 2024

View reviewed changes

src/ir/possible-contents.h Outdated Show resolved Hide resolved

kripken reviewed Aug 15, 2024

View reviewed changes

src/ir/possible-contents.cpp Outdated Show resolved Hide resolved

aheejin reviewed Aug 16, 2024

View reviewed changes

test/lit/passes/gufa-eh.wast Show resolved Hide resolved

test/lit/passes/local-subtyping.wast Outdated Show resolved Hide resolved

test/lit/passes/vacuum-eh.wast Outdated Show resolved Hide resolved

test/spec/exception-handling.wast Outdated Show resolved Hide resolved

sjrd added 2 commits August 20, 2024 10:06

Merge branch 'main' into full-support-try-table

40da584

Address review comments.

d200d06

kripken approved these changes Aug 20, 2024

View reviewed changes

Enable the fuzzer on tests with new EH.

25d5af9

aheejin approved these changes Aug 20, 2024

View reviewed changes

kripken merged commit 340ad71 into WebAssembly:main Aug 20, 2024
13 checks passed

sjrd deleted the full-support-try-table branch August 20, 2024 23:31

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

	// of a try_table.
	// of a try_table. No data is stored here (exnrefs contain a stack at runtime, but we don't track that), so this is the same as NullLocation in a way: we just need a source of contents for places that receive an exnref.

Complete the support for try_table. #6814

Complete the support for try_table. #6814

Conversation

sjrd commented Aug 6, 2024 • edited Loading

kripken left a comment

Choose a reason for hiding this comment

kripken commented Aug 6, 2024

sjrd commented Aug 7, 2024

kripken left a comment

Choose a reason for hiding this comment

kripken commented Aug 7, 2024

tlively left a comment

Choose a reason for hiding this comment

sjrd commented Aug 8, 2024

sjrd commented Aug 8, 2024

kripken commented Aug 8, 2024

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Aug 9, 2024

kripken commented Aug 12, 2024

aheejin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin left a comment

Choose a reason for hiding this comment

sjrd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Aug 13, 2024

sjrd commented Aug 14, 2024 • edited Loading

aheejin left a comment

Choose a reason for hiding this comment

sjrd commented Aug 15, 2024

kripken commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aheejin Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd commented Aug 16, 2024

kripken commented Aug 16, 2024

sjrd commented Aug 20, 2024

kripken commented Aug 20, 2024

kripken left a comment

Choose a reason for hiding this comment

sjrd commented Aug 20, 2024

aheejin left a comment

Choose a reason for hiding this comment

kripken commented Aug 20, 2024

sjrd commented Aug 6, 2024 •

edited

Loading

sjrd commented Aug 14, 2024 •

edited

Loading

aheejin Aug 16, 2024 •

edited

Loading