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

[serguei.spitsyn at oracle.com]

Hi David,


Thank you for sharing your opinion!
I'm taking of my claim then. :)


On 12/5/16 22:07, David Holmes wrote:
> Hi Serguei,
>
> On 6/12/2016 3:57 PM, 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; }
>
> This saves _exception_state into *state.
>
>> + inline void restore_exception_state(ExceptionState state) {
>> _exception_state = state; }
>
> This restores _exception_state from state.
>
>> The functions above do not really
>> encapsulate and save/restore the _exception_state value so that these
>
> I think they do.

Just to explain.
Of course, I see the prefixes save_ and restore_. :)
I was concerned about the direction of saving/restoring.
The consumer of the api is some other class, not the class itself.
In this case, there is no encapsulation of the saved/restored value as
the value is stored away of the object that should manage it.
Then a misapplication is possible: one value is saved - another restored.

The examples I like are:

   interpreter/templateInterpreterGenerator.hpp:  void 
restore_native_result(void);
   interpreter/templateInterpreterGenerator.hpp:  void 
save_native_result(void);

   runtime/sharedRuntime.hpp:  static void 
restore_native_result(MacroAssembler *_masm, BasicType ret_type, int 
frame_slots);
   runtime/sharedRuntime.hpp:  static void 
save_native_result(MacroAssembler *_masm, BasicType ret_type, int 
frame_slots);

   services/threadService.hpp:    save_old_state(java_thread);

   opto/superword.hpp:    void store_depth()    {_depth_save = _depth;}
   opto/superword.hpp:    void restore_depth()  {_depth = _depth_save;}


However, it looks minor to me, maybe a matter of taste.
I'm silent now.


Thanks!
Serguei

>
> Cheers,
> David
> -----
>
>
>> 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
>>>
>>