Quantcast

[9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi,

please review the following patch:
https://bugs.openjdk.java.net/browse/JDK-8173373

An unresolved field access on a null receiver throws a NPE instead of a NoClassDefFoundError if the method is compiled by C1.

The problem is that C1 inserts the null check before the field access that gets patched at runtime. The PatchingStub that would throw a NoClassDefFoundError is only executed after the null check:

  0x00007efc985eb2ec: movabs $0x0,%rsi          ;   {oop(NULL)}
  0x00007efc985eb2f6: cmp    (%rsi),%rax        ; implicit exception: dispatches to 0x00007efc985eb312
  [some nops]
  0x00007efc985eb300: jmpq   0x00007efc985eb415  ; implicit exception: dispatches to 0x00007efc985eb38e
  0x00007efc985eb305: nop                       ;*getfield f {reexecute=0 rethrow=0 return_oop=0}
                                                ; - compiler.c1.TestUnresolvedField::test@1

The jmpq jumps to the PatchingStub and is patched at runtime to a mov (see Runtime1::patch_code) with the correct field offset.

I think there are multiple ways to fix this:

1) We could omit the explicit null check when the receiver is unresolved at compile time and only insert the check after patching. This is complex and would require lots of changes to the patching code.

2) We could deoptimize if the class is not loaded and the receiver is null by setting CodeEmitInfo::_deoptimize_on_exception = true for the load:
http://cr.openjdk.java.net/~thartmann/8173373/webrev.00/
This has the disadvantage that we call Runtime1::predicate_failed_trap() which sets the nmethod to non-entrant.

3) We could emit an explicit null check if the receiver is not resolved at compile time and call the deoptimize stub with Action_none if the check fails:
http://cr.openjdk.java.net/~thartmann/8173373/webrev.01/
This way we don't make the nmethod non-entrant but have an explicit instead of an implicit null check.

What do you think?

Tested with regression test and RBT (running).

Thanks,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Andrew Haley
On 27/01/17 16:04, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8173373


Permission violation
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi Andrew,

On 27.01.2017 17:07, Andrew Haley wrote:
> Permission violation

Sorry, the bug is open now.

Best regards,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Vladimir Ivanov
In reply to this post by Tobias Hartmann-2

> http://cr.openjdk.java.net/~thartmann/8173373/webrev.01/

I prefer option #3.

Looks good.

But I'd prefer to see the check being inverted: !needs_patching =>
needs_patching.

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi Vladimir,

On 27.01.2017 18:29, Vladimir Ivanov wrote:
>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.01/
>
> I prefer option #3.
>
> Looks good.
>
> But I'd prefer to see the check being inverted: !needs_patching => needs_patching.

Thanks for the review! Here's the new webrev:
http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/

Best regards,
Tobias
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Vladimir Ivanov
> http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/

Looks good.

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Thanks, Vladimir!

Best regards,
Tobias

On 30.01.2017 13:48, Vladimir Ivanov wrote:
>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/
>
> Looks good.
>
> Best regards,
> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi,

I missed that the problem also affects putfield (of course). I extended the test accordingly and added the same fix to LIRGenerator::do_StoreField():
http://cr.openjdk.java.net/~thartmann/8173373/webrev.03/

Thanks,
Tobias

On 30.01.2017 13:51, Tobias Hartmann wrote:

> Thanks, Vladimir!
>
> Best regards,
> Tobias
>
> On 30.01.2017 13:48, Vladimir Ivanov wrote:
>>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/
>>
>> Looks good.
>>
>> Best regards,
>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Vladimir Ivanov
Good catch!

> I missed that the problem also affects putfield (of course). I extended the test accordingly and added the same fix to LIRGenerator::do_StoreField():
> http://cr.openjdk.java.net/~thartmann/8173373/webrev.03/

Can you try to factor new code into a helper method?

Otherwise, looks good.

Best regards,
Vladimir Ivanov

> On 30.01.2017 13:51, Tobias Hartmann wrote:
>> Thanks, Vladimir!
>>
>> Best regards,
>> Tobias
>>
>> On 30.01.2017 13:48, Vladimir Ivanov wrote:
>>>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/
>>>
>>> Looks good.
>>>
>>> Best regards,
>>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi Vladimir,

On 31.01.2017 13:25, Vladimir Ivanov wrote:
> Good catch!
>
>> I missed that the problem also affects putfield (of course). I extended the test accordingly and added the same fix to LIRGenerator::do_StoreField():
>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.03/
>
> Can you try to factor new code into a helper method?

Sure, what about adding a parameter to __ null_check() to force deoptimization?
http://cr.openjdk.java.net/~thartmann/8173373/webrev.04/

Thanks,
Tobias

> Otherwise, looks good.
>
> Best regards,
> Vladimir Ivanov
>
>> On 30.01.2017 13:51, Tobias Hartmann wrote:
>>> Thanks, Vladimir!
>>>
>>> Best regards,
>>> Tobias
>>>
>>> On 30.01.2017 13:48, Vladimir Ivanov wrote:
>>>>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.02/
>>>>
>>>> Looks good.
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Vladimir Ivanov

> Sure, what about adding a parameter to __ null_check() to force deoptimization?
> http://cr.openjdk.java.net/~thartmann/8173373/webrev.04/

I like it. Maybe deoptimize => deoptimize_on_null?

No need to send updated version.

Best regards,
Vladimir Ivanov
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [9] RFR(S): 8173373: C1: NPE is thrown instead of LinkageError when accessing inaccessible field on NULL receiver

Tobias Hartmann-2
Hi Vladimir,

On 31.01.2017 15:42, Vladimir Ivanov wrote:
>
>> Sure, what about adding a parameter to __ null_check() to force deoptimization?
>> http://cr.openjdk.java.net/~thartmann/8173373/webrev.04/
>
> I like it. Maybe deoptimize => deoptimize_on_null?

Okay, sounds better, I changed it in-place. Thanks for the review!

Best regards,
Tobias

>
> No need to send updated version.
>
> Best regards,
> Vladimir Ivanov
Loading...