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

[Frederic Parain]

Dan,

Thank you for your review, I've updated the comments
as you suggested.

fRed

On 09/23/2016 02:03 PM, Daniel D. Daugherty wrote:
> 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/
>
> src/os/windows/vm/os_windows.cpp
>    If you want, the second instance of the comment can be
>    as simple as:
>
>    // See java_sender() comment above.
>
> src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
>    See java_sender() comment above. :-)
>
> src/os_cpu/linux_x86/vm/os_linux_x86.cpp
>    See java_sender() comment above. :-)
>
> src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
>       See java_sender() comment above. :-)
>
> Thumbs up.
>
> Dan
>
>
>>
>> Thank you,
>>
>> Fred
>