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
8272586: emit abstract machine code in hs-err logs #5446
Conversation
👋 Welcome back dnsimon! A progress list of the required criteria for merging this PR into |
} | ||
// The first 10 unique code units on the stack should be sufficient | ||
// for investigating crashes. | ||
int printed_len = 10; |
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 10 is too big?
…e abstract assembly if a disassembler is not installed
Webrevs
|
This is going to be very useful! I used to copy+paste the instructions hex block into disassembler.io, having relevant assembly in hs_err will make debugging such situations much easier! This is not a review, just a big thumbs-up on the feature as such! :-) Thanks, Roman |
ResourceMark rm(thread); | ||
Disassembler::decode(cb, st); | ||
st->cr(); |
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.
Why RM used only here?
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 see that you copied existing code. Okay.
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.
Look good in general. I have few small comments.
// Compiled code may use EBP register on x86 so it looks like | ||
// non-walkable C frame. Use frame.sender() for java frames. | ||
frame invalid; | ||
if (t && t->is_Java_thread()) { |
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.
use (t != nullptr)
explicitly
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.
Will do.
BTW, I've never seen nullptr
before - where is it defined?
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.
We start using C++ features:
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#nullptr
if (os::is_first_C_frame(&fr)) break; | ||
fr = os::get_sender_for_C_frame(&fr); | ||
fr = next_frame(fr, t); | ||
if (!fr.pc()) { |
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.
Please use == nullptr
memset(printed, 0, sizeof(address) * printed_len); | ||
|
||
frame fr = os::fetch_frame_from_context(_context); | ||
for (int count = 0; count < printed_len && fr.pc(); ) { |
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.
Use (fr.pc() != nullptr)
* Determines if the code unit denoted by `owner` should be printed. | ||
* If this method returns true, then `owner` has been added to `printed`. | ||
* If `owner` was already in `printed`, this methods returns false. |
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.
Is it because of recursive calls?
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.
Yes - no point in printing the same code more than once.
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.
Good.
Someone from runtime group have to look on this too.
@dougxc This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. 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 25 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. ➡️ To integrate this PR with the above commit message to the |
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.
Hi Doug,
Seems okay from a runtime perspective. A few minor code nits below - and the test needs a change.
Thanks,
David
return false; | ||
} | ||
if (printed[i] == 0) { | ||
printed[i] = owner; |
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.
Seems odd to have this side-effect in what appears to be a query - perhaps add_to_printed
would be more apt: adds owner
to the list of printed owners, unless already present. Returns true
if owner was added, and false it if was already present, or else we've exceeded the printing limit. ?
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.
That's a good suggestion. I've changed it to be add_if_absent
and made it more generic.
* @param printed_len the length of `printed` | ||
*/ | ||
static bool print_code(outputStream* st, Thread* thread, address pc, bool is_crash_pc, | ||
address* printed, int printed_len) { |
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.
Nit: Please align the parameters with the line above
if (is_crash_pc) { | ||
// The interpreter CodeBlob is very large so try to print the codelet instead. | ||
InterpreterCodelet* codelet = Interpreter::codelet_containing(pc); | ||
if (codelet != NULL) { |
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.
Please use nullptr
in new code.
"-Xcomp", | ||
"-XX:CompileCommand=compileonly,MachCodeFramesInErrorFile$Crasher.*", | ||
"-XX:CompileCommand=dontinline,MachCodeFramesInErrorFile$Crasher.*", |
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.
This looks like it requires C2, so there should be an @requires
for that in case the test is run under Zero or a MinimalVM build.
changed should_print_code to add_if_absent made printed_capacity a const value handle platforms where native stack cannot be walked directly by VMError improved Crasher such that the crash happens in a JIT compiled frame
… can have its code printed
I have pushed further changes to this PR to address test failures:
I would appreciate a review of these changes. |
st->print("%7d ", (int)tty->time_stamp().milliseconds()); | ||
if (timestamp) { | ||
// Print current time | ||
st->print("%7d ", (int)tty->time_stamp().milliseconds()); |
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 sure why a timestamp should be included when printing the contents of an nmethod outside the context of a CompileTask. The timestamp was causing the DisassembleCodeBlobTest
to fail as it expects disassembling a given nmethod twice to produce the same result.
The MachCodeFramesInErrorFile test is crashing on macosx-aarch64. I've isolated the problem in lldb:
This looks like a pre-existing bug. |
@dougxc I looked on issues with SIGBUS(0xa) and may be related: Fixes enable WX. May be we need something like this here. |
Thanks, you're right. I placed it in BTW, I noticed that often nmethod info is written to |
Update looks good. |
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.
Hi Doug,
Updates to non-compiler code look fine to me.
Thanks,
David
Thanks @vnkozlov and @dholmes-ora for the input and reviews. |
/integrate |
Going to push as commit b60837a.
Your commit was automatically rebased without conflicts. |
|
This enhances hs-err logs to:
An interpreter or stub frame is only shown if it is the crashing frame.
A sample of the enhanced hs-err log can be seen here.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5446/head:pull/5446
$ git checkout pull/5446
Update a local copy of the PR:
$ git checkout pull/5446
$ git pull https://git.openjdk.java.net/jdk pull/5446/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5446
View PR using the GUI difftool:
$ git pr show -t 5446
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5446.diff