RFR: 8240530: CheckUnhandledOops breaks BacktraceBuilder::set_has_hidden_top_frame

[coleen.phillimore at oracle.com]

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