[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(M): JDK-8165496 assert(_exception_caught == false) failed: _exception_caught is out of phase
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-12-06 16:48:46
Message-ID: 7ae54cdf-1948-4340-d0ac-d92cc160364f () oracle ! com
[Download RAW message or body]

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.


Thanks,
Serguei

>
> -Dmitry
>
>
> On 2016-12-06 08:57, serguei.spitsyn@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
>>>
>

[prev in list] [next in list] [prev in thread] [next in thread]