RFR(S): 8008357: [sampling] assert(sender_blob->is_runtime_stub() || sender_blob->is_nmethod()) failed: Impossible call chain

[Markus Grönlund]

Rickard,

Sorry, please forget about my first comments on the first assert:

"The comments on line 221-222 is are a bit confusion in association with the conditional and the follow-up assert on line 225. The comment outlines the layout and invariant for nmethods, but in the conditional (sender_blob->frame_size() <= 0), there is an assert that sender_blob is NOT an nmethod?"

This is of course correct, I read it wrongly. The assert is placed ok there.

Maybe just fix up the comments?

Cheers
Markus



-----Original Message-----
From: Markus Grönlund 
Sent: den 5 mars 2013 08:37
To: Rickard Bäckman
Cc: serviceability-dev serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: RE: RFR(S): 8008357: [sampling] assert(sender_blob->is_runtime_stub() || sender_blob->is_nmethod()) failed: Impossible call chain

Rickard,

The comments on line 221-222 is are a bit confusion in association with the conditional and the follow-up assert on line 225. The comment outlines the layout and invariant for nmethods, but in the conditional (sender_blob->frame_size() <= 0), there is an assert that sender_blob is NOT an nmethod? This is a bit unclear. Also, the sentence is weird:

"if the frame size is 0 something (or less) is bad because every nmethod..."

Also, do you need that particular assert there? The same logic comes again in non-asserting code at line 234. Maybe just rework/remove the assert and weave in the real logic for that check?


Not a Reviewer.

Cheers
Markus



-----Original Message-----
From: Rickard Bäckman
Sent: den 5 mars 2013 08:08
To: serviceability-dev serviceability-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(S): 8008357: [sampling] assert(sender_blob->is_runtime_stub() || sender_blob->is_nmethod()) failed: Impossible call chain

Anyone have time to look at this?

Thanks
/R

On Mar 1, 2013, at 10:27 AM, Rickard Bäckman wrote:

> Hi all,
> 
> here comes another update to frame.safe_for_sender. 
> If the PC at a place where the stack doesn't match the _frame_size we sometimes read an invalid return PC. 
> In this case we read one that pointed into the Safepoint blob. 
> 
> We now pretty much guarded for all kind of blobs in safe_for_sender, 
> so I've changed the method to not assert in the end but to do it the 
> same was as the frame_sparc.cpp did it. Everything that is not a nmethod at the end of the method is not safe.
> 
> I've also changed another check for a frame_size == 0 to frame_size <= 0 to make it somewhat more safer.
> 
> The bug: http://bugs.sun.com/view_bug.do?bug_id=8008357
> Webrev: http://cr.openjdk.java.net/~rbackman/8008357/
> 
> This webrev is for HS24.
> 
> Thanks
> /R