<headius> Good morning!
<byteit101[m]> headius: when were you planning on posting the blog post?
<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> if (restKey == -1 && keywords != UNDEFINED) checkForExtraUnwantedKeywordArgs(context, scope, (RubyHash) keywords);
<headius> keywords ends up null here so it proceeds to check for unwanted args and NPEs
<headius> perhaps you started passing null instead of undefined?
<enebo[m]> java.lang.VerifyError: Bad local variable type
<enebo[m]> I am getting this for every JIT
<enebo[m]> well it might be something happening in the JIT which doesn't in the interpreter
<enebo[m]> If you run with --dev and you see it then something else is wrong (like I committed something later I suppose)
<enebo[m]> I should say I do not notice any successes in JITTing in the logs
<headius> ok that comes from here:
<enebo[m]> You are running spec:compiler?
<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
<enebo[m]> *r for example will
<headius> that fixes the verification error
<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
<headius> I got past rake boot into mspec
<enebo[m]> benoit updated specs and we only got a single new failure for kwargs and it was an MRI bug one so cool there
<headius> then it fails here:
<headius> Unhandled Java exception: java.lang.IllegalArgumentException: Unhandled operation: recv_kw... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/652d7ba86aeb796d63211f411f181443aeb50abc)
<headius> IR persistence
<enebo[m]> I imagine it is not having keywords to check_arity
<enebo[m]> oh wow
<enebo[m]> ok I am not surprised
<enebo[m]> it is missing the decode line to construct the instr
<enebo[m]> but why is that happening?
<headius> that would do it
<enebo[m]> oh spec:compiler
<enebo[m]> haha ok
<enebo[m]> Trivial
<headius> ah yeah
<enebo[m]> I will address that
<headius> I will apply your patch
<enebo[m]> hook up that arity and I will fix that
<headius> the rest of mine is just this
<headius> diff --git a/core/src/main/java/org/jruby/ir/targets/indy/Bootstrap.java b/core/src/main/java/org/jruby/ir/targets/indy/Bootstrap.java... (full message at https://libera.ems.host/_matrix/media/r0/download/libera.chat/ecfc76344e5d2ed1b8a859d2018efde5f15d3d46)
<headius> ick
<headius> I don't know if that is right or not
<enebo[m]> we want what the variable provides
<enebo[m]> unless it is only specificArity but then that shoudl store undefined
<enebo[m]> Is that patch working for you?
<enebo[m]> jvmAdapter().ldc(jvm.methodData().specificArity >= 0);
<headius> I'm trying with your patch now
<enebo[m]> hahaha
<enebo[m]> ok yeah that won't work
<enebo[m]> loadArgs is still in there
<headius> yeh I see that now
<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]> updated
<enebo[m]> gem list has two -1 errors now
<headius> hmm I get compile errors now
<headius> oh you didn't remove the extra boolean from the signature
<enebo[m]> ah yeah...weird it did not barf on me
<headius> gem list worked
<headius> no JIT errors
<enebo[m]> progress but check_atiry is not providing keywords
<headius> just adds the UNDEFINED passing in checkArity
<enebo[m]> yeah that UNDEFINED needs to be the keywords
<enebo[m]> from receive_keywords
<headius> ah I see
<enebo[m]> Largely passing it as undefined should cause some off by one arity errors because undefined means no keywords
<enebo[m]> I think intellij decided to do a bunch of stuff at same time as my build
<enebo[m]> I am going to commit what is here now for this part since I think it is right
<headius> ok
<enebo[m]> as right as the deisgn is anyways
<enebo[m]> There are some oddities in MRI but it is largely involving **nil and local_variables in these methods
<enebo[m]> but ruby/spec only has a single fail and that just came in this morning
<enebo[m]> I figure if we knuckle to getting rails to fully boot that stuff can come after
<headius> I need to store the local somewhere so I can retrieve it
<headius> does the CheckArity instr have it?
<enebo[m]> It is a variable
<enebo[m]> so receive_keywords stores the result
<headius> right and it is then input to another instr
<enebo[m]> it is input to every recieve instr (mostly)
<headius> but I need to pass it to checkArity
<headius> yes?
<enebo[m]> yes
<headius> how do I find it
<headius> Object keyword = getOperand1().retrieve(context, self, scope, dynamicScope, temp);
<headius> so add this into the args
<enebo[m]> visit(instr.getKeywords());
<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
<headius> this is on head of your branch
<enebo[m]> ok applied
<enebo[m]> rebuilding
<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
<headius> not sure what zsuper I needed to change