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

[Frederic Parain]

Gerard,

Thank you for your review.

Fred

On 09/23/2016 03:28 PM, Gerard Ziemski wrote:
> hi Frederic,
>
> Looks good.
>
>
> cheers
>
>> On Sep 23, 2016, at 12:17 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>>
>> Gerard,
>>
>> Thank you for reviewing this change.
>>
>> Good catch, I forgot the os_bsd_x86 platform, this is now fixed:
>>
>> http://cr.openjdk.java.net/~fparain/8146546/webrev.01/
>>
>> Fred
>>
>> On 09/15/2016 02:25 PM, Gerard Ziemski wrote:
>>> hi Frederic,
>>>
>>> Why aren’t we removing the assert, which you proposed, from os_bsd_x86.cpp?
>>>
>>>
>>> cheers
>>>
>>>
>>>> On Sep 15, 2016, at 9:44 AM, Frederic Parain <frederic.parain at oracle.com> 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
>>>>
>>>> Thank you,
>>>>
>>>> Fred
>>>
>