RFR(M): JDK-8165496 assert(_exception_caught == false) failed: _exception_caught is out of phase

[Coleen Phillimore]

On 12/7/16 12:28 PM, Dmitry Samersoff wrote:
> Coleen,
>
>> This save_exception_state() is a bit unusually named (now that you
>> brought my attention to it).
> Changed. Agree, it looks better now.
>
> http://cr.openjdk.java.net/~dsamersoff/JDK-8165496/webrev.08/

This looks good to me.
Thanks,
Coleen

>
> -Dmitry
>
> On 2016-12-06 23:42, Coleen Phillimore wrote:
>>
>> On 12/6/16 11:48 AM, serguei.spitsyn at oracle.com wrote:
>>> Dmitry,
>>>
>>>
>>> On 12/6/16 01:54, Dmitry Samersoff wrote:
>>>> Serguei.
>>>>
>>>> I intentionally decide not to provide generic accessor functions
>>>> for _exception_state.
>>> Unfortunately, provided functions are the same functionally.
>>>
>>>
>>>> Developers should use set_exception_detected(), set_exception_caught(),
>>>>    clear_exception_state() but don't change _exception_state directly.
>>>>
>>>> And save_exception_state()/restore_exception_state() name and signature
>>>> clear indicates that the only valid usage of these functions is backup
>>>> of exception_state.
>>>>
>>>> So I would prefer to leave it as is.
>>> No pressure.
>> This save_exception_state() is a bit unusually named (now that you
>> brought my attention to it).  I think it would be more clear if:
>>
>> http://cr.openjdk.java.net/~dsamersoff/JDK-8165496/webrev.07/src/share/vm/prims/jvmtiExport.cpp.frames.html
>>
>>
>> 133 JvmtiThreadState::ExceptionState _exception_state;
>>
>>
>> This was
>>
>> 133 JvmtiThreadState::ExceptionState _saved_exception_state;
>>
>>
>> and your function that doesn't actually save the exception state was
>> called:
>>
>> 151 state->save_exception_state(&_exception_state);
>>
>> was
>>
>> 151 _saved_exception_state = state->get_exception_state();
>>
>>
>> This might be what David and Serguei suggested also.
>>
>> It is a minor point but looking at the function "save_exception_state",
>> it's surprising that it doesn't actually save the exception state in the
>> JvmtiThreadState type.
>>
>> Can you make these small changes?
>> thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2016-12-06 08:57, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Dmitry,
>>>>>
>>>>>
>>>>> I'm repeating my comments from the internal review.
>>>>>
>>>>> It looks pretty good to me.
>>>>> Thank you for the work on it!
>>>>>
>>>>> I'm not a big fun of new jvmtiThreadState functions though:
>>>>>
>>>>> + inline void save_exception_state(ExceptionState *state) { *state =
>>>>> _exception_state; }
>>>>> + inline void restore_exception_state(ExceptionState state) {
>>>>> _exception_state = state; } The functions above do not really
>>>>> encapsulate and save/restore the _exception_state value so that these
>>>>> names are kind of misleading. The same functionality can be provided
>>>>> with a simplified approach:     inline JvmtiThreadState
>>>>> exception_state() { return_exception_state;  }
>>>>>      inline void set_exception_state(ExceptionState state){
>>>>> _exception_state = state; }
>>>>>
>>>>>    I do not want to insist on my approach and will wait for other people
>>>>> comments on this.
>>>>> I hope, some compromise can be found here.
>>>>>
>>>>>
>>>>> On 12/5/16 01:30, Dmitry Samersoff wrote:
>>>>>> Everybody,
>>>>>>
>>>>>> Please review.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8165496/webrev.07/
>>>>>>
>>>>>> Bug link:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8165496
>>>>>>
>>>>>> Two separate flags, exception_detected and exception_caught, replaced
>>>>>> with one that changes it's state.
>>>>>>
>>>>>> Assert assert(_exception_caught == false) and fix for JDK-8134434 are
>>>>>> removed.
>>>>>>
>>>>>> Testing:
>>>>>>      1. Manual testing of instrumented hotspot
>>>>>>      2. Local ks with stress agent
>>>>>>      3. RBT ks with stress agent
>>>>> What the 'ks' stands for?
>>>>> I have some guess but still would like to know this abbreviation.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>>      4. RBT hotspot_all
>>>>>>
>>>>>> -Dmitry
>>>>>>
>