<headius>
byteit101: oh yeah I suppose we could do it this week!
<byteit101[m]>
I think everything on my side is done for it, only optionally have someone else run through it and make sure all the code works/I'm not missing some subtlety
<headius>
ok
<byteit101[m]>
9.3.4 is out, jrubyfx 2.0 is out, maven-require gem is out
<headius>
yeah it's time to do it
<headius>
enebo: what can I run to see the jit failures?
<headius>
spec:compiler will probably do it
<headius>
byteit101: I'll coordinate with enebo to get it posted this week
<byteit101[m]>
Awesome!
<enebo[m]>
headius: anything. it is a verification error
<headius>
I am getting NPE in checkArity
<headius>
just starting up rake
<enebo[m]>
oh yeah
<enebo[m]>
headius: jruby -Xjit.logging -Xjit.logging.verbose -S gem list
<headius>
yeah but it doesn't even get past rake booting
<enebo[m]>
Can you run gem list like I did above and see something consistent with what I am seeing
<enebo[m]>
I guess I am not totally surprised some JIT methods are not quite right since JIT has never worked since I changed it. I will look at checkArity
<headius>
yeah I see the verification errors
<enebo[m]>
I even partially remember doing that to checkArity now that I see it
<enebo[m]>
ah yeah that is in indy right?
<headius>
these are doing dload... seems unlikely that's what they are supposed to do
<headius>
yeah
<headius>
indy is used for a few things all the time, like the checkArity logic
<enebo[m]>
so keywords is a variable
<enebo[m]>
it needs to get wound into that method
<enebo[m]>
and obviously this will constantly fail for all kwargs signatures until it is pushed over to that
<headius>
yeah
<enebo[m]>
yeah the dload_3 is strange
<enebo[m]>
I half wondered if I had changed more JIT called methods than I thought but not properly changed the JIT
<headius>
yeah I don't see it in a dump of the bytecode
<enebo[m]>
but the runtimehelpers and the receives were all I could remember
<enebo[m]>
but checkArity is a thing I did not see even after looking
<enebo[m]>
although the way I broke it should not have broken bytecode verification...just broke check_arity from working
<enebo[m]>
This has been a frustrating morning because everything I did do seems really simple
<enebo[m]>
I think to hook this up I would load keywords in JVMVisitor#checkArity then change sig there and in boostrap for checkarity?
<headius>
yeah this is strange
<headius>
yeah that sounds right
<enebo[m]>
I don't really see any point until the verification error is done but you might just do it if you are solving this
<enebo[m]>
I obviously will do what I can to figure this out as well but I keep staring at those signatures and I cannot see anything not line up
<enebo[m]>
I wish I would have tackled this before the merges I did as well
<headius>
I'll try to fix anything I find in the jit
<enebo[m]>
Since I can search for author=enebo but it is not that useful
<enebo[m]>
headius: so summary of changes is remove frobnicate+ in emitScope as it is not used any more
<enebo[m]>
I see nothing odd there. It does stop doing a storeArgs but we are not actually changing them so that seemed unimportant
<enebo[m]>
new instr for ReceiveKeywords. This is a simple IRRuntimeHelpers call
<headius>
oh it's this: ALOAD -1
<headius>
I saw that the other day too
<enebo[m]>
Most Receives* need to have an extra keywords param. I also removed unneeded parameters
<headius>
ALOAD -1 must get encoded as DLOAD_3
<enebo[m]>
haha
<headius>
it is back to trying to load args array when there's a single arg
<headius>
this is what I moved the receive keywords logic inside the hasKeywords preamble for the other day
<enebo[m]>
wow that is the same problem
<enebo[m]>
and I just removed that entire section
<enebo[m]>
Although that frob section exists as far back as 9.3
<headius>
so basically it looks like receiveKeywords instr is firing for this method: def min3(a, b, c)
<enebo[m]>
maybe the storeArgs() is needed?
<headius>
but there's no args array so that aload is -1
<enebo[m]>
like something with that stores it as non-fixed
<headius>
it is specific arity, there's nothing to store
<enebo[m]>
but why would adding that keywords stuff at front fix it?
<enebo[m]>
I am not saying it does but you more or less got rid of aload -1 by putting it into same if
<headius>
yes because that logic only ran if we receive keywords, which does not get specific arity
<headius>
so there would be an arg[]
<enebo[m]>
ah well in the new design we don't know if we will have **something or not
<enebo[m]>
or in old design for that matter
<headius>
but the above method can never receive keywords
<enebo[m]>
The relevance is it might be **{} in some form which is acceptable to that min3
<headius>
I am confused... def min(a, b, c) can't receive keywords no matter what you do
<enebo[m]>
but if I min3(1, 2, 3, **empty) it will run
<enebo[m]>
and I do clean that on caller side but I am not sure it can be done 100% of the time
<headius>
it still won't have an arg array though
<headius>
not with specific-arity jit
<headius>
the kwargs part should get stripped off and only the regular args passed, and the body should not try to receive keywords because there are none
<enebo[m]>
The immediate fix to this would be for me to change in JIT fixed args to always return undefined for kwrest
<enebo[m]>
I have been staring at callsite generation side of things and I did eliminate some amount of passing the empty kwargs in the first place but I am not 100% it can be 100%
<enebo[m]>
but I suppose this is a big motivation
<enebo[m]>
There are forms of methods which do not accept kwargs which do accept them
<headius>
then gem list gets as far as an arity error: wrong number of arguments (given 1, expected 2)
<enebo[m]>
ruby 3 kwargs is complicated
<enebo[m]>
in my snippets directory I am up to kwargs71.rb
<headius>
rake no longer NPEs but fails with arity errors
<headius>
hah nice
<enebo[m]>
I have hit many many weird broken things (more than 71)
<enebo[m]>
So arity error likely is doing nothing is worse than calling letting it know it cant work
<enebo[m]>
So I can take your diff and pass specificArity as a flag to that
<enebo[m]>
It might make most things work properly
<enebo[m]>
receive_keywords returns undefined or the value
<enebo[m]>
everything depends on it being undefined when nothing
<enebo[m]>
but specificArity will never contain rest args right?
<headius>
never
<enebo[m]>
Just simple 0,1,2,3 aritties
<headius>
only up to three regular positional args
<headius>
and only if the method is defined with just those positional args
<headius>
we don't do anything to split methods with variable arities
<headius>
(in the jit)
<enebo[m]>
I think this will be fine if I just change to push undefined for specificArity
<headius>
I am wondering why a specific-arity method with no rest and no kwargs would have this instr at all though
<enebo[m]>
all the instrs require this instr now
<enebo[m]>
which if it is just pushing undefined onto the stack is fine
<headius>
yeah it ends up storing a local, can we just store the value we know it should be for specific-arity?
<enebo[m]>
At some point these instrs in JIT can do something else and ignore emitting anything
<enebo[m]>
but all the isntrs which know about it also need to be aware
<enebo[m]>
which is not a big deal but it means bifurcating the helpers a little more and in the visitor too
<enebo[m]>
if you plan on some indy followup none of this seems like it is worth it
<enebo[m]>
headius: the quickest path would be passing a boolean to receivesKeywrods. If it is true we just return undefined at the top
<enebo[m]>
That is some extra overhead but that can be trivially changed later
<enebo[m]>
then we don't need to cache undefined into the method
<headius>
yeah whatever works right now is fine
<enebo[m]>
headius: if you can hook up keywords in check_arity that would complete this enough for us to probably get you do looking at the maybe final issue with rails console starting (visibility issue)
<enebo[m]>
I still have a problem with 'def foo(...); super; end'
<enebo[m]>
Trivially worked around by def foo(...); super(...); end'
<enebo[m]>
something which looks so trivial ends up not being that trivial :)
<enebo[m]>
oh god I would have never figured out the fix arg part of this
<enebo[m]>
so I thought it would be simpler but I guess having it do equiv of 'push undefined; jvmStoreLocal(instr.getResult())' is what we want if specific
<enebo[m]>
the boolean thing is dumb now since if will need to be used to avoid loadArgs regardless
<enebo[m]>
I made an undefined() in IRRuntimeHelpers which is probably silly but it is quick for me
<enebo[m]>
UNDEFINED is an odd duck as it implemented IRubyObject directly and is static
<enebo[m]>
So in theory I guess this could just be a static field access in bytecode
<enebo[m]>
yay
<headius>
doesn't matter too much, static methods are trivially inlinable
<enebo[m]>
This is how all the other methods get it although thisd could be jvmLocalLocal(variable)?
<headius>
there is no getKeywords
<headius>
in CheckArity
<headius>
I'm going to assume this CheckArity.getOperand1 will load the keywords
<enebo[m]>
oh it never got renamed there is a keyword as last parm
<enebo[m]>
it will be getOperand1() until I make a getKeywords() method for it
<enebo[m]>
headius: getKeywords() is now a method that I pushed
<headius>
for specific arity what should it be?
<enebo[m]>
It will get stored into that variable/local as undefined
<headius>
all specific arity bodies are wrapped by a varargs body that calls checkArity
<headius>
so I need to pass something for keywords there
<headius>
I guess just undefined?
<enebo[m]>
get_keywords currently will still be emitted even for specific arity methods but that stored local is the same local check_arity accesses
<enebo[m]>
so it will save undefined to that local
<enebo[m]>
specific arity method can omit loading this local completely but for now let's just get it passing using the same basic logic
<headius>
what I mean is this is a synthetic method that just checks arity and calls the specific-arity version
<enebo[m]>
all methods which have check_arity will have a receive_keywords and that will save as undefined on specific arity methods
<headius>
it is the wrapper that allows args[] call paths to get into the specific-arity body
<enebo[m]>
oh so it is doing this check already
<enebo[m]>
ok I didn't realize this was a thing
<headius>
at least it knows it is going into a specific-arity target
<headius>
it is just the bridge from IRubyObject[] call path to specific arity
<enebo[m]>
If this happens before the actual method then I guess at the same place that call happens the receive keywords method can be called
<headius>
ok if that is part of the protocol it will need to go into other places where we take IRubyObject[] and split it among arities
<enebo[m]>
yeah hmm. I am looking at checkArity calls
<enebo[m]>
So things call from native calls will not have keywords
<enebo[m]>
so in those cases it could just pass undefined native -> specific
<headius>
right
<enebo[m]>
empty kwrest MIGHT be something the callside of Ruby -> anything can eliminate. I might have did it but it is not as obvious as you would think
<enebo[m]>
args lists are array, splat, args_push and args_cat
<enebo[m]>
I believe I only needed to introduce logic for push and cat
<enebo[m]>
but non-empty **kw can go to non-accepting keyword methods if it has a *arg
<enebo[m]>
done it
<headius>
trying my patches with yours now
<enebo[m]>
headius: So for JIT the wrapper for specific getting args[] is emitVarargsMethodWrapper?
<headius>
yes
<headius>
I just have two overloads of checkArity, one that takes a keywords operand and one that just passes UNDEFINED
<headius>
for the moment
<enebo[m]>
yeah
<enebo[m]>
the cover for this method would be if we still have any paths for **empty we could do receive_keywords helper and pass its result to checkArity
<enebo[m]>
but we may as well try without it and see if anything happens
<headius>
we probably can do that in these wrappers
<headius>
gem list works again
<enebo[m]>
THE last bugs I fixed were ones where I had to strip **empty from call side for native methods which are incapable of it
<enebo[m]>
at this point if this is ok enough you can set --dev and run rails app...fix the single zsuper to be super(...) and then run into the next blocker on rails console coming up
<enebo[m]>
it is a permission issue
<headius>
Sure I'll give Rails a try next
<enebo[m]>
I will fix spec:compiler until I can't since I hitnk it will just be fixing persistance
<enebo[m]>
gem list is working
<enebo[m]>
I will just commit this I guess
<enebo[m]>
It looked good to me. I guess we might maybe hit an issue on varargs path into specific args but we shall see I guess
<enebo[m]>
not too hard to fix that if so
<enebo[m]>
I would like to figure out if I can completely eliminate passing **{} from the callside...or whether I did already
<enebo[m]>
but I need to figure out zsuper before that since we want to cleanly start up rails
<headius>
Varargs into specific arity should do what exactly?
<headius>
Does it ignore the keywords?
<enebo[m]>
it may need to realize **{} is there and then assign that to keywords
<enebo[m]>
check_arity will then ignore it for checks
<enebo[m]>
specific arity ignores the recv instrs so it will just pass the first n params along
<headius>
If it's going to ignore anyway why does it have to look for **{}
<enebo[m]>
sometimes it gets passed along (or it might not now)
<enebo[m]>
I am not sure if it ever gets passed now or not but the instrs can cope with that
<headius>
I guess that is what is confusing me, if it is specific arity it can neither receive nor propagate keywords
<enebo[m]>
if I can prove it never happens I can remove some logic and then specific arity would not need this in the varargs conversion stuff
<enebo[m]>
but varargs is an unchecked path no?
<headius>
If you mean the wrapper, It is only used when calling specific arity bodies
<enebo[m]>
for specific arity paths I am assuming we only pass ordinary values through those
<enebo[m]>
oh the args[] is still only going to get ordinary parameters?
<headius>
When we cannot do specific arity, then it will process the argument array and keywords as normal
<headius>
Well it will get whatever was passed but it only calls the specific body
<headius>
With the first three arguments in the list for a three arity method for example
<enebo[m]>
yeah. I am not sure if whatever is passed will ever contain **{} without knowing what can call that. I assume anything can
<headius>
Prior to this all it did was check if there's enough arguments to pass on
<headius>
It seems to me that someone passing keywords to a method that does not receive them is also going to be a rarity and probably a user bug
<enebo[m]>
probably yeah
<enebo[m]>
ruby2_keywords is a weird case but even then for an ordinary method without keywords it would justl eave it as an ordinary hash argument (still marked as ruby2_keywords)
<headius>
Okay that might be a reason to handle it
<enebo[m]>
but that marked value makes it all a bit weirder since you may end up propagating something a ways away and it will turn into keywords then if that hits (...) then it will pass it as keywords
<headius>
If we have four arguments but the fourth is a new style keyword hash we ignore it
<headius>
If it is a Ruby 2 keyword hash we would pass it as a positional argument?
<headius>
Maybe simpler with a one argument method...
<enebo[m]>
ruby2_keywords for fixed simple arity methods will leave the marker on it
<headius>
One argument method that receives a new style keyword hash, that should be an arity error?
<enebo[m]>
but it will just be a normal value as far as that method is concerned
<headius>
Receives only the hash
<enebo[m]>
unless you splat it to that method
<enebo[m]>
if you splat it then it removes the flag unless it happens to potentially receive keywords
<enebo[m]>
and in that case it dups the last argument
<enebo[m]>
The logic is pretty crazy
<headius>
And in that case it would be passed as the soul argument a la ruby 2
<headius>
Sole
<enebo[m]>
if it is normal arity it will just be a hash as last arg
<enebo[m]>
If it is kwargs arity it will make it into a kwarg
<headius>
So I think the varargs wrapper does need to look at the last argument and treat it accordingly when calling the specific body
<enebo[m]>
callsite matters but just for *args where it mostly nukes the ruby2_keyword flag
<headius>
def foo(a)
<enebo[m]>
yeah I don't think so
<headius>
If I pass only keywords to that what should happen
<enebo[m]>
foo(a, **{})
<enebo[m]>
This will work for native or Ruby code
<headius>
Sure, what will be in the argument list?
<enebo[m]>
(and we do some opt here so you need empty = {}, foo(a, **empty)
<headius>
And in that case it will pass an empty hash marked as keywords
<enebo[m]>
as it happens I believe I am stripping out **empty at this point but I have not proven it
<headius>
As the last argument
<enebo[m]>
any IR though can also see this on method side and ignore it as a parameter
<headius>
I'm just trying to think through scenarios in the varargs wrapper
<enebo[m]>
In reading MRI I swear they have this logic but as I said above the LAST bug I fixed made me realize native methods 100% have to strip off an empty kwargs
<enebo[m]>
This was the actual reason root was not working in Rails
<enebo[m]>
We ended up hitting some native class of ours which it was erroring on and not really in root
<enebo[m]>
So once I fixed that I am standing back wondering whether any of the logic on receive side for this case is now needed
<enebo[m]>
The answer is I don't know yet
<headius>
Are there specs for calling a specific method with various keyword forms?
<enebo[m]>
yeah we pass them all short of **nil
<enebo[m]>
well one new one came in today with the spec update
<enebo[m]>
The big exposed issue not covered in MRI nor specs is calling native methods from Ruby using keyword combos
<enebo[m]>
Not all impls have the same methods as native so this is probably a challenge to make good tests for too
<enebo[m]>
I am done for today but I have some amount of spec:compiler running
<enebo[m]>
like 18 ran off the interpreter errors so something is not decoding right in several cases
<enebo[m]>
In the end this will be something like optargs instr encoding an extra field we don't use
<enebo[m]>
persistence and the java bytecode have a lot in common...both give horrible errors
<headius>
Haha
<headius>
Yeah
<headius>
e
<headius>
enebo: Rails app generation fails in the delegate stuff again