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
JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test #12535
JDK-8302320: AsyncGetCallTrace obtains too few frames in sanity test #12535
Conversation
|
@parttimenerd The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
src/hotspot/cpu/x86/frame_x86.cpp
Outdated
// unextended sp must be within the stack | ||
if (!thread->is_in_full_stack_checked(unextended_sp)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not at all sure this relaxation is valid. What are you basing this on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fails without this relaxation and the relaxation has been used on PPC to solve a similar issue a few years back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original assumption does not always hold anymore and should therefore relaxed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unextended_sp < sp
is possible on x86 when interpreter entries generated by MethodHandles::generate_method_handle_interpreter_entry()
are executed. They modify sp
(here for example) but not sender_sp
(InterpreterMacroAssembler ::_bcp_register
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From frame_x86.hpp
:
// The interpreter and adapters will extend the frame of the caller.
// Since oopMaps are based on the sp of the caller before extension
// we need to know that value. However in order to compute the address
// of the return address we need the real "raw" sp. By convention we
// use sp() to mean "raw" sp and unextended_sp() to mean the caller's
// original sp.
by that definition unextended_sp
is always > sp
. If something has violated that definition/convention then we may have bigger problems!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If an ordinary interpreter entry is used for a call this means the callee will be interpreted
Maybe we're not talking about the same thing?
Method
has a from_interpreted_entry
and a from_compiled_entry
, but which one is used depends on the caller, and which calling convention they are using. The callee can be either interpreted or compiled (and if there's a mismatch, we go through an adapter). The entry generated by generate_method_handle_interpreter_entry
is essentially the from_interpreted_entry
, AFAIU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entry generated by generate_method_handle_interpreter_entry is essentially the from_interpreted_entry, AFAIU.
But it is also stored in Method::_i2i_entry
which is used by c2i adapters.
(Maybe too) long version:
The interpreter has entries depending on the MethodKind
. See AbstractInterpreter::entry_for_kind()
. The entries are stored in AbstractInterpreter::_entry_table
. In MethodHandlesAdapterGenerator::generate()
the interpreter entries for method handle intrinsics MethodKinds are generated using the generator MethodHandles::generate_method_handle_interpreter_entry()
.
In Method::link_method()
the entry points are set depending on the MethodKind
. The interpreter entry (see above) for the method's MethodKind
is looked up and stored in Method::_i2i_entry
which is also used by c2i adapters.
So to me it looks as if the entry generated by generate_method_handle_interpreter_entry()
can be reached coming from a compiled caller. The generator for ordinary methods is TemplateInterpreterGenerator::generate_normal_entry()
. The entries it generates are surely reached by compiled callers. I might be missing something in the case of MH intrinsics but right now it looks to me as if the caller can be compiled too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Maybe too) long version:
Not at all :) I don't know much about the interpreter, so this is pretty helpful.
Ok. I think the main point is that a MH linker method doesn't have use a c2i adapter, it instead has separate versions for interpreted and compiled callers altogether (except linkToNative
which doesn't have an interpreter version atm)
Longer version:
I think there are 4 theoretical cases (are there more?):
- interpreted -> MH interpreter entry -> interpreted (the case we want to fix, I think)
- interpreted -> MH interpreter entry -> i2c -> compiled
- compiled -> c2i -> MH interpreter entry -> interpreted
- compiled -> c2i -> MH interpreter entry -> i2c -> compiled
So... my understanding is that a c2i adapter is used when a callee is interpreted, so its from_compiled_entry
points to the c2i adapter. A compiled caller calls the from_compiled_entry
, so it ends up going through the c2i
adapter of an interpreted callee. However, for method handle linkers, the from_compiled_entry
never points to a c2i adapter, but to the MH linker compiler entry generated in gen_special_dispatch
in e.g sharedRuntime_x86_64
. So, in other words, the 3. and 4. scenarios above are not possible, AFAICS. An MH linker's interpreted/compiled entries simply replace the regular i2c
and c2i
adapters for a MH linker method. Though, the actual target method can be either compiled or interpreted, so we can go into either a c2i
adapter from a MH linker compiler entry, or a i2c
adapter from a MH linker interpreter entry (which is scenario 2.).
For scenario 1.: caller sets r13
before jumping to callee (MH linker), we end up in the MH adapter, pop the last argument, and adjust r13
for the actual callee. The MH interpreter entry uses from_interpreted_entry
of the target method, so we don't go through any adapter.
For scenario 2.: mostly the same as 1., but we go through an i2c
adapter, and the compiled callee doesn't care about what's in r13
any ways. (there's a comment in the ic2 adapter code about preserving r13
for the case that we might go back into a c2i adapter if we lose a race where the callee is made non-entrant, but r13
is also used as a scratch register later in the i2c code, and the c2i code overwrites r13
any ways. So, I think that is an outdated comment...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So... my understanding is that a c2i adapter is used when a callee is
interpreted, so itsfrom_compiled_entry
points to the c2i adapter. A
compiled caller calls thefrom_compiled_entry
, so it ends up going through
thec2i
adapter of an interpreted callee. However, for method handle
linkers, thefrom_compiled_entry
never points to a c2i adapter, but to the
MH linker compiler entry generated ingen_special_dispatch
in e.g
sharedRuntime_x86_64
. So, in other words, the 3. and 4. scenarios above are
not possible, AFAICS.
I see. Thanks! That's what I've been missing. It's implemented in SystemDictionary::find_method_handle_intrinsic()
I think.
I agree that it will be ok to modify r13 as you suggested. Maybe explaining why we never reach here coming from a c2i adapter: because native wrappers are generated eagerly in SystemDictionary::find_method_handle_intrinsic()
unless -Xint is given but then the caller cannot be compiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I integrated your suggestion, it seems to work now. I'm hopeful that our internal queue does not find a test error.
I'm happy for any commenting suggestions.
There are crashes in stack walking with this change. Try e.g. vmTestbase/vm/mlvm/indy/func/jvmti/stepBreakPopReturn/INDIFY_Test.java
|
There also errors on Linux, e.g.
So back to my original suggestion which is simple and worked (ran it through our internal test queue and found no errors)? |
Java stack
|
Thanks for trying it out! I ran tier 1-4 here as well, and all failing tests are in Also, I later realized that interpreter frames also save the sp before before making a call ( There's probably a bit of tech debt here (filed: https://bugs.openjdk.org/browse/JDK-8302745). I had a closer look at the original fix, and it looks like the |
Could you be so kind to synthesize a comment for me? I'm slightly overwhelmed by the discussion. |
Something like:
|
Thanks, I modified my PR accordingly. |
Is this PR then ready? |
I'm somewhat concerned that the code doesn't seem to have a clear notion of exactly how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new check looks good to me. I think the only potential issue is in cases where unextended_sp == sp
and sp - Interpreter::stackElementSize
is somehow outside of the stack. But, since this is looking at a sender, it would mean that the current frame would definitely be outside of the stack, so I think we're good.
I'll submit tier 1-4 on our CI, and give a check mark when it comes back green.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest version seems a more conservative constraint, so as long as that fixes things it seems good to me. Thanks.
|
@parttimenerd This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 133 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JornVernee, @dholmes-ora, @reinrich) but any other Committer may sponsor as well.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Richard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing looks good
/integrate |
@parttimenerd |
/sponsor |
Going to push as commit db483a3.
Your commit was automatically rebased without conflicts. |
@JornVernee @parttimenerd Pushed as commit db483a3. |
/backport jdk17u |
@parttimenerd To use the |
Extends the existing AsyncGetCallTrace test case and fixes the issue.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12535/head:pull/12535
$ git checkout pull/12535
Update a local copy of the PR:
$ git checkout pull/12535
$ git pull https://git.openjdk.org/jdk pull/12535/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12535
View PR using the GUI difftool:
$ git pr show -t 12535
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12535.diff