[10] RFR(M): 8176506: C2: loop unswitching and unsafe accesses cause crash

[Vladimir Kozlov]

Looks good to me. I will submit RBT testing, lets see how it goes.

Thanks,
Vladimir

On 4/28/17 1:46 AM, Roland Westrelin wrote:
>
>> In addition to Andrew’s test i have a simple JMH microbenchmark i can
>> send you, which i used to measure performance and identify issues with
>> code generation, misalignment, and unrolled loops for byte buffer
>> access (off/on heap) and VarHandle views. specjvm2008 is likely too
>> coarse grained.
>
> I found some regressions with Paul's tests but I can recover lost
> performance using profiling data and some tweaks to the compiler. I'll
> take care of all that as part of a follow up change.
>
> Here is a new webrev:
>
> http://cr.openjdk.java.net/~roland/8176506/webrev.03/
>
> I found I introduced a bug in LibraryCallKit::make_unsafe_address(): in
> case of a raw memory access classify_unsafe_addr() changes base but we
> don't want that change to propagate to
> LibraryCallKit::inline_unsafe_access() otherwise it won't see a null
> base for a raw memory access.
>
> If C2 compiles a method that makes incorrect use of Unsafe (by trying to
> load a field from a null object for instance), the C2 HaltNode can be
> executed. Its current implementation on several platforms result in a
> call to os::breakpoint() which is an empty method. So execution would
> happily return to the compiled method and continue and fail in some
> mysterious way. That's why in my previous webrevs, I had implemented the
> HaltNode on x86 and aarch64 as a call to a method that call
> ShouldNotReachHere(). This needs to be taken care of on all
> platforms. The webrev now includes an implementation on arm64 (that I
> tested), arm32 and sparc (that I can't build or test). I would need
> someone to verify them. I suppose PPC would need something similar as
> well but I have limited PPC skills so I would need code for that
> too. The easy way to test the halt implementation is to modify the
> TestMaybeNullUnsafeAccess test from the patch and add a:
>
> test2(null);
>
> call after the loop of the main method. This should result in the Halt
> node being executed.
>
> Roland.
>