binary/wat: Implement EHv4#2470
Conversation
|
marking this as ready for review even if it's not fully complete, would love to hear feedback. |
|
Adding @aheejin too |
| case Type::I8: return "i8"; | ||
| case Type::I16: return "i16"; | ||
| case Type::ExnRef: | ||
| return "exnref"; |
There was a problem hiding this comment.
Use a single line like the surrounding code here and below?
| if (!buffer.empty()) { | ||
| // remove trailing space | ||
| buffer.pop_back(); | ||
| } |
There was a problem hiding this comment.
I wonder if there is some way to refactor this such to avoid adding and removing the trailing whitespace like this?
| {}, | ||
| {Istream::kInvalidOffset}, | ||
| static_cast<u32>(local_decl_count_), | ||
| 0}); |
There was a problem hiding this comment.
Yup, lets split out whatever we can.
sbc100
left a comment
There was a problem hiding this comment.
Wow, thanks for all the work on this. Very impressive!
Looks good so far.
Fixes the value stack size of the catch handler. There were two (related) issues here: - The previous code used `func_->locals.size()` as soon as the function was available, but it hadn't processed the function's locals yet, so it was always empty. (This might not matter in practice, as it's only used by the "function-wide catch handler", which just rethrows.) - The previous code didn't take the function's locals into account when computing the value stack height (relative to the function frame) for a try-catch block. So, it would drop the locals when catching an exception. Closes #2476 (Split from #2470 )
|
@sbc100 how do you fix the lint task? |
|
I think you want to add |
|
there we go. good enough. smh. sigh... |
|
we can bump the testsuite later, so we're splitting that out as well... |
|
there are a lot of TODOs related to reftypes in the interpreter... |
|
we believe that's all we really need for EHv4, @sbc100 let us know if there's something we missed (or if you have any ideas for how to split this up, because we don't...) |
|
@sbc100 when do you think you can get around to reviewing this? |
|
Do you want to rebase this change and we can see how much of it remains after the split out changes. |
|
It looks this still contains many stuff other than the interpreter. I think parsing and printing can go first before the interpreter and split to another PR. |
|
parsing and printing are tricky because we don't run roundtrip against spec 🙃 |
|
yeeted interp, why not |
|
@sbc100 ptal |
|
@sbc100 ping? |
| Index tag; | ||
| Index depth; | ||
| }; | ||
| using RawCatchVector = std::vector<RawCatch>; |
There was a problem hiding this comment.
But we do we the term "Raw" to mean "in the binary" elsewhere in this file? i.e. what do we do for other types that have binary and wabt IR representations?
|
@sbc100 how is this? |
This pull request implements EHv4. Binary is mostly untested until interp is working.