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

[Doerr, Martin]

Hi Zoltan,

thank you very much for reviewing and for sponsoring.

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.
Please take a look.

New webrev is here:
http://cr.openjdk.java.net/~mdoerr/8176518_C2_NullCheck/webrev.01/

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();
>
>         }
>
>     }
>
> }
>