RFR(s): 8076185: Provide SafeFetchX implementation for zero

[Severin Gehwolf]

Hi Thomas,

On Tue, 2015-03-31 at 10:38 +0200, Thomas Stüfe wrote:
> Hi David.
> 
> 
> I am fine with that, thanks! 
> 
> 
> But Severin should give his ok, too.

You have my OK.

> We also should accept that my SafeFetch implementation will be slower
> than the standard one using inline assembly, because setjmp() does a
> lot of stores. As far as I can see now, we do not use SafeFetch
> extensivly anywhere, so this should be ok, but something to keep in
> mind. 

This is expected. The only use of SafeFetch I'm aware is in
src/share/vm/runtime/objectMonitor.cpp and the error handler. Speaking
of which: 

$ java -XX:+UnlockDiagnosticVMOptions -Xmx100M -XX:ErrorHandlerTest=14 -XX:+TestSafeFetchInErrorHandler -version

Does not yet work as expected. But that's a separate issue.

> So far, I only tested this on Linux x64. Because of the build error
> after 8074345, I was not able to test a product build of zero, only a
> slowdebug. I ran a number of JTReg tests with it and got a number of
> errors, but I got the same errors without my patch, so I assume the
> errors stem from the slowdebug VM just being too slow. 

JDK-8076181 should have fixed this. FWIW, I've built this with your
patch and JDK-8076181 using a product build and I've seen no issues.

Cheers,
Severin

> On Tue, Mar 31, 2015 at 3:45 AM, David Holmes
> <david.holmes at oracle.com> wrote:
>         Hi Thomas,
>         
>         If you are happy with Severin's review I can add my Review to
>         the shared part (Zero part looks okay too) and sponsor it.
>         
>         David
>         
>         
>         On 28/03/2015 4:00 AM, Thomas Stüfe wrote:
>                 Hi all,
>                 
>                 Please review this change which provides a real
>                 SafeFetch implementation
>                 on zero.
>                 
>                 webrev:
>                 http://cr.openjdk.java.net/~stuefe/webrevs/8076185/webrev.01/webrev/
>                 bug:
>                  https://bugs.openjdk.java.net/browse/JDK-8076185
>                 
>                 It works like this:
>                 
>                 Before a load is attempted from a potentially unsafe
>                 address, we set up a
>                 jump buffer with sigsetjmp(). In the signal handler,
>                 for SIGSEGV and SIGBUS,
>                 we test whether there is a jump buffer set and if yes,
>                 take this as an
>                 indication
>                 that the crash was an attempted SafeFetch. In this
>                 case we jump back via
>                 longjmp().
>                 
>                 Coding is a bit more difficult because we need to be
>                 threadsafe. We keep
>                 the jump
>                 buffer on the thread stack - this is ok, because that
>                 stack never gets
>                 unwinded
>                   - either we crash, in which case signal handler
>                 stack frames are built up
>                 below
>                 us, or we don't crash, in which case we never leave
>                 the SafeFetch function.
>                 In
>                 both cases, we never loose the jump buffer.
>                 
>                 To communicate the jump buffer location to the signal
>                 handler, TLS is used.
>                 I
>                 use POSIX tls, because that always works and is not
>                 dependend on VM
>                 infrastructure.
>                 
>                 ---
>                 
>                 I built and tested this on Linux x64 zero (Ubuntu
>                 14.4). It works and the
>                 initialization tests for SafeFetch in the stub routine
>                 initialization now
>                 work for
>                 zero too.
>                 
>                 Note that I do not have a BSD system right now, so I
>                 cannot check whether
>                 this
>                 change builds and works for BSD. But the change only
>                 requires POSIX Apis, so
>                 BSD should probably be ok.
>                 
>                 Someone from the zero team should definitly check and
>                 test this for other
>                 zero
>                 platforms, but the chances are good that this just
>                 works.
>                 
>                 Thanks and Kind Regards,
>                 
>                 Thomas Stuefe
>                 
> 
>