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

Fix assertion failure in JavascriptArray.cpp #6846

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ShortDevelopment
Copy link
Contributor

@ShortDevelopment ShortDevelopment commented Oct 3, 2022

Fix #6770

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 3, 2022

Please can you add a test case for this?

@ShortDevelopment
Copy link
Contributor Author

Sure, but it seems like I’ve created a new assertion failure; I’ll have another look at this…

@ShortDevelopment
Copy link
Contributor Author

Problem is, that the assertion error only occurred with extremely large arrays.
Using the poc of the mentioned issue causes the test to timeout.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 3, 2022

The timeouts are on a per JS file basis - does it still timeout if this test is done in a separate file with nothing else?

Copy link
Member

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

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

For the timeout, is there a better way to go about the test? The array copy of such size if going to be slow.

lib/Runtime/Library/JavascriptArray.cpp Show resolved Hide resolved
@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 5, 2022

Could the test complete in 3 minutes? Normally each test file gets a 1 min timeout but tests run with the Slow tag get 3 minutes instead (and are limited to only a couple of the test builds though that's not the point here).

If even 3 minutes is too short I don't know what to do - an Assert we can't test seems pointless. @ppenzin thoughts?

@ppenzin
Copy link
Member

ppenzin commented Oct 6, 2022

I agree, we should try to run it as a separate test and mark as slow (given it does finish in 3 minutes).

@ppenzin
Copy link
Member

ppenzin commented Jun 5, 2024

I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Jun 6, 2024

I took a peek at the test - I think the key is separating this test from the reset of ES6 Array API tests by putting it into a separate JS file that would be marked as Slow. I've done that and the testing succeeded, though do we actually run these slow files? Question to @rhuanjl, I think. Both of my runs (debug and release with debug info) produced "exclude: slow" in the output.

You have to use a flag to run slow tests. Currently we do this on two of the windows CI builds x86 test and x64 test.

@ShortDevelopment
Copy link
Contributor Author

ShortDevelopment commented Jun 6, 2024

I should note I never actually checked whether the assertion was just a left-over from a refactoring or a valid check for the following code.

Edit: The fast path uses uint32 as indices, so the asserts have to ensure no overflow occurs.

Copy link
Member

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

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

This looks good in my opinion, though needs a rebase. MSVC CI failure is very likely due to the patch being based on an older version which doesn't have the stringier macro change.

@ShortDevelopment
Copy link
Contributor Author

ShortDevelopment commented Jun 8, 2024

@ppenzin
It seems like the macro change hasn't been merged yet (See #6970).
I gues their checks were failing because of a wrong copyright notice...

@ppenzin
Copy link
Member

ppenzin commented Jun 18, 2024

I guess it is time for mass update to PAL copyright headers...

@ppenzin
Copy link
Member

ppenzin commented Jul 30, 2024

@ShortDevelopment did you catch if these were the new tests failing? Azure link sometimes opens and sometimes doesn't for me

@ShortDevelopment
Copy link
Contributor Author

@ppenzin Yes. Sadly the tests still timeout.
I'm not sure whether I did apply the slow flag correctly...

@ppenzin
Copy link
Member

ppenzin commented Aug 1, 2024

I've tried that on Windows sometime ago, but can't remember how I did it, need to revisit.

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.

Assertion failure in JavascriptArray.cpp
3 participants