RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
Alex Menkov
alexey.menkov at oracle.com
Wed Dec 12 23:09:39 UTC 2018
Hm..
I considered unnamed namespaces "C++ style" (and static globals as "C
style").
Static globals were deprecated in C++ (but some time ago the deprecation
was reverted).
--alex
On 12/12/2018 13:55, serguei.spitsyn at oracle.com wrote:
> Agreed.
>
> Thanks,
> Serguei
>
>
> On 12/12/18 13:52, David Holmes wrote:
>> FWIW I think namespaces are overkill in all of this test code and just
>> obfuscates things - the declaration is easily missed. A static
>> variable in a .cpp is clearly a global variable to the file.
>>
>> Cheers,
>> David
>>
>>
>>
>> On 13/12/2018 5:37 am, serguei.spitsyn at oracle.com wrote:
>>> Hi Jc,
>>>
>>>
>>> On 12/11/18 21:16, JC Beyler wrote:
>>>> Hi all,
>>>>
>>>> Here is the new webrev with the TEST.groups change. Serguei, let me
>>>> know if I convinced you with the static vs anonymous namespaces or
>>>> if you'd still rather have a "static" for now :-)
>>>
>>>
>>> What do you think about this post? :
>>> https://stackoverflow.com/questions/11623451/static-vs-non-static-variables-in-namespace
>>>
>>>
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.05/
>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.05/>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>>>
>>> The update looks fine.
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> Thanks again for the reviews!
>>>> Jc
>>>>
>>>> On Mon, Dec 10, 2018 at 3:10 PM JC Beyler <jcbeyler at google.com
>>>> <mailto:jcbeyler at google.com>> wrote:
>>>>
>>>> Hi Serguei,
>>>>
>>>> Yes basically it is equivalent :) I can put them in but they are
>>>> not required. The norm actually wanted to deprecate it but then
>>>> remembered that C compatibility would require the static key-word
>>>> for this case [1]
>>>>
>>>> So, really, they are not required here and will amount to the same
>>>> thing: only that file can refer to them and you cannot get to them
>>>> without a globally available method to return a pointer to them
>>>> (ie same as a static variable in C).
>>>>
>>>> I can put static if it makes it easier to see but, by being in an
>>>> anonymous namespace they are only available for the file's
>>>> translation unit. For example:
>>>>
>>>> $ cat main.cpp
>>>>
>>>> int totally_global;
>>>> static int explictly_static;
>>>>
>>>> namespace {
>>>> int implicitly_static;
>>>> }
>>>>
>>>> void foo();
>>>> int main() {
>>>> foo();
>>>> }
>>>>
>>>> $ g++ -O3 main.cpp -c
>>>> $ nm main.o
>>>> U _GLOBAL_OFFSET_TABLE_
>>>> 0000000000000000 T main
>>>> 0000000000000000 B totally_global
>>>> U _Z3foov
>>>>
>>>> As you can see, the static and anonymous namespace variables are
>>>> not in the file due to not being used. If you were to use them,
>>>> you'd see them show up as something like:
>>>> 0000000000000008 b _ZL17explicitly_static
>>>> 0000000000000004 b _ZN12_GLOBAL__N_117implicitly_staticE
>>>>
>>>> Where again, it shows that it is mangling the names so that no
>>>> external usage can happen without tinkering.
>>>>
>>>> Hopefully that helps :-),
>>>> Jc
>>>>
>>>> [1]
>>>> http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012
>>>>
>>>>
>>>> On Mon, Dec 10, 2018 at 2:04 PM 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,
>>>>
>>>> I had little experience with the C++ namespaces.
>>>> My understanding is that static in this context should mean
>>>> internal linkage.
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 12/10/18 13:57, JC Beyler wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> The variables and functions are in a anonymous namespace; my
>>>>> understanding of C++ is that this is equivalent to putting it
>>>>> as static.Hence, I didn't add them there. Does that make
>>>>> sense?
>>>>>
>>>>> Thanks!
>>>>> Jc
>>>>>
>>>>> On Mon, Dec 10, 2018 at 1:33 PM 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,
>>>>>
>>>>> It looks good in general.
>>>>> One question though.
>>>>>
>>>>> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03a_04/test/hotspot/jtreg/vmTestbase/nsk/share/ExceptionCheckingJniEnv/exceptionjni001/exceptionjni001.cpp.html
>>>>>
>>>>>
>>>>> I wonder if the variables and functions have to be static.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 12/5/18 11:36, 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/
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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/
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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
>>>>>>
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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
>>>>>>
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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
>>>>>> <mailto: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/
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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>
>>>>>> > <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:
>>>>>> >
>>>>>> > 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/
>>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/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>>
>>>>>> > >>> <mailto: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>>
>>>>>> > <mailto: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>>
>>>>>> > >>> <mailto: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>>
>>>>>> <mailto: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/>
>>>>>> > >>>>>>
>>>>>> <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>
>>>>>>
>>>>>> >
>>>>>> > >>>>>>
>>>>>> > >>>>>>
>>>>>> >
>>>>>> <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>>
>>>>>> > >>>>>> <mailto: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>
>>>>>>
>>>>>> >
>>>>>> > >>>>>>
>>>>>> > >>>>>>
>>>>>> >
>>>>>> <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>
>>>>>> > >>>>>>>
>>>>>> <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>>
>>>>>> > >>>>>>> <mailto: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>>
>>>>>> > >>>>>>>> <mailto: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>>
>>>>>> > >>>>>>>> <mailto: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>>
>>>>>> > >>>>>>>>> <mailto: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/>
>>>>>> > >>>>>>>>>
>>>>>> <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
>>>>
>>>>
>>>>
>>>> --
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>
>
More information about the hotspot-dev
mailing list