RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Dec 12 21:55:25 UTC 2018
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