RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
David Holmes
david.holmes at oracle.com
Tue Dec 4 07:29:03 UTC 2018
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
More information about the hotspot-dev
mailing list