Skip to content

review call-info reset#9302

Merged
kares merged 14 commits intojruby:jruby-10.0from
kares:kares-10.0_kwargs-call-info_clean
Mar 16, 2026
Merged

review call-info reset#9302
kares merged 14 commits intojruby:jruby-10.0from
kares:kares-10.0_kwargs-call-info_clean

Conversation

@kares
Copy link
Copy Markdown
Member

@kares kares commented Mar 12, 2026

trying to get the keywords = true aligned with proper resetCallInfo, where it's needed (follow-up on #9285).

on some places this won't trigger an issue unless something changes in the method (would be easy to miss).

but there also are places where ArgumentError reproduces easily (similar to #9281) e.g.

# On JRuby 10.0.4.0: the 1-arg, 2-arg, and 3-arg forms all leak

class Victim
  def initialize(a, k:)
    @a = a
    @k = k
  end
  def result; [@a, @k]; end
end

puts "Struct#initialize callInfo leak test:"

# Control: no struct creation, no leak
begin
  v = Victim.new(1, k: 2)
  puts "  Control (no Struct):       #{v.result.inspect}"
rescue ArgumentError => e
  puts "  UNEXPECTED: #{e.message}"
end

# Test 1-arg form: S.new(val, **{})
S1 = Struct.new(:x)
begin
  S1.new(1, **{})
  v = Victim.new(1, k: 2)
  puts "  After S1.new(1, **{}):     #{v.result.inspect}"
rescue ArgumentError => e
  puts "  BUG (1-arg form leak):     #{e.message}"
end

# Test 2-arg form: S.new(v1, v2, **{})
S2 = Struct.new(:x, :y)
begin
  S2.new(1, 2, **{})
  v = Victim.new(1, k: 2)
  puts "  After S2.new(1, 2, **{}):  #{v.result.inspect}"
rescue ArgumentError => e
  puts "  BUG (2-arg form leak):     #{e.message}"
end

# Test 3-arg form: S.new(v1, v2, v3, **{})
S3 = Struct.new(:x, :y, :z)
begin
  S3.new(1, 2, 3, **{})
  v = Victim.new(1, k: 2)
  puts "  After S3.new(1,2,3, **{}): #{v.result.inspect}"
rescue ArgumentError => e
  puts "  BUG (3-arg form leak):     #{e.message}"
end

the above isn't completely fixed due being quite specific and reveals an issue with IRRuntimeHelper.setCallInfo which retains the empty flag:

    @JIT @Interp
    public static void setCallInfo(ThreadContext context, int flags) {
        // FIXME: This may propagate empty more than the current call?   empty might need to be stuff elsewhere to prevent this.
        context.callInfo = (context.callInfo & CALL_KEYWORD_EMPTY) | flags;
    }

think that should only be used in specific circumstances as the retaining causes issues (such as above).

not 100% why it's kept (esp. the JIT parts) yet and whether simply changing to context.callInfo = flags would not cause more harm for now this is not needed, the above bug was resolved.

@headius
Copy link
Copy Markdown
Member

headius commented Mar 12, 2026

@kares I think you should give it a try (make it just fully overwrite the flags) and we'll check with @enebo as to why he wrote this to preserve the empty flag. I suspect it was either not intentional or served a purpose that no longer exists.

@kares kares marked this pull request as ready for review March 16, 2026 07:52
@kares kares added this to the JRuby 10.0.5.0 milestone Mar 16, 2026
@kares kares self-assigned this Mar 16, 2026
@kares kares merged commit 3c116ac into jruby:jruby-10.0 Mar 16, 2026
84 checks passed
isLambda = (callInfo & ThreadContext.CALL_KEYWORD) != 0 ?
lambdaOpt.isTrue() :
isLambda();
isLambda = hasKeywords(callInfo) ? lambdaOpt.isTrue() : isLambda();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to:

var isLambda = !lambdaOpt.isNice` && hasKeywords(callInfo) ?
   lambdaOpt.isTrue() :
   isLambda();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is merged. Just noticed the logic could be simplified.

@enebo
Copy link
Copy Markdown
Member

enebo commented Mar 16, 2026

As a comment for posterity. I think passing callInfo as a value in the two places which introduced the extra passing is baking callInfo into the API which was not a goal of callInfo (it was meant to be a temporary workaround for not changing all call code paths). That said, those two places are private and can be backed out simply when a longer term solution comes along. So I was ok with this PR but just thought it is worth mentioning somewhere that the goal was to minimally affect API call paths with callInfo as a value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants