(S) 8176518: [9] C2: Invalid ImplicitNullChecks with non-protected heap base

Wed Mar 15 14:40:21 UTC 2017

Hi Martin,

On 03/15/2017 03:27 PM, Doerr, Martin wrote:
> Hi Zoltan,
>
> thank you very much for reviewing and for sponsoring.

thank you for spending time on this issue.

>
> I have updated the comments and added the test.
> We think it should be fine to run the test on all 64 bit platforms as it is fast and a little more testing in this area is not bad.
> If you don't agree, I can change it to run only on fewer platforms.

OK, let's run it on all platforms then.

> Please take a look.
>
> New webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8176518_C2_NullCheck/webrev.01/

The updated webrev looks good to me. I'll start pre-integration testing 
now and (if nothing unexpected happens) push the change afterwards. I'll 
keep you updated.

Thank you!

Best regards,

Zoltan

>
> Thanks and best regards,
> Martin
>
>
> -----Original Message-----
> From: Zoltán Majó [mailto:zoltan.majo at oracle.com]
> Sent: Mittwoch, 15. März 2017 11:07
> To: Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: (S) 8176518: [9] C2: Invalid ImplicitNullChecks with non-protected heap base
>
> Hi Martin,
>
>
> thank you for looking into this issue! Your fix looks good to me in
> general. Here are a few minor comments that you could maybe consider.
>
> - Could you please update the comment on line 261 to describe that we
> skip generating the implicit null check if the heap base is not protected?
>
>    260 !Universe::narrow_oop_use_implicit_null_checks()))
>    261           continue;             // Give up if offset is beyond
> page size
>
> - Could you please update the comment on line 279 to be more consistent
> with other comments in the same code region. Something along the lines
> "Give up if operation is a DecodeN and the heap base is not protected"
> could do. I don't see anything wrong with the check of the graph's shape.
>
> - Could you please change comment
>
>    278           continue;             // Give up i*s* reference is
> beyond 4K page size
>
> to
>
>    278           continue;             // Give up i*f* reference is
> beyond 4K page size
>
> - Could you please add the test case you have to the fix you're
> proposing? I'm not sure if you need to record the duration of the test's
> execution. Maybe you can reduce the number of iterations -- 20K could be
> sufficient. Could you please make the test throw an exception if the
> NullPointerException is not thrown? And finally, can you force the test
> to be executed only on AIX/Windows and with the parameters you mentioned
> in the RFR (i.e., -XX:HeapBaseMinAddress=32g on AIX and
> -XX:ObjectAlignmentInBytes=16 -XX:+UseLargePages on Windows)
>
> I can do the pre-integration testing for the change and then sponsor it.
>
> Thank you!
>
> Best regards,
>
>
> Zoltan
>
> On 03/14/2017 02:35 PM, Doerr, Martin wrote:
>> Hi,
>>
>> I have experimented on x86 and I get a very similar graph when using
>> -XX:ObjectAlignmentInBytes=16 (prevents oop decoding matched into
>> operands) in addition to the flags below under [1].
>>
>> If -XX:+UseLargePages (which is used to skip the heap base protection)
>> is supported on Windows, the problem exists there as well.
>>
>> As the graph pattern on PPC64 is the same as on x86, I believe that
>> there's nothing wrong except the missing check for
>> narrow_oop_use_implicit_null_checks(), which is added by the webrev below.
>>
>> The remaining question is if there's a better test than
>> "base->as_Mach()->ideal_Opcode() == Op_DecodeN".
>>
>> I will also need a sponsor for this change, please.
>>
>> Thanks and best regards,
>>
>> Martin
>>
>> *From:*hotspot-compiler-dev
>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] *On Behalf Of
>> *Doerr, Martin
>> *Sent:* Freitag, 10. März 2017 18:49
>> *To:* 'hotspot-compiler-dev at openjdk.java.net'
>> <hotspot-compiler-dev at openjdk.java.net>
>> *Subject:* RFR: (S) 8176518: [9] C2: Invalid ImplicitNullChecks with
>> non-protected heap base
>>
>> Hi,
>>
>> we have observed that C2 generates implicit null checks for write
>> accesses to the heap base even though it was not protected
>> (narrow_oop_use_implicit_null_checks = false).
>>
>> The problem showed up on AIX with -XX:HeapBaseMinAddress=32g.
>> ReservedHeapSpace::establish_noaccess_prefix protects the base area on
>> almost all platforms, but not on AIX (in some case).
>>
>> PhaseCFG::implicit_null_check needs to skip the transformation in this
>> case.
>>
>> The problem can be reproduced by the simple test program below under
>> [1]. The null pointer exception is just missing when running with the
>> given parameters.
>>
>> The problem can be prevented by applying this patch:
>>
>> http://cr.openjdk.java.net/~mdoerr/8176518_C2_NullCheck/webrev.00/
>> <http://cr.openjdk.java.net/%7Emdoerr/8176518_C2_NullCheck/webrev.00/>
>>
>> Especially, the case "base->is_Mach() &&
>> base->as_Mach()->ideal_Opcode() == Op_DecodeN" gets hit. I'm not sure
>> if this is allowed at that place. Maybe something went wrong before.
>>
>> The node "base" is a storeI and "addr"="base" is a decodeN node (the
>> "index" retrieved by mach->memory_inputs is NULL).
>>
>> The patch helps, but is there a better way to fix the problem?
>>
>> Thanks and best regards,
>>
>> Martin
>>
>> [1] Reproduction case:
>>
>> /jdk/bin/java -XX:HeapBaseMinAddress=32g -XX:-TieredCompilation
>> -Xbatch TestWriteNPE
>>
>> *public**class*TestWriteNPE{
>>
>> TestWriteNPE instance = *null*;
>>
>> *int*i = 0;
>>
>> *public**void*bogus_test(*int*value) {
>>
>> instance.i = value;
>>
>>      }
>>
>> *static**final**int*loop_cnt=100000;
>>
>> *public**static**void*main(String args[]){
>>
>> TestWriteNPE xyz=*new*TestWriteNPE();
>>
>> xyz.instance = xyz;
>>
>> *long*duration = System.nanoTime();
>>
>> *for*(*int*x=0; x<loop_cnt; x++) xyz.bogus_test(x);
>>
>> duration = System.nanoTime() - duration;
>>
>> System.out.println("value: "+ xyz.i + " (duration: "+
>> duration/loop_cnt + ")");
>>
>> xyz.instance = *null*;
>>
>> *try*{
>>
>> xyz.bogus_test(0);
>>
>> } *catch*(NullPointerException np) {
>>
>> System.out.println("Got NPE:");
>>
>> np.printStackTrace();
>>
>>          }
>>
>>      }
>>
>> }
>>