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

[David Holmes]

On 7/12/2016 6:42 AM, 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.

This is what Serguei was discussing. The current API is for saving and 
restoring the exception-state to/from an external location; it isn't 
asking JvmtiThreadState to internally save/restore its own state. I 
think this distinction is more than just a matter of style, but a matter 
of semantics - in particular whether multiple saves can be applied in 
sequence: an external API handles that fine, while the internal API 
would have to implement a stack of saved values. I'm not clear what is 
required in the current context.

David

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