RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Mon Dec 10 20:54:27 UTC 2018
Hi Chris,
Thanks for looking again. That's a great point. I looked at the TEST.groups
file and it seems to me that maybe we could put it in vmTestbase_nsk_jvmti
or vmTestbase_nsk_monitoring? The issue I have in finding the right spot is
that there is no "general vmTestbase" spot. Does anyone have a better test
group where it could fit?
Thanks!
Jc
On Mon, Dec 10, 2018 at 11:34 AM Chris Plummer <chris.plummer at oracle.com>
wrote:
> Hi JC,
>
> The changes look fine. With regard to your new test, does it get run with
> any test group? It seems no. If we are going to keep it, it should be run
> automatically with one of the test groups.
>
> thanks,
>
> Chris
>
> On 12/5/18 11:36 AM, JC Beyler wrote:
>
> 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
>
>
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list