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

Daniel D. Daugherty daniel.daugherty at oracle.com
Fri Sep 23 18:03:50 UTC 2016

On 9/23/16 11:16 AM, Frederic Parain wrote:
> Dan,
> Thank you for the review.
> My answers are in-lined below.
> On 09/15/2016 11:33 AM, Daniel D. Daugherty wrote:
>> 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
>> src/os/windows/vm/os_windows.cpp
>>     No comments on the assert removal.
>> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>>     No comments on the assert removal.
>> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>>     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.
> Done, comment has been added at each place where the assert has
> been removed.
>> 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?
> We support the reserved stack area only on x86 and SPARC, and
> the SPARC version of safe_for_sender() doesn't contain the checks
> that are causing issues on the x86 platform. So this fix is
> specific to the x86 platform.

If get_frame_at_stack_banging_point() on SPARC is only called
when we have well defined stacks, then the same conditions
apply for not needing to call safe_for_sender() first.

Updating get_frame_at_stack_banging_point() on SPARC to not call
safe_for_sender() will get rid of an unnecessary difference
between platforms...

> New webrev:
> http://cr.openjdk.java.net/~fparain/8146546/webrev.01/

    If you want, the second instance of the comment can be
    as simple as:

    // See java_sender() comment above.

    See java_sender() comment above. :-)

    See java_sender() comment above. :-)

       See java_sender() comment above. :-)

Thumbs up.


> Thank you,
> Fred

More information about the hotspot-runtime-dev mailing list