RFR: 8240530: CheckUnhandledOops breaks BacktraceBuilder::set_has_hidden_top_frame

[Stefan Karlsson]

Thanks, David!

StefanK

On 2020-03-10 08:09, David Holmes wrote:
> LGTM!
> 
> Thanks,
> David
> 
> On 10/03/2020 4:32 pm, Stefan Karlsson wrote:
>> Could I get a second review of the the patch:
>> https://cr.openjdk.java.net/~stefank/8240530/webrev.02
>>
>> Thanks!
>> StefanK
>>
>> On 2020-03-05 13:21, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 3/5/20 5:01 AM, Stefan Karlsson wrote:
>>>> On 2020-03-04 16:08, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I don't like the change to void* for _has_hidden_top_frame.
>>>>>
>>>>> Can you change this line to
>>>>>
>>>>>     if (_has_hidden_top_frame == _methods) {
>>>>>
>>>>> instead?
>>>>
>>>> I think you mean?
>>>> if (_has_hidden_top_frame != _methods) {
>>>>
>>>> That doesn't work for at least two reason:
>>>> 1) _has_hidden_top_frame is still being clobbered by CheckUnhandledOops
>>>> 2) _methods are being reinitialized and changed value in 
>>>> BacktraceBuilder::expand
>>>>
>>>>>
>>>>> Can you add an 'e' to Therefor
>>>>>
>>>>>       // It would be nice to add java/lang/Boolean::TRUE here
>>>>>       // to indicate that this backtrace has a hidden top frame.
>>>>>       // But this code is used before TRUE is allocated.
>>>>>       // Therefor let's just use an arbitrary legal oop
>>>>>       // available right here. We only test for != null
>>>>>       // anyways. _methods is a short[].
>>>>
>>>> Sure.
>>>>
>>>>>
>>>>> The reason that it's an oop is because the _backtrace is an 
>>>>> objArrayOop of java.lang.Object[6], and hidden boolean is 
>>>>> Object[5]. It's not a nice data structure but it needs to be fast.
>>>>
>>>> That's the reason why we put an object in trace_hidden_offset:
>>>>       _head->obj_at_put(trace_hidden_offset, <here>);
>>>>
>>>> but I'm specifically asking about the field _has_hidden_top_frame.
>>>>
>>>> I created a patch to show what I mean, to show that that field can 
>>>> simply be a bool:
>>>>  https://cr.openjdk.java.net/~stefank/8240530/webrev.02.delta
>>>>  https://cr.openjdk.java.net/~stefank/8240530/webrev.02
>>>>
>>>> It compiles and seems to work.
>>>
>>> Yes. That looks great.   We should have done that in the first place.
>>> Thanks,
>>> Coleen
>>>>
>>>> StefanK
>>>>
>>>>> Coleen
>>>>>
>>>>>
>>>>> On 3/4/20 9:48 AM, Stefan Karlsson wrote:
>>>>>> Hi all,
>>>>>>
>>>>>> Please review this patch to get rid of a unhandled oop null check 
>>>>>> in the BacktraceBuilder.
>>>>>>
>>>>>> https://cr.openjdk.java.net/~stefank/8240530/webrev.01/
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8240530
>>>>>>
>>>>>> The patch changes the type from oop to void* to prevent NULL oops 
>>>>>> from being overwritten with non-NULL values.
>>>>>>
>>>>>> I think an alternative fix would be to change the type of 
>>>>>> _has_hidden_top_frame to bool. Maybe someone familiar with this 
>>>>>> code knows why this was encoded with an oop NULL check instead of 
>>>>>> a plain bool?
>>>>>>
>>>>>> Testing with tier1-3 with CheckUnhandledOops on by default.
>>>>>>
>>>>>> StefanK
>>>>>
>>>>
>>>