RFR: 8240530: CheckUnhandledOops breaks BacktraceBuilder::set_has_hidden_top_frame

[David Holmes]

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