RFR(m): 8074552: SafeFetch32 and SafeFetchN do not work in error handling

Thomas Stüfe thomas.stuefe at gmail.com
Wed Mar 11 17:10:29 UTC 2015


Hi Goetz,

thanks for reviewing!

Here the updated changes:

http://cr.openjdk.java.net/~stuefe/webrevs/8074552/webrev.03/webrev/

- I changed os_linux_sparc.cpp to reflect your comments: removed
set_cont_address() and replaced it with ucontext_set_pc(). However, I do
not have a linux sparc machine to build and test, so the change is made
blind. Hope it does not break.

- I removed the "CanUseSafeFetch.." calls from the initial test routine.
Originally I was afraid there were platforms where there are no
implementations for SafeFetch32 or SafeFetchN, but that does not seem to be
the case. Hopefully, I cannot speak for arm or whatever other closed
platforms are there.

Kind Regards, Thomas



On Wed, Mar 11, 2015 at 4:17 PM, Lindenmaier, Goetz <
goetz.lindenmaier at sap.com> wrote:

> Hi Thomas,
>
> I had a look at your change.  I'm fine with it, except for two minor
> things:
>
> os_linux_sparc.cpp
> Why didn't you just replace set_cont_address by ucontext_set_pc on
> linux_sparc?
> As it is now, ucontext_set_pc is just a rename of an existing function.
>
> stubRoutines.cpp:
> Why do you test for availability of the stubs when testing them?  I would
> consider
> it a bug if the stub is not generated at that point, and that would not be
> seen here
> as is now.
>
> Best regards,
>   Goetz.
>
>
>
> -----Original Message-----
> From: hotspot-runtime-dev [mailto:
> hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of Thomas Stüfe
> Sent: Dienstag, 10. März 2015 18:21
> To: David Holmes
> Cc: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(m): 8074552: SafeFetch32 and SafeFetchN do not work in
> error handling
>
> Hi David,
>
> see here my new patch:
>
> http://cr.openjdk.java.net/~stuefe/webrevs/8074552/webrev.02/webrev/
>
> Turns out that SafeFetch32 does not work in 32bit windows during
> initialization, because there is no SEH exception guard installed at that
> time (during the call to CreateJavaVM). Neither is there one for 64bit
> Windows, but there, we register SEH handlers for dynamic code, that hides
> the error.
>
> This is an issue I'd like to address with a separate patch (see
> https://bugs.openjdk.java.net/browse/JDK-8074860). For now, I deactivated
> the SafeFetch test during stub routine initialization for Windows 32bit.
>
> Other changes in the fix:
> - as you wished I changed the ULL literals to UCONST64
> - I activated the Jtreg test for windows too (SafeFetch works on windows
> during error handling, only not during initialization).
>
> Thomas
>
>
>
>
>
> On Mon, Mar 9, 2015 at 11:06 PM, David Holmes <david.holmes at oracle.com>
> wrote:
>
> > On 10/03/2015 2:30 AM, Thomas Stüfe wrote:
> >
> >> Hi David,
> >>
> >> I see now that only Windows 32bit is broken. I will investigate this.
> >>
> >
> > Yes, sorry, I hadn't noticed it was only 32-bit failing.
> >
> > Thanks,
> > David
> >
> >  Thomas
> >>
> >> On Mon, Mar 9, 2015 at 4:12 PM, Thomas Stüfe <thomas.stuefe at gmail.com
> >> <mailto:thomas.stuefe at gmail.com>> wrote:
> >>
> >>     Hi David,
> >>
> >>     I am sorry, but I am unable to reproduce the error. Both slowdebug
> >>     and fastdebug builds on Windows come up and SafeFetch works (also in
> >>     error handling). Are you sure the crash is caused by my change? If
> >>     yes, could you send me an hs-err file or at least the configure line
> >>     you used for building?
> >>
> >>     Kind Regards, Thomas
> >>
> >>     On Mon, Mar 9, 2015 at 7:51 AM, David Holmes
> >>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
> >>
> >>         On 9/03/2015 4:21 PM, Thomas Stüfe wrote:
> >>
> >>             Hi David,
> >>
> >>             Ouch. The only thing I can think about are the added
> >>             initialization
> >>             tests in stubRoutines.cpp. I will check this.
> >>
> >>
> >>         Ah! It hadn't registered with me that this test code was going
> >>         to be executed. Seems the likely cause as a clean repo has no
> >>         problems.
> >>
> >>         David
> >>
> >>             Kind Regards, Thomas
> >>
> >>
> >>
> >>             On Mon, Mar 9, 2015 at 6:56 AM, David Holmes
> >>             <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
> >>             <mailto:david.holmes at oracle.__com
> >>             <mailto:david.holmes at oracle.com>>> wrote:
> >>
> >>                  Hi Thomas,
> >>
> >>                  FYI When I applied this to latest hs-rt forest and ran
> >>             through JPRT
> >>                  the Windows fastdebug builds were DOA (instant segfault
> >>             just running
> >>                  "java -version"). Can't see how your changes could do
> >>             this so
> >>                  running a test on unmodified forest.
> >>
> >>                  David
> >>
> >>
> >>                  On 9/03/2015 9:40 AM, David Holmes wrote:
> >>
> >>                      Hi Thomas,
> >>
> >>                      Minor nit: in
> >>
> >>             src/share/vm/runtime/____stubRoutines.cpp::test_____
> >> safefetchN
> >>             can
> >>                      you use UCONST64 macro instead of explicit ULL -
> >>             thanks.
> >>
> >>                      Otherwise this seems okay, but we will also need
> >>             the aarch64
> >>                      component
> >>                      (and I'll look into our internal needs).
> >>
> >>                      Thanks,
> >>                      David
> >>
> >>                      On 7/03/2015 2:10 AM, Thomas Stüfe wrote:
> >>
> >>                          Hi all,
> >>
> >>                          could someone please review the following fix
> >>             (I also will
> >>                          need a
> >>                          sponsor):
> >>
> >>             http://cr.openjdk.java.net/~____stuefe/webrevs/8074552/
> >> webrev.____01/
> >>             <http://cr.openjdk.java.net/~__stuefe/webrevs/8074552/
> >> webrev.__01/>
> >>
> >>             <http://cr.openjdk.java.net/~__stuefe/webrevs/8074552/
> >> webrev.__01/
> >>             <http://cr.openjdk.java.net/~stuefe/webrevs/8074552/webrev.
> >> 01/>>
> >>
> >>             https://bugs.openjdk.java.net/____browse/JDK-8074552
> >>             <https://bugs.openjdk.java.net/__browse/JDK-8074552>
> >>
> >>
> >>             <https://bugs.openjdk.java.__net/browse/JDK-8074552
> >>
> >>             <https://bugs.openjdk.java.net/browse/JDK-8074552>>
> >>
> >>                          The fix will make SafeFetch[32,N] work in error
> >>             reporting.
> >>
> >>                          At SAP, we use SafeFetch a lot in error
> >>             reporting to poke
> >>                          around in
> >>                          potentially invalid memory (e.g. writing hex
> >>             dumps over
> >>                          areas which
> >>                          may be
> >>                          partly unmapped), and we feel that this could
> >>             be useful for the
> >>                          OpenJDK too.
> >>
> >>                          Without this fix, SafeFetch will cause a crash,
> >>             the current
> >>                          error
> >>                          reporting
> >>                          step will be interrupted and error reporting
> >>             will continue
> >>                          with the next
> >>                          step; that is not optimal because the
> >>             interrupted step may
> >>                          have shown
> >>                          valuable information.
> >>
> >>                          This fix handles SafeFetch faults during error
> >>             reporting the
> >>                          same way as
> >>                          they are handled normally. The changes are:
> >>
> >>                          - handle safe fetch fault in the various
> >>             (os_cpu dependend)
> >>                          secondary
> >>                          signal handlers
> >>
> >>                          - provide a function to check if it is safe to
> >>             use SafeFetch:
> >>                          CanUseSafeFetch32(). SafeFetch needs stub
> >>             routines and will
> >>                          crash when
> >>                          used
> >>                          before stub generation.
> >>
> >>                          - set_context_pc() is added which complements
> >>             the existing
> >>                          get_context_pc()
> >>                          and all instances where the pc in ucontext_t
> >>             was modified
> >>                          directly are
> >>                          changed to use set_context_pc()
> >>
> >>                          - in stubRoutines.cpp, a small test was added
> >>             to the already
> >>                          existing
> >>                          stub
> >>                          routines tests which run at VM init
> >>
> >>                          - in vmError.cpp, a test was added to test
> >>             SafeFetch during
> >>                          error
> >>                          reporting, similar to the tests introduced for
> >>             8065895
> >>
> >>                          - A JTreg test was added
> >>
> >>                          Thanks and Kind Regards,
> >>
> >>                          Thomas Stuefe
> >>
> >>
> >>
> >>
> >>
>


More information about the hotspot-runtime-dev mailing list