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

Implement EHv4 #2470

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Implement EHv4 #2470

wants to merge 11 commits into from

Conversation

SoniEx2
Copy link
Contributor

@SoniEx2 SoniEx2 commented Sep 22, 2024

This pull request implements EHv4.

@SoniEx2 SoniEx2 marked this pull request as ready for review October 1, 2024 02:12
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 1, 2024

marking this as ready for review even if it's not fully complete, would love to hear feedback.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 1, 2024

@sbc100

@sbc100
Copy link
Member

sbc100 commented Oct 1, 2024

Adding @aheejin too

@@ -87,6 +88,8 @@ class Type {
case Type::V128: return "v128";
case Type::I8: return "i8";
case Type::I16: return "i16";
case Type::ExnRef:
return "exnref";
Copy link
Member

Choose a reason for hiding this comment

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

Use a single line like the surrounding code here and below?

if (!buffer.empty()) {
// remove trailing space
buffer.pop_back();
}
Copy link
Member

Choose a reason for hiding this comment

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

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});
Copy link
Member

Choose a reason for hiding this comment

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

Why did this code move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix for #2476 (you may want us to split this out, along with the fix for #2469 ?). we implemented the fix while trying to figure out how to work the interpreter (unfortunately we still don't know how to implement EHv4 in the interpreter)

Copy link
Member

Choose a reason for hiding this comment

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

Yup, lets split out whatever we can.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Wow, thanks for all the work on this. Very impressive!

Looks good so far.

sbc100 pushed a commit that referenced this pull request Oct 1, 2024
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 )
@SoniEx2 SoniEx2 marked this pull request as draft October 3, 2024 10:35
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 3, 2024

@sbc100 how do you fix the lint task?

@sbc100
Copy link
Member

sbc100 commented Oct 3, 2024

I think you want to add // clang-format off`` / // clang-format on` around those switch statements.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 3, 2024

there we go. good enough. smh.

sigh...

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 3, 2024

we can bump the testsuite later, so we're splitting that out as well...

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 4, 2024

there are a lot of TODOs related to reftypes in the interpreter...

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 5, 2024

we think we figured it out, just need to get #2483 and #2484 merged and then we can get all tests to pass (not that we have enough tests for EHv4 but anyway)

@SoniEx2 SoniEx2 changed the title Add partial support for EHv4 Implement EHv4 Oct 8, 2024
@SoniEx2 SoniEx2 marked this pull request as ready for review October 8, 2024 21:51
@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Oct 8, 2024

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

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.

2 participants