Skip to content

[v0.53.0] Defer generating dereference of vft-symbol in JProfilingValue #22055

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

Conversation

hzongaro
Copy link
Member

@hzongaro hzongaro commented Jun 6, 2025

In the process of finding values to profile, the method TR_JProfilingValue::cleanUpAndAddProfilingCandidates would insert calls to the jProfileValueWithNullCHK non-helper for instanceof and checkcast operations, with the child of each call being an indirect load of the <vft-symbol> for the value that is to be profiled.

During TR_JProfilingValue::addProfilingTrees, those calls would be transformed to guard the dereference of the indirect load with a test of whether the value is a null reference.

However, if generation of profiling trees is halted early during TR_JProfilingValue::lowerCalls, any value beneath the jProfileValueWithNullCHK will simply be left anchored in its original position, unguarded. This can result in a segmentation fault if the value that was to be profiled was a null reference.

This change delays introducing the indirect load of the <vft-symbol> until TR_JProfilingValue::lowerCalls, so that the indirect load is properly guarded. If generation of profiling trees is halted early, the original value will be left behind anchored, allowing it be properly evaluated in its original relative position.

Fixes: #21822

Port of pull request #22028 to v0.53.0-release branch.

hzongaro added 2 commits June 6, 2025 06:18
In the process of finding values to profile, the method
TR_JProfilingValue::cleanUpAndAddProfilingCandidates would insert calls
to the jProfileValueWithNullCHK non-helper for instanceof and checkcast
operations, with the child of each call being an indirect load of the
<vft-symbol> for the value that is to be profiled.

During TR_JProfilingValue::addProfilingTrees, those calls would be
transformed to guard the dereference of the indirect load with a test of
whether the value is a null reference.

However, if generation of profiling trees is halted early during
TR_JProfilingValue::lowerCalls, any value beneath the
jProfileValueWithNullCHK will simply be left anchored in its original
position, unguarded.  This can result in a segmentation fault if the
value that was to be profiled was a null reference.

This change delays introducing the indirect load of the <vft-symbol>
until TR_JProfilingValue::lowerCalls, so that the indirect load is
properly guarded.  If generation of profiling trees is halted early, the
original value will be left behind anchored, allowing it be properly
evaluated in its original relative position.

Signed-off-by:  Henry Zongaro <[email protected]>
If the operation that's being profiled is instanceof or checkcast, the
code in the quickTest block and the helper call block should test the
result of the load of the vft-symbol for profiling.  The previous change
only added the indirect load of the vft-symbol in the IL generated for
the helper call.  This change adds the missing load of the vft-symbol to
the quickTest block as well.

Signed-off-by:  Henry Zongaro <[email protected]>
@hzongaro
Copy link
Member Author

hzongaro commented Jun 6, 2025

@pshipton, @r30shah, may I ask one of you to merge this change to the v0.53.0-release branch?

@pshipton
Copy link
Member

pshipton commented Jun 6, 2025

I would wait until Monday after the head stream change has gone through nightly and other builds.

@rmnattas
Copy link
Contributor

rmnattas commented Jun 11, 2025

Given that this fixes 2 arraylet issues #21985 #21986, wondering if it also needs to be delivered to 25_03?

@hzongaro hzongaro deleted the delay-inserting-vft-symbol-deref-v0.53.0 branch August 11, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants