RFR(m): 8074552: SafeFetch32 and SafeFetchN do not work in error handling
David Holmes
david.holmes at oracle.com
Thu Mar 12 23:36:22 UTC 2015
All looking good so pushing now.
Thanks,
David
On 12/03/2015 10:22 PM, David Holmes wrote:
> Hi Thomas,
>
> I'll try to re-test this internally in the morning. Sorry for the delay.
>
> David
>
> On 12/03/2015 3:10 AM, Thomas Stüfe wrote:
>> 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 <mailto: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
>> <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
>> <mailto: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 <mailto: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>
>> >> <mailto: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>
>> <mailto: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>>
>> >> <mailto:david.holmes at oracle.
>> <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