Skip to content

Conversation

@adinn
Copy link
Collaborator

@adinn adinn commented Jun 19, 2017

The Problem:
UseTrappingNullChecksPhase checks to see whether a read from an object field is guarded by an IfNode/IsNullNode and, where possible, converts the read to use an implicit null check and splices the If out of the graph. The check involves identifying whether the oop address tested by the IsNullNode equals the oop base address used by the ReadNode. When using compressed oops this can lead to failures on architectures other than x86.

If the oop value in question is a narrow oop then the IsNullNode is passed a CompressionNode. It dereferences this to store the underlying NarrowOop value as the target value to be tested for null. If this same CompressionNode is used as the base or index for the ReadNode then the comparison will not succeed unless the address lowering can optimize the address to strip the CompressionNode and employ the underlying narrowoop as it's base or index, with a suitable base + index addressing mode.

x86 is able to do this because it can generate offset loads which also apply a shift to the input address. AArch64 can use either a base + shifted index register addressing mode or a base + offset mode but cannot combine the two (I suspect the same problem arises on SPARC?). As a result the addresss equality check always fails on AArch64 causing a NullCheckNode to be inserted. The NullCheckNode is converted to an uncompress and explicit load via the resulting address. to trigger a SEGV in case the oop is null. What is worse, the following read(s) re-execute the uncompress.

The Fix:
The fix changes the comparison to detect the situation where the input base or index address to the ReadNode is a CompressionNode, indirecting through it to locate the underlying NarrowOop base/index. This ensures that a corresponding IsNullNode guard is correctly detected even when CompressionNode folding is not possible.

Testing:
I detected this problem by eyeballing the code generated for String::equals on AArch64 and x86 where StringLatin1::equals had been inlined (using compileOnly,java.lang.String*::equals). The reads of the underlying byte arrays are both implicit null candidates.

On x86 there is no difference in before and after. On AArch64 before the fix each byte array load was preceded by an uncompress, a null check load (with offset 0) and a 2nd uncompress feeding the byte array load (with offset 12). After the fix there was only a single uncompress preceding the byte array load. The latter served as an implicit null check (preceding uncompress was tagged with an OopMap).

I also reran unit tests on Aarch64 with no problems detected.

@adinn adinn force-pushed the trapping_null_check_patch branch 2 times, most recently from ca61b52 to edf0480 Compare June 23, 2017 13:56
@adinn adinn force-pushed the trapping_null_check_patch branch from edf0480 to a441d07 Compare June 30, 2017 07:55
@adinn
Copy link
Collaborator Author

adinn commented Jun 30, 2017

This PR appears to be held up by a problem with Travis (looks like it still cannot find a suitable JDK9 to run with). It did build and run ok when first submitted. Is there any chance of getting the Travis build fixed so I can get this PR accepted. It's been sitting waiting on review and pull for 11 days now.

@adinn adinn force-pushed the trapping_null_check_patch branch 2 times, most recently from 796fafc to c949137 Compare July 4, 2017 10:07
@adinn
Copy link
Collaborator Author

adinn commented Jul 4, 2017

This PR s now ready to pull again. Any chance of a review?

@adinn adinn force-pushed the trapping_null_check_patch branch 2 times, most recently from 7545d43 to f9ff8d6 Compare July 12, 2017 15:47
@adinn adinn force-pushed the trapping_null_check_patch branch from d717b41 to bf41796 Compare July 20, 2017 14:43
@tkrodriguez tkrodriguez self-requested a review July 20, 2017 17:36
@tkrodriguez
Copy link
Member

This looks ok to me. I'll push it.

@dougxc dougxc merged commit bf0b6fa into oracle:master Jul 20, 2017
@adinn adinn deleted the trapping_null_check_patch branch July 21, 2017 10:24
@dougxc dougxc added the accept label Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants