RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests

JC Beyler jcbeyler at google.com
Tue Dec 4 06:04:01 UTC 2018


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 <
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>> 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>>
> 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> 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>>
> >>>>>         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>> 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>> 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>
> >>>>>>>>                     <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>> 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