RFR(S): JDK-8146546 assert(fr->safe_for_sender(thread)) failed: Safety check

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu Sep 15 15:33:41 UTC 2016

On 9/15/16 8:44 AM, Frederic Parain wrote:
> Greetings,
> Please review this small fix for bug JDK-8146546:
> https://bugs.openjdk.java.net/browse/JDK-8146546
> Initial bug report is about an assertion failure in the reserved
> stack code. The failing assertion calls safe_for_sender() after
> the reconstruction of the first frame to initiate the stack
> walking.
> After investigation, it appears that the issue is that
> safe_for_sender() is used for different purposes in different contexts.
> JFR uses this method to check if it is safe to walk the stack, if the
> method returns false, JFR simply records the current event without
> stack information. JFR has to be very conservative on the conditions to
> be satisfied to safely walk the stack, because JFR events could occur
> at any time.
> In the current case, safe_for_sender() is not called by JFR, but by the
> reserved stack management code. The implementation of the reserved
> stack requires to walk the stack too, but always on well defined points
> in execution: when the stack banging is performed to detect potential
> stack overflow ahead of time. Because the reserved stack code knows
> exactly the state of the stack when it has to browse it, it has less
> constraints than the JFR code. The condition that makes
> safe_for_sender() to return false here, and by consequence causes the
> assertion failure, are harmless for the reserved stack code.
> Removing the condition in safe_for_sender() doesn't seem a good idea,
> as it could be harmful for JFR code.
> Modifying safe_for_sender() to support both usages would make this
> method even more ugly.
> However, removing the assertion in the reserved stack code would be
> harmless, this is the solution proposed by this fix:
> http://cr.openjdk.java.net/~fparain/8146546/webrev.00/index.html

     No comments on the assert removal.

     No comments on the assert removal.

     No comments on the assert removal.

We're used to seeing this code pattern in stack walking:

     assert(fr->safe_for_sender(thread), "Safety check");
     *fr = fr->java_sender();

so seeing a java_sender() call without the assert is a bit
troubling without an explanation. Perhaps you can add a
comment like this above each get_frame_at_stack_banging_point()
function definition:

     // get_frame_at_stack_banging_point() is only called when we
     // have well defined stacks so java_sender() calls do not need
     // to assert safe_for_sender() first.

And if we are really saying that the above comment is true, then
hotspot/src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp should
also be updated... Are there others? Are they also safe?


> Thank you,
> Fred

More information about the hotspot-runtime-dev mailing list