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

[Frederic Parain]

Dean,

Thank you for looking at this change.
I've contacted the JFR team, to know why these checks have been added,
but it seems that the only one who knows is Richard, and he's in
vacation until beginning of October.
If it is safe to remove this checks in safe_for_sender(), I would
prefer to see them being removed by someone from the JFR team,
because they were added by and for this team.

Regards,

Fred

On 09/15/2016 03:21 PM, dean.long at oracle.com wrote:
> Hi Frederic.  I see in the bug report that Tobias was wondering why
> safe_for_sender() checks for deoptimized frames on x86.  My guess is
> that it was to avoid bug JDK-8161598, which is now fixed.  If that's the
> case, then it would be safe to remove that code from safe_for_sender().
> It looks like Staffan added that code, so maybe he remembers the reason
> it was added.
>
> dl
>
>
> On 9/15/16 7: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
>>
>> Thank you,
>>
>> Fred
>