ykjit / yklua

yk-enabled Lua interpreter
4 stars 4 forks source link

Add test coroutine #70

Closed Pavel-Durov closed 11 months ago

Pavel-Durov commented 1 year ago

Related Issue: https://github.com/ykjit/yklua/issues/58

Pavel-Durov commented 1 year ago

We still can't run CI validation with the latest YK Failed buildbot job - https://ci.soft-dev.org/#/builders/1/builds/2527

vext01 commented 1 year ago

The issue is still open, so how comes we are re-enabling this test?

Pavel-Durov commented 1 year ago

The issue is still open because I don't want to close it before I can validate tests with buildbot. I've tested coroutine.lua myself and it works with try_repeat 1000.

vext01 commented 1 year ago

Was there any change that you'd expect to have fixed that test?

Pavel-Durov commented 1 year ago

Yes, this commit: https://github.com/ykjit/yklua/commit/b931338cb3a2960a7047fe564cdc52e1738e7d7e

vext01 commented 1 year ago

bors r+

bors[bot] commented 1 year ago

Build failed:

vext01 commented 1 year ago

What should we do with this?

Pavel-Durov commented 1 year ago

good question :) I don't think we can merge any PRs in yklua since breaking changes in YK. Last working version: 4a955668d5a6647c5de8f043314983f057a5039e (Sep 15, 2023)

Pavel-Durov commented 1 year ago

bors try

bors[bot] commented 1 year ago

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

Pavel-Durov commented 1 year ago

Bors CI succeeded I guess we can merge it then!? @vext01

vext01 commented 1 year ago

I think so.

Any squashing to do?

Pavel-Durov commented 1 year ago

No squashing needed :)

vext01 commented 1 year ago

Great!

bors r+

bors[bot] commented 1 year ago

Build failed:

vext01 commented 1 year ago
16:19:38 thread '<unnamed>' panicked at ykrt/src/mt.rs:469:21:
16:19:38 assertion failed: sidetrace.is_none() ||
16:19:38     matches!(hl_arc.lock().kind, HotLocationKind :: Compiled(_))

One for @ptersilie ?

Pavel-Durov commented 1 year ago

Oh, that's new :)

ptersilie commented 1 year ago
16:19:38 thread '<unnamed>' panicked at ykrt/src/mt.rs:469:21:
16:19:38 assertion failed: sidetrace.is_none() ||
16:19:38     matches!(hl_arc.lock().kind, HotLocationKind :: Compiled(_))

One for @ptersilie ?

Hm, this error is caused by https://github.com/ykjit/yk/blob/master/ykrt/src/mt.rs#L470. This essentially says "if the current location doesn't have a compiled trace, then there also can't be a side trace". Since the only way sidetrace can be set here is via StopSideTracing (https://github.com/ykjit/yk/blob/master/ykrt/src/mt.rs#L279), I don't quite see how we can end up here.

It can't be a coincidence that this is caused by adding coroutines.lua. Maybe @ltratt is more suited for this?

Pavel-Durov commented 1 year ago

Closing this PR as coroutine.lua test is currently failing.

ltratt commented 1 year ago

@Pavel-Durov I don't think we should give up quite that quickly :) Can you investigate what part of the assert is failing? Might be worth inserting something like db!hl_arc.lock().kind); just before the assert.

Pavel-Durov commented 1 year ago

I'm not giving up :) I'm just closing the PR until we fix the issue https://github.com/ykjit/yklua/issues/58

ltratt commented 1 year ago

The bug in that issues seems different than the failing assert here?

Pavel-Durov commented 1 year ago

It does look different but I can't reproduce the same error as in buildbot with the latest YK version c6a0b9ca333408693a0c904bd502b1cdf0dc8d3f

ltratt commented 1 year ago

I wonder if you might have a slightly stale build or similar. If you rm -rf target in yk and make clean in yklua, and rebuild both, does that change anything?

Pavel-Durov commented 1 year ago

I will try doing clean build and run this test again

Pavel-Durov commented 1 year ago

bors try

bors[bot] commented 1 year ago

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

vext01 commented 1 year ago

Is this one ready for squashing then? If so, please go ahead.

Pavel-Durov commented 1 year ago

not ready yet, tests are failing again.

Pavel-Durov commented 1 year ago

bors try

bors[bot] commented 1 year ago

try

Build failed:

Pavel-Durov commented 11 months ago

closing in favour of https://github.com/ykjit/yklua/pull/79