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

Address all tests failing with asserts enabled #5692

Closed
headius opened this issue Apr 12, 2019 · 8 comments

Comments

@headius
Copy link
Member

commented Apr 12, 2019

In #5612 (comment), @kares fixed our builds to properly pass -ea flag down to child processes, and as a result a bunch of test suites went red. This may indicate these asserts are not correct, or that we have code that happens to work even though it violates valid asserts. Either way they all need to be addressed.

The patch is here:

diff --git a/bin/jruby.bash b/bin/jruby.bash
index 4e50cb0b5dc..7318d896deb 100755
--- a/bin/jruby.bash
+++ b/bin/jruby.bash
@@ -240,6 +240,7 @@ do
         else
             if [ "${val:0:3}" = "-ea" ]; then
                 VERIFY_JRUBY="yes"
+                java_args=("${java_args[@]}" "${1:2}")
             elif [ "${val:0:16}" = "-Dfile.encoding=" ]; then
                 JAVA_ENCODING=${val:16}
             elif [ "${val:0:20}" = "-Djava.security.egd=" ]; then

An example build showing all failed suites (for as long as it exists in Travis history) is here: https://travis-ci.org/kares/jruby/builds/518835139

Note that at least one of these failures is due to the issue reported in #5612.

@headius headius added this to the JRuby 9.2.8.0 milestone Apr 12, 2019

@kares

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

yeah, I was looking into that. will shoot off a PR

@kares kares closed this Apr 12, 2019

@kares kares reopened this Apr 12, 2019

headius added a commit that referenced this issue Apr 12, 2019

Eliminate circular static init for Frame and Binding.
Block aggregates a Binding which aggregates a Frame which
aggregates a Block, so having these all initialize statically
caused at least one of those reference to end up null. In #5693
this manifested as a failure to dup the frame for a Symbol proc,
since Symbol procs are initialized with Frame.DUMMY, which static
initialized before Block, so Block.NULL_BLOCK was null and as a
result the block field in Frame was null and triggered asserts.

Block.NULL_BLOCK, as the most heavily used, remains as the sole
static "dummy" in this cycle. NULL_BLOCK is initialized with
a new Binding containing an uninitialized Frame, and then the
Frame is updated to point back to NULL_BLOCK. This avoids the
circulate initialization. Previous references to the dummy Frame
and dummy Binding now use NULL_BLOCK's contents.

See #5692 for the catch-all issue that goes with PR #5693.
@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Merged #5693 to master to go with my fixes, which should make everything green. None of the ones I fixed were really critical, but they might have led to odd errors further down the line.

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Still seems to be a few red; I'm working on it.

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Fixed an issue with BEGIN and END breaking the compiler tests; they compile ok, but we were not running optimization passes, and then the test attempted to wrap the code in CompiledIRMethod which requires those passes.

ef1cb4d

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

@enebo Do you know any reason why we wouldn't be able to optimize BEGIN and END like any other block, especially for blocks? See my commit above; I just turned it on and it seems to work.

@enebo

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

@headius I think rationale behind not bothering is that they only execute once and AOT essentially will save them as startup interp instrs. As far as whether they can be compiled? Only thing I wonder about is variable scoping itself.

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

GREEN

@headius headius closed this Apr 13, 2019

@headius

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

@enebo Yeah they seem to be working fine, and I could find no reason why they wouldn't optimize. Lots of old comments in there they predate 9k.

See #5695 for my PR to get BEGIN working 100% in bytecode-compiled code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.