RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Wed Dec 5 19:36:38 UTC 2018
Hi all,
My apologies to having to come back for another review for this change: I
ran into a snag when trying to pull the latest changes compared to the base
I was working on. I basically forgot that there was an issue with snprintf
and that I had solved it via JDK-8213622.
Could I have a new review of this webrev:
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.04/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
Incremental from the port of webrev.03 that got LGTMs:
http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a_04/
A few comments on this because it took me a while to get things in a state
I thought was good:
- I had to implement an itoa method, do we have something like that in
the test base (remember that JDK-8213622 could not use sprintf due to being
in the test code)?
- The differences here compared to the one you all reviewed are:
- I found that adding to the strlen/memcpy error prone and thought
that I would try to make it less so. If you want to compare, I extended the
strlen/memcpy with the new format to show you if you prefer [1]
- Note that the diff between the "old extended way from [1]" to
the webrev.04 can be found in [2]
- I added a test to test the exception wrapper in tests :); I'm not
sure it is deemed useful or not but helped me assure myself that I was not
doing things wrong; you can find the base test file here [3]; should we
have this or not? (I know that normally we don't add tests to vmTestbase
but thought this might be an exception)
Thanks for your help and my apologies for the snag,
Jc
[1]:
http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
[2]: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a_04
[3]
http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/ExceptionCheckingJniEnv/exceptionjni001/exceptionjni001.cpp.html
On Mon, Dec 3, 2018 at 11:29 PM David Holmes <david.holmes at oracle.com>
wrote:
> Looks fine to me.
>
> Thanks,
> David
>
> On 4/12/2018 4:04 pm, JC Beyler wrote:
> > Hi both,
> >
> > Thanks for the reviews! Since Serguei did not insist on get_basename, I
> > went for get_dirname since the method is a local static method and won't
> > have its name start spreading, I think it's ok too.
> >
> > For the naming of the local variable, the idea initially was to use the
> > same name as the local variable for JNIEnv already used to reduce the
> > code change. Since I'm now adding the line macro at the end anyway, this
> > does not matter anymore so I converged all local variables to "jni".
> >
> > So, without further ado, here is the new version:
> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
> >
> > This passes the various tests changed by the webrev on my dev machine.
> >
> > Let me know what you think,
> > Jc
> >
> > On Mon, Dec 3, 2018 at 8:40 PM serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com> <serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>> wrote:
> >
> > On 12/3/18 20:15, Chris Plummer wrote:
> > > Hi JC,
> > >
> > > Overall it looks good. A few naming nits thought:
> > >
> > > In bi01t001.cpp, why have you declared the
> > ExceptionCheckingJniEnvPtr
> > > using jni_env(jni). Elsewhere you use jni(jni_env) and rename the
> > > method argument passed in from jni to jni_env.
> > >
> > > Related to this, I also noticed in some files that already are
> using
> > > ExceptionCheckingJniEnvPtr, such as CharArrayCriticalLocker.cpp,
> you
> > > delcared it as env(jni_env). So that means there are 3 different
> > names
> > > you have used for the ExceptionCheckingJniEnvPtr local variable.
> > They
> > > should be consistent.
> > >
> > > Also, can you rename get_basename() to get_dirname()? I know
> Serguei
> > > suggested get_basename() a while back, but unless "basename" is
> > > commonly used for this purpose, I think "dirname" is more self
> > > explanatory.
> >
> > In general, I'm Okay with get_dirname().
> > Just to mention dirname can be both short or full, so it is a little
> > confusing as well.
> > It is the reason why the get_basename() was suggested.
> > However, I do not insist on get_basename() nor get_full_dirname(). :)
> >
> > Thanks,
> > Serguei
> >
> >
> > > thanks,
> > >
> > > Chris
> > >
> > > On 12/2/18 10:29 PM, David Holmes wrote:
> > >> Hi Jc,
> > >>
> > >> I've been lurking on this one and have had a look through. I'm
> okay
> > >> with the FatalError approach for the tests - we don't expect
> > anything
> > >> to go wrong in a well written test in a correctly functioning VM.
> > >>
> > >> Thanks,
> > >> David
> > >>
> > >>
> > >>
> > >> On 3/12/2018 3:24 pm, JC Beyler wrote:
> > >>> Hi all,
> > >>>
> > >>> Would someone on the GC or runtime team be motivated to give
> > this a
> > >>> review? :)
> > >>>
> > >>> It would be much appreciated!
> > >>>
> > >>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
> > >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
> > >>>
> > >>> Thanks for your help,
> > >>> Jc
> > >>>
> > >>> On Tue, Nov 27, 2018 at 4:36 PM JC Beyler <jcbeyler at google.com
> > <mailto:jcbeyler at google.com>
> > >>> <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>>
> wrote:
> > >>>
> > >>> Hi Chris,
> > >>>
> > >>> Yes I was waiting for another review since you had
> explicitly
> > >>> asked :)
> > >>>
> > >>> And sounds good that when someone from GC or runtime gives a
> > >>> review,
> > >>> I'll wait for your full review on the webrev.02!
> > >>>
> > >>> Thanks again for your help,
> > >>> Jc
> > >>>
> > >>>
> > >>> On Tue, Nov 27, 2018 at 12:48 PM Chris Plummer
> > >>> <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>
> > <mailto:chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>>>
> > wrote:
> > >>>
> > >>> Hi JC,
> > >>>
> > >>> I think it would be good to get a review from the gc or
> > runtime
> > >>> teams, since this also affects their tests.
> > >>>
> > >>> Also, once we are settled on this FatalError approach,
> > I still
> > >>> need to give your webrev-02 a full review. I only
> > skimmed over
> > >>> parts of it (I did look at all the changes in
> webrevo-01).
> > >>>
> > >>> thanks,
> > >>>
> > >>> Chris
> > >>>
> > >>> On 11/27/18 8:58 AM, serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>
> > >>> <mailto:serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>> wrote:
> > >>>> Hi Jc,
> > >>>>
> > >>>> I've already reviewed this too.
> > >>>>
> > >>>> Thanks,
> > >>>> Serguei
> > >>>>
> > >>>>
> > >>>> On 11/27/18 06:56, JC Beyler wrote:
> > >>>>> Thanks Chris,
> > >>>>>
> > >>>>> Anybody else motivated to look at this and review it?
> :)
> > >>>>> Jc
> > >>>>>
> > >>>>> On Mon, Nov 26, 2018 at 1:26 PM Chris Plummer
> > >>>>> <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com> <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>>
> > >>>>> wrote:
> > >>>>>
> > >>>>> Hi JC,
> > >>>>>
> > >>>>> I'm ok with the FatalError approach, but would
> > like to
> > >>>>> hear opinions from others also.
> > >>>>>
> > >>>>> thanks,
> > >>>>>
> > >>>>> Chris
> > >>>>>
> > >>>>> On 11/21/18 8:19 AM, JC Beyler wrote:
> > >>>>>> Hi Chris,
> > >>>>>>
> > >>>>>> Thanks for taking the time to look at it and yes
> you
> > >>>>>> have raised exactly why the webrev is between two
> > >>>>>> worlds: in cases where a fatal error on failure
> is
> > >>>>>> wanted, should we simplify the code to remove
> > the return
> > >>>>>> tests since we do them internally? Now that I've
> > looked
> > >>>>>> around for non-fatal cases, I think the answer
> > is yes,
> > >>>>>> it simplifies the code while maintaining the
> checks.
> > >>>>>>
> > >>>>>> I looked a bit and it seems that I can't find
> > easily a
> > >>>>>> case where the test accepts a JNI failure to
> > then move
> > >>>>>> on. Therefore, perhaps, for now, the fail with a
> > Fatal
> > >>>>>> is enough and we can work on the tests to clean
> > them up?
> > >>>>>>
> > >>>>>> That means that this is the new webrev with only
> > Fatal
> > >>>>>> and cleans up the tests so that it is no longer
> in
> > >>>>>> between two worlds:
> > >>>>>>
> > >>>>>> Webrev:
> > >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
> > >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/>
> > >>>>>> Bug:
> > https://bugs.openjdk.java.net/browse/JDK-8213501
> > >>>>>>
> > >>>>>> (This passes testing on my dev machine for all
> the
> > >>>>>> modified tests)
> > >>>>>>
> > >>>>>> with the example you provided, it now looks like:
> > >>>>>>
> >
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> > >>>>>>
> > >>>>>>
> > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >
> > >>>>>>
> > >>>>>>
> > >>>>>> Where it does, to me at least, seem cleaner and
> less
> > >>>>>> "noisy".
> > >>>>>>
> > >>>>>> Let me know what you think,
> > >>>>>> Jc
> > >>>>>>
> > >>>>>>
> > >>>>>> On Tue, Nov 20, 2018 at 9:33 PM Chris Plummer
> > >>>>>> <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>
> > >>>>>> <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>> wrote:
> > >>>>>>
> > >>>>>> Hi JC,
> > >>>>>>
> > >>>>>> Sorry about the delay. I had to go back an
> > look at
> > >>>>>> the initial 8210842 webrev and RFR thread to
> see
> > >>>>>> what this was initially all about.
> > >>>>>>
> > >>>>>> In general the changes look good.
> > >>>>>>
> > >>>>>> I don't have a good answer to your
> > >>>>>> FatalError/NonFatalError question. It makes
> > the code
> > >>>>>> a lot cleaner to use FatalError, but then it
> > is a
> > >>>>>> behavior change, and you also need to deal
> with
> > >>>>>> tests that intentionally induce errors (do
> > you have
> > >>>>>> an example of that).
> > >>>>>>
> > >>>>>> In any case, right now your webrev seems to
> be
> > >>>>>> between two worlds. You are producing
> > FatalError,
> > >>>>>> but still checking results. Here's a good
> > example:
> > >>>>>>
> > >>>>>>
> >
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> > >>>>>>
> > >>>>>>
> > <
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >
> > >>>>>>
> > >>>>>>
> > >>>>>> I'm not sure if this is just a temporary
> > state until
> > >>>>>> it was decided which approach to take.
> > >>>>>>
> > >>>>>> thanks,
> > >>>>>>
> > >>>>>> Chris
> > >>>>>>
> > >>>>>>
> > >>>>>> On 11/20/18 2:14 PM, JC Beyler wrote:
> > >>>>>>> Hi all,
> > >>>>>>>
> > >>>>>>> Chris thought it made sense to have more
> > eyes on
> > >>>>>>> this change than just serviceability as it
> will
> > >>>>>>> modify to tests that are not only
> > serviceability
> > >>>>>>> tests so I've moved this to conversation
> > here :)
> > >>>>>>>
> > >>>>>>> For convenience, I've copy-pasted the
> > initial RFR:
> > >>>>>>>
> > >>>>>>> Could I have a review for the extension and
> > usage
> > >>>>>>> of the ExceptionJniWrapper. This adds lines
> and
> > >>>>>>> filenames to the end of the wrapper JNI
> > methods,
> > >>>>>>> adds tracing, and throws an error if need
> > be. I've
> > >>>>>>> ported the gc/lock files to use the new
> > >>>>>>> TRACE_JNI_CALL add-on and I've ported a few
> > of the
> > >>>>>>> tests that were already changed for the
> > assignment
> > >>>>>>> webrev for JDK-8212884.
> > >>>>>>>
> > >>>>>>> Webrev:
> > >>>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01
> > >>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01>
> > >>>>>>> Bug:
> > >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> > >>>>>>>
> > >>>>>>> For illustration, if I force an error to the
> > >>>>>>> AP04/ap04t03 test and set the verbosity on,
> > I get
> > >>>>>>> something like:
> > >>>>>>>
> > >>>>>>> >> Calling JNI method FindClass from
> > >>>>>>> ap04t003.cpp:343
> > >>>>>>> >> Calling with these parameter(s):
> > >>>>>>> java/lang/Threadd
> > >>>>>>> Wait for thread to finish
> > >>>>>>> << Called JNI method FindClass from
> > >>>>>>> ap04t003.cpp:343
> > >>>>>>> Exception in thread "Thread-0"
> > >>>>>>> java.lang.NoClassDefFoundError:
> > java/lang/Threadd
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> > >>>>>>>
> > >>>>>>> Method)
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >
> > >>>>>>>
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >
> > >>>>>>>
> > >>>>>>> Caused by: java.lang.ClassNotFoundException:
> > >>>>>>> java.lang.Threadd
> > >>>>>>> at
> > >>>>>>>
> >
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
> >
> > >>>>>>>
> > >>>>>>> at
> > >>>>>>>
> >
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
> >
> > >>>>>>>
> > >>>>>>> at
> > >>>>>>>
> java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
> > >>>>>>> ... 3 more
> > >>>>>>> FATAL ERROR in native method: JNI method
> > FindClass
> > >>>>>>> : internal error from ap04t003.cpp:343
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> > >>>>>>>
> > >>>>>>> Method)
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >
> > >>>>>>>
> > >>>>>>> at
> > >>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> Questions/comments I have about this are:
> > >>>>>>> - Do we want to force fatal errors when a
> JNI
> > >>>>>>> call fails in general? Most of these tests
> > do the
> > >>>>>>> right thing and test the return of the JNI
> > calls,
> > >>>>>>> for example:
> > >>>>>>> thrClass =
> > jni->FindClass("java/lang/Threadd",
> > >>>>>>> TRACE_JNI_CALL);
> > >>>>>>> if (thrClass == NULL) {
> > >>>>>>>
> > >>>>>>> but now the wrapper actually would do a
> > fatal if
> > >>>>>>> the FindClass call would return a nullptr,
> > so we
> > >>>>>>> could remove that test altogether. What do
> you
> > >>>>>>> think?
> > >>>>>>> - I prefer to leave them as the tests
> then
> > >>>>>>> become closer to what real users would have
> in
> > >>>>>>> their code and is the "recommended" way of
> > doing it
> > >>>>>>>
> > >>>>>>> - The alternative is to use the
> > NonFatalError I
> > >>>>>>> added which then just prints out that
> something
> > >>>>>>> went wrong, letting the test continue.
> Question
> > >>>>>>> will be what should be the default? The
> > fatal or
> > >>>>>>> the non-fatal error handling?
> > >>>>>>>
> > >>>>>>> On a different subject:
> > >>>>>>> - On the new tests, I've removed the
> > >>>>>>> NSK_JNI_VERIFY since the JNI wrapper
> > handles the
> > >>>>>>> tracing and the verify in almost the same
> > way; only
> > >>>>>>> difference I can really tell is that the
> > complain
> > >>>>>>> method from NSK has a max complain before
> > stopping
> > >>>>>>> to "complain"; I have not added that part
> > of the
> > >>>>>>> code in this webrev
> > >>>>>>>
> > >>>>>>> Once we decide on these, I can continue on
> the
> > >>>>>>> files from JDK-8212884 and then do both the
> > >>>>>>> assignment in an if extraction followed-by
> this
> > >>>>>>> type of webrev in an easier fashion.
> > Depending on
> > >>>>>>> decisions here, NSK*VERIFY can be
> deprecated as
> > >>>>>>> well as we go forward.
> > >>>>>>>
> > >>>>>>> Thanks!
> > >>>>>>> Jc
> > >>>>>>>
> > >>>>>>> On Mon, Nov 19, 2018 at 11:34 AM Chris
> Plummer
> > >>>>>>> <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>
> > >>>>>>> <mailto:chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>>> wrote:
> > >>>>>>>
> > >>>>>>> On 11/19/18 10:07 AM, JC Beyler wrote:
> > >>>>>>>> Hi all,
> > >>>>>>>>
> > >>>>>>>> @David/Chris: should I then push this
> > RFR to
> > >>>>>>>> the hotspot mailing or the runtime
> > one? For
> > >>>>>>>> what it's worth, a lot of the tests
> > under the
> > >>>>>>>> vmTestbase are jvmti so the review also
> > >>>>>>>> affects serviceability; it just turns
> > out I
> > >>>>>>>> started with the GC originally and
> > then hit
> > >>>>>>>> some other tests I had touched via the
> > >>>>>>>> assignment extraction.
> > >>>>>>> I think hotspot would be best.
> > >>>>>>>
> > >>>>>>> Chris
> > >>>>>>>>
> > >>>>>>>> @Serguei: Done for the method
> > renaming, for
> > >>>>>>>> the indent, are you talking about
> > going from
> > >>>>>>>> the 8-indent to 4-indent? If so, would
> > it not
> > >>>>>>>> just be better to do a new JBS bug and
> > do the
> > >>>>>>>> whole files in one go? I ask because
> > >>>>>>>> otherwise, it will look a bit weird to
> > have
> > >>>>>>>> parts of the file as 8-indent and
> others
> > >>>>>>>> 4-indent?
> > >>>>>>>>
> > >>>>>>>> Thanks for looking at it!
> > >>>>>>>> Jc
> > >>>>>>>>
> > >>>>>>>> On Mon, Nov 19, 2018 at 1:25 AM
> > >>>>>>>> serguei.spitsyn at oracle.com <mailto:
> serguei.spitsyn at oracle.com>
> > >>>>>>>> <mailto:serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>>
> > >>>>>>>> <serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>
> > >>>>>>>> <mailto:serguei.spitsyn at oracle.com
> > <mailto:serguei.spitsyn at oracle.com>>> wrote:
> > >>>>>>>>
> > >>>>>>>> Hi Jc,
> > >>>>>>>>
> > >>>>>>>> We have to start this review
> > anyway. :)
> > >>>>>>>> It looks good to me in general.
> > >>>>>>>> Thank you for your consistency in
> this
> > >>>>>>>> refactoring!
> > >>>>>>>>
> > >>>>>>>> Some minor comments.
> > >>>>>>>>
> > >>>>>>>>
> >
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
> >
> > >>>>>>>>
> > >>>>>>>> +static const char*
> > remove_folders(const
> > >>>>>>>> char* fullname) { I'd suggest to
> > rename
> > >>>>>>>> the function name to something
> > traditional
> > >>>>>>>> like get_basename. Otherwise, it
> > sounds
> > >>>>>>>> like this function has to really
> > remove
> > >>>>>>>> folders. :) Also, all *Locker.cpp
> have
> > >>>>>>>> wrong indent in the bodies of if
> > and while
> > >>>>>>>> statements. Could this be fixed
> > with the
> > >>>>>>>> refactoring? I did not look on how
> > this
> > >>>>>>>> impacts the tests other than
> > >>>>>>>> serviceability. Thanks, Serguei
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> On 11/16/18 19:43, JC Beyler wrote:
> > >>>>>>>>> Hi all,
> > >>>>>>>>>
> > >>>>>>>>> Anybody motivated to review this?
> :)
> > >>>>>>>>> Jc
> > >>>>>>>>>
> > >>>>>>>>> On Wed, Nov 7, 2018 at 9:53 PM JC
> > Beyler
> > >>>>>>>>> <jcbeyler at google.com
> > <mailto:jcbeyler at google.com>
> > >>>>>>>>> <mailto:jcbeyler at google.com
> > <mailto:jcbeyler at google.com>>> wrote:
> > >>>>>>>>>
> > >>>>>>>>> Hi all,
> > >>>>>>>>>
> > >>>>>>>>> Could I have a review for the
> > >>>>>>>>> extension and usage of the
> > >>>>>>>>> ExceptionJniWrapper. This
> > adds lines
> > >>>>>>>>> and filenames to the end of
> the
> > >>>>>>>>> wrapper JNI methods, adds
> > tracing,
> > >>>>>>>>> and throws an error if need
> > be. I've
> > >>>>>>>>> ported the gc/lock files to
> > use the
> > >>>>>>>>> new TRACE_JNI_CALL add-on and
> > I've
> > >>>>>>>>> ported a few of the tests
> > that were
> > >>>>>>>>> already changed for the
> > assignment
> > >>>>>>>>> webrev for JDK-8212884.
> > >>>>>>>>>
> > >>>>>>>>> Webrev:
> > >>>>>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/
> > >>>>>>>>> <
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/>
> > >>>>>>>>> Bug:
> > >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> > >>>>>>>>>
> > >>>>>>>>> For illustration, if I force
> > an error
> > >>>>>>>>> to the AP04/ap04t03 test and
> > set the
> > >>>>>>>>> verbosity on, I get something
> > like:
> > >>>>>>>>>
> > >>>>>>>>> >> Calling JNI method
> > FindClass from
> > >>>>>>>>> ap04t003.cpp:343
> > >>>>>>>>> >> Calling with these
> > parameter(s):
> > >>>>>>>>> java/lang/Threadd
> > >>>>>>>>> Wait for thread to finish
> > >>>>>>>>> << Called JNI method
> > FindClass from
> > >>>>>>>>> ap04t003.cpp:343
> > >>>>>>>>> Exception in thread "Thread-0"
> > >>>>>>>>> java.lang.NoClassDefFoundError:
> > >>>>>>>>> java/lang/Threadd
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> > >>>>>>>>>
> > >>>>>>>>> Method)
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >
> > >>>>>>>>>
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >
> > >>>>>>>>>
> > >>>>>>>>> Caused by:
> > >>>>>>>>> java.lang.ClassNotFoundException:
> > >>>>>>>>> java.lang.Threadd
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
> >
> > >>>>>>>>>
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
> >
> > >>>>>>>>>
> > >>>>>>>>> at
> > >>>>>>>>>
> > java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
> > >>>>>>>>> ... 3 more
> > >>>>>>>>> FATAL ERROR in native method:
> JNI
> > >>>>>>>>> method FindClass : internal
> error
> > >>>>>>>>> from ap04t003.cpp:343
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> > >>>>>>>>>
> > >>>>>>>>> Method)
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >
> > >>>>>>>>>
> > >>>>>>>>> at
> > >>>>>>>>>
> >
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> Questions/comments I have
> about
> > >>>>>>>>> this are:
> > >>>>>>>>> - Do we want to force fatal
> > errors
> > >>>>>>>>> when a JNI call fails in
> general?
> > >>>>>>>>> Most of these tests do the
> right
> > >>>>>>>>> thing and test the return of
> > the JNI
> > >>>>>>>>> calls, for example:
> > >>>>>>>>> thrClass =
> > >>>>>>>>> jni->FindClass("java/lang/Threadd",
> > >>>>>>>>> TRACE_JNI_CALL);
> > >>>>>>>>> if (thrClass == NULL) {
> > >>>>>>>>>
> > >>>>>>>>> but now the wrapper actually
> > would do
> > >>>>>>>>> a fatal if the FindClass call
> > would
> > >>>>>>>>> return a nullptr, so we could
> > remove
> > >>>>>>>>> that test altogether. What do
> > you
> > >>>>>>>>> think?
> > >>>>>>>>> - I prefer to leave them
> > as the
> > >>>>>>>>> tests then become closer to
> > what real
> > >>>>>>>>> users would have in their
> > code and is
> > >>>>>>>>> the "recommended" way of
> doing it
> > >>>>>>>>>
> > >>>>>>>>> - The alternative is to
> > use the
> > >>>>>>>>> NonFatalError I added which
> > then just
> > >>>>>>>>> prints out that something
> > went wrong,
> > >>>>>>>>> letting the test continue.
> > Question
> > >>>>>>>>> will be what should be the
> > default?
> > >>>>>>>>> The fatal or the non-fatal
> error
> > >>>>>>>>> handling?
> > >>>>>>>>>
> > >>>>>>>>> On a different subject:
> > >>>>>>>>> - On the new tests, I've
> > removed
> > >>>>>>>>> the NSK_JNI_VERIFY since the
> JNI
> > >>>>>>>>> wrapper handles the tracing
> > and the
> > >>>>>>>>> verify in almost the same
> > way; only
> > >>>>>>>>> difference I can really tell
> > is that
> > >>>>>>>>> the complain method from NSK
> > has a
> > >>>>>>>>> max complain before stopping
> to
> > >>>>>>>>> "complain"; I have not added
> that
> > >>>>>>>>> part of the code in this
> webrev
> > >>>>>>>>>
> > >>>>>>>>> Once we decide on these, I can
> > >>>>>>>>> continue on the files from
> > >>>>>>>>> JDK-8212884 and then do both
> the
> > >>>>>>>>> assignment in an if extraction
> > >>>>>>>>> followed-by this type of
> > webrev in an
> > >>>>>>>>> easier fashion. Depending on
> > >>>>>>>>> decisions here, NSK*VERIFY
> can be
> > >>>>>>>>> deprecated as well as we go
> > forward.
> > >>>>>>>>>
> > >>>>>>>>> Thank you for the
> > reviews/comments :)
> > >>>>>>>>> Jc
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> --
> > >>>>>>>>> Thanks,
> > >>>>>>>>> Jc
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> --
> > >>>>>>>> Thanks,
> > >>>>>>>> Jc
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> --
> > >>>>>>> Thanks,
> > >>>>>>> Jc
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>>
> > >>>>>> --
> > >>>>>> Thanks,
> > >>>>>> Jc
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> --
> > >>>>> Thanks,
> > >>>>> Jc
> > >>>>
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>> Thanks,
> > >>> Jc
> > >>>
> > >>>
> > >>>
> > >>> --
> > >>>
> > >>> Thanks,
> > >>> Jc
> > >
> > >
> >
> >
> >
> > --
> >
> > Thanks,
> > Jc
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list