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

feat: add finally return support #60

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

jameslahm
Copy link
Member

@jameslahm jameslahm commented Jun 30, 2021

Fix #7

@playXE
Copy link
Collaborator

playXE commented Jun 30, 2021

Oh wow! That's nice work! But seems like there's some regression as now half of previously passing test262 tests do not pass:

Results:
Total tests: 78695
Passed tests: 6187
Ignored tests: 24233
Failed tests: 48275 (panics: 1497)
Conformance: 7.86%

dev branch:

Results:
Total tests: 78695
Passed tests: 12579
Ignored tests: 24233
Failed tests: 41883 (panics: 1244)
Conformance: 15.98%

EDIT: You can see how much tests pass in Actions

@playXE playXE marked this pull request as draft June 30, 2021 08:45
@jameslahm
Copy link
Member Author

Hi, I encountered a segment fault error when I run sl examples/hello-world.js without .startupshot. I feel like there has some problem in add_conservative in gc.rs. Do you have this problem?

@jameslahm
Copy link
Member Author

It seems that I found it. I will push and try to run test262.

@playXE playXE marked this pull request as ready for review June 30, 2021 11:48
@playXE
Copy link
Collaborator

playXE commented Jun 30, 2021

Hi, I encountered a segment fault error when I run sl examples/hello-world.js without .startupshot. I feel like there has some problem in add_conservative in gc.rs. Do you have this problem?

Unfortunately, I will not have access to PC for one or two days so Idk where the problem is. I actually tested the new GC when I was merging it and hello-world was working fine. Can you please check that you run the latest dev branch GC?

@jameslahm
Copy link
Member Author

jameslahm commented Jun 30, 2021

The problem should be here. And I have added the index check. It works fine now.

pub fn test(&self, object: *const u8) -> bool {
let object = object as usize;
let offset = object as isize - self.heap_begin as isize;
let index = Self::offset_to_index(offset as _);
let mask = Self::offset_to_mask(offset as _);
if index >= self.bitmap_size / size_of::<usize>() {
return false;
}
let atomic_entry = unsafe { *self.bitmap_begin.add(index) };
(atomic_entry & mask) != 0

EDIT: I have checked that I am on the latest dev branch

@jameslahm
Copy link
Member Author

And I continue to look at the problem of regression testing 🤣

@jameslahm jameslahm marked this pull request as draft July 2, 2021 03:07
@playXE playXE closed this Jul 9, 2021
@playXE playXE reopened this Jul 9, 2021
@jameslahm jameslahm mentioned this pull request Jul 21, 2021
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.

Support for "unsafe" cases of finally
2 participants