RFR: 8240530: CheckUnhandledOops breaks BacktraceBuilder::set_has_hidden_top_frame

[Stefan Karlsson]

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.

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
>