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

YJIT: Properly deal with cfunc splat when no args needed #7413

Merged
merged 1 commit into from Mar 1, 2023

Conversation

jimmyhmiller
Copy link
Contributor

@jimmyhmiller jimmyhmiller commented Mar 1, 2023

Related to:
#7377

Previously it was believed that there was a problem with a combination of cfuncs + splat + send, but it turns out the same issue happened without send. For example Integer.sqrt(1, *[]). The issue was happening not because of send, but because of setting the wrong argc when we don't need to splat any args.

Related to:
ruby#7377

Previously it was believed that there was a problem with a combination
of cfuncs + splat + send, but it turns out the same issue happened
without send. For example `Integer.sqrt(1, *[])`. The issue was
happened not because of send, but because of setting the wrong argc
when we don't need to splat any args.
@matzbot matzbot requested a review from a team March 1, 2023 20:02
@jimmyhmiller
Copy link
Contributor Author

The failures are an rbs thing that I've seen elsewhere unrelated to this patch.

@maximecb maximecb merged commit cb8a040 into ruby:master Mar 1, 2023
@maximecb
Copy link
Contributor

maximecb commented Mar 1, 2023

Good catch Jimmy :)

@k0kubun
Copy link
Member

k0kubun commented Mar 2, 2023

I'm not sure why our CI never catches it, but it seems like this PR re-introduced the bug that was stopped by #7377.

As of a80906a, spec/ruby/library/bigdecimal/mult_spec.rb succeeds:

$ make test-spec SPECOPTS="spec/ruby/library/bigdecimal/mult_spec.rb" RUBYOPT=--yjit-call-threshold=1
make: Entering directory '/home/k0kubun/src/github.com/ruby/ruby/.ruby'
$ /home/k0kubun/src/github.com/ruby/ruby/.ruby/miniruby -I/home/k0kubun/src/github.com/ruby/ruby/lib /home/k0kubun/src/github.com/ruby/ruby/tool/runruby.rb --archdir=/home/k0kubun/src/github.com/ruby/ruby/.ruby --extout=.ext -- /home/k0kubun/src/github.com/ruby/ruby/spec/mspec/bin/mspec-run -B ../spec/default.mspec ../spec/ruby/library/bigdecimal/mult_spec.rb
ruby 3.3.0dev (2023-03-01T21:07:40Z master a80906a714) +YJIT dev [x86_64-linux]
[/ | ==================100%================== | 00:00:00]      0F      0E

Finished in 0.160945 seconds

1 file, 7 examples, 228 expectations, 0 failures, 0 errors, 0 tagged
make: Leaving directory '/home/k0kubun/src/github.com/ruby/ruby/.ruby'

In the next commit cb8a040 with this patch, it fails:

$ make test-spec SPECOPTS="spec/ruby/library/bigdecimal/mult_spec.rb" RUBYOPT=--yjit-call-threshold=1
make: Entering directory '/home/k0kubun/src/github.com/ruby/ruby/.ruby'
$ /home/k0kubun/src/github.com/ruby/ruby/.ruby/miniruby -I/home/k0kubun/src/github.com/ruby/ruby/lib /home/k0kubun/src/github.com/ruby/ruby/tool/runruby.rb --archdir=/home/k0kubun/src/github.com/ruby/ruby/.ruby --extout=.ext -- /home/k0kubun/src/github.com/ruby/ruby/spec/mspec/bin/mspec-run -B ../spec/default.mspec ../spec/ruby/library/bigdecimal/mult_spec.rb
ruby 3.3.0dev (2023-03-01T21:33:16Z master cb8a040b79) +YJIT dev [x86_64-linux]

1)
BigDecimal#mult returns zero of appropriate sign if self or argument is zero ERROR
TypeError: no implicit conversion of false into Integer
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:36:in `mult'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:36:in `block (2 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/mult_spec.rb:5:in `<top (required)>'

2)
BigDecimal#mult returns NaN if NaN is involved ERROR
TypeError: no implicit conversion of false into Integer
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:54:in `mult'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:54:in `block (3 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:53:in `each'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:53:in `block (2 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/mult_spec.rb:5:in `<top (required)>'

3)
BigDecimal#mult returns zero if self or argument is zero ERROR
TypeError: no implicit conversion of false into Integer
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:64:in `mult'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:64:in `block (4 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:63:in `each'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:63:in `block (3 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:62:in `each'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:62:in `block (2 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/mult_spec.rb:5:in `<top (required)>'

4)
BigDecimal#mult returns infinite value if self or argument is infinite ERROR
TypeError: no implicit conversion of false into Integer
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:78:in `mult'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:78:in `block (4 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:77:in `each'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:77:in `block (3 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:76:in `each'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:76:in `block (2 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/mult_spec.rb:5:in `<top (required)>'

5)
BigDecimal#mult returns NaN if the result is undefined ERROR
TypeError: no implicit conversion of false into Integer
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:92:in `mult'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/shared/mult.rb:92:in `block (2 levels) in <top (required)>'
/home/k0kubun/src/github.com/ruby/ruby/spec/ruby/library/bigdecimal/mult_spec.rb:5:in `<top (required)>'
[- | ==================100%================== | 00:00:00]      0F      5E

Finished in 0.175124 seconds

1 file, 7 examples, 4 expectations, 0 failures, 5 errors, 0 tagged
make: *** [uncommon.mk:896: yes-test-spec] Error 1
make: Leaving directory '/home/k0kubun/src/github.com/ruby/ruby/.ruby'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants