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

[Coleen Phillimore]

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
>>>>
>>
>