RFR: 8240530: CheckUnhandledOops breaks BacktraceBuilder::set_has_hidden_top_frame

[Stefan Karlsson]

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