RFR(XXS): 8165014: Unaligned unsafe access should throw InternalError on Solaris

[Doerr, Martin]

Hi John,

thank you very much for the detailed background information.

I agree with that it is illegal to run such code on platforms which don't support unaligned accesses.
Unsafe provides a way to figure out if it's legal:
    /**
     * @return Returns true if this platform is capable of performing
     * accesses at addresses which are not aligned for the type of the
     * primitive type being accessed, false otherwise.
     */
    public final boolean unalignedAccess() { return unalignedAccess; }

The check seems to be missing in the test which uses Unsafe this way.

Best regards,
Martin


-----Original Message-----
From: John Rose [mailto:john.r.rose at oracle.com] 
Sent: Donnerstag, 1. September 2016 02:44
To: David Holmes <david.holmes at oracle.com>
Cc: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(XXS): 8165014: Unaligned unsafe access should throw InternalError on Solaris

On Aug 30, 2016, at 11:18 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Martin,
> 
> On 30/08/2016 8:42 PM, Doerr, Martin wrote:
>> Hi,
>> 
>> as discussed in http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-August/020888.html,
>> the signal handler on Solaris SPARC should throw a java.lang.InternalError when Unsafe accesses unaligned addresses.
>> This is currently broken because the signal handler only accepts BUS_OBJERR which is not sufficient.
>> 
>> Proposed fix:
>> http://cr.openjdk.java.net/~mdoerr/8165014_sparcSIGBUS/webrev.00/
>> 
>> Please review. I will also need a sponsor.
> 
> I can sponsor this for you.
> 
> I was unsure whether we should in fact cater for unaligned accesses, but causing InternalError to be thrown instead of crashing is somewhat cleaner.
> 
> Reviewed.

Yeah, that looks fine.  I don't know about other OS's but Solaris says
that touching a truncated file segment will always produce that error
code, and the trap handler is covering only that case, just to avoid
hiding other bugs in the JVM that might manifest as traps.

(Background:  If you map a file into a direct byte buffer, and then
an external agent truncates the file, Solaris will cause pages not
used by the shortened file to be mapped with no access, so as
to trap attempts to read beyond the last page of the file.  At least,
it used to work this way, when I wrote that code.)

Now that we have unaligned accesses in Unsafe we need to allow
through whatever error codes correspond to alignment failures.

And it's reasonable just to detune the logic; the only cost of that
will be to turn JVM crashes (which are very rare) into InternalErrors
(which will be mysterious in a different way than the crashes),
when the JVM falls over itself some other way, and it happens
to do so in JIT code that is marked as using Unsafe.

I will point out one thing:  If you are getting crashes from Unsafe
code, your first resort is not to ask the JVM to somehow make
it safe again.  Your first resort is to fix the bug that caused you
to hand bad inputs to Unsafe.  For the JVM to turn a crashing
Unsafe operation into an exception of any sort is a special favor.
The JVM is perfectly in its rights to crash if Unsafe is misused.

(More background:  Why not just crash the JVM in all cases?
After all, the user was playing with Unsafe.  Answer:  Yes, we
should not try to turn such programming errors into exceptions,
on the grounds that trying to make Unsafe safe again is a losing
battle.  If you use Unsafe, you had darn well better figure out
yourself how to pre-check the arguments to make them safe.
The case of file truncation is special, in that actions by other
processes, outside the control of the Java code, might cause
the truncation to happen, and we judged that an unavoidable
crash was so bad that an InternalError was warranted.  But
note that we didn't attempt to dress it up as a file-error of
some sort, or to make it a precise exception.  Precise
exceptions for this corner case would have hurt the
performance of all direct byte buffers.)

Finally, I hope we agree that the test is buggy if it is passing
misaligned addresses to aligned Unsafe access routines,
on CPUs that require alignment.  The InternalError is a special
favor to make such a buggy test a little easier to fix or refactor.
But if you pass a null pointer in such a test, you will probably
get a crash, probably not a NPE.  (NPE is not in the contract.)

HTH
- John