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

Jean Christophe Beyler jcbeyler at google.com
Tue Apr 2 16:59:11 UTC 2019


Hi all,

Friendly ping on this one, I know I've used up quite a bit of time on it;
my apologies again :)
Jc

On Tue, Mar 26, 2019 at 12:37 PM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

> Hi all,
>
> My apologies again for this one. This has been a bit tricky to get in and
> has fell off my priority list due to other issues/commitments but I'm back
> to finishing this up and the next webrevs regarding this work :)
>
> Problem is that we change years so the copyrights changed on some of these
> and there were a few problems with various architectures/build systems that
> made the testing fail on the submit repo.
>
> So I offer two webrevs:
>
> - The incremental from the last LGTM stamped one:
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06_07/
>
> - The full webrev that cleaned up a few problems for windows and solaris
> and now passes the submit repo:
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.07/
>
> And again, my sincere apologies that it took me SO long to get back to
> this but I had to work through the random submit repo failures and some of
> them took time for me to debug (thanks Serguei for your help :-)),
> Jc
>
> On Tue, Jan 22, 2019 at 6:03 PM Alex Menkov <alexey.menkov at oracle.com>
> wrote:
>
>> +1
>>
>> --alex
>>
>> On 01/22/2019 10:29, JC Beyler wrote:
>> > Thanks Paul!
>> >
>> > Anybody else for the review for version 6?
>> >
>> > Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/
>> > <http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/>
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>> > <https://bugs.openjdk.java.net/browse/JDK-8213501>
>> >
>> > Thanks,
>> > Jc
>> >
>> > On Tue, Jan 22, 2019 at 6:10 AM Hohensee, Paul <hohensee at amazon.com
>> > <mailto:hohensee at amazon.com>> wrote:
>> >
>> >     Lgtm :)
>> >
>> >     Paul
>> >
>> >     On 1/14/19, 7:46 AM, "hotspot-dev on behalf of JC Beyler"
>> >     <hotspot-dev-bounces at openjdk.java.net
>> >     <mailto:hotspot-dev-bounces at openjdk.java.net> on behalf of
>> >     jcbeyler at google.com <mailto:jcbeyler at google.com>> wrote:
>> >
>> >          Hi all,
>> >
>> >          Friendly ping on this one, I know that it has been a long
>> >     process with back
>> >          and forths, to which I apologize...
>> >
>> >          But is there any way I could get a final LGTM for version 6?
>> >
>> >          Webrev:
>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/
>> >          Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>> >
>> >          Thanks!
>> >          Jc
>> >
>> >          On Tue, Jan 8, 2019 at 10:05 AM JC Beyler <jcbeyler at google.com
>> >     <mailto:jcbeyler at google.com>> wrote:
>> >
>> >          > Happy new year all!
>> >          >
>> >          > Could I get a final LGTM for version 6?
>> >          >
>> >          > Webrev:
>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/
>> >          > Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>> >          >
>> >          > Thanks!
>> >          > Jc
>> >          >
>> >          > On Mon, Dec 17, 2018 at 8:43 AM JC Beyler
>> >     <jcbeyler at google.com <mailto:jcbeyler at google.com>> wrote:
>> >          >
>> >          >> Hi all,
>> >          >>
>> >          >> I don't believe I got actual LGTM for this version:
>> >          >>
>> >          >>
>> >          >> Webrev:
>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/
>> >          >> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>> >          >>
>> >          >>
>> >          >> It removed the namespaces and uses explicit static instead
>> :)
>> >          >>
>> >          >> Thanks!
>> >          >> Jc
>> >          >>
>> >          >> On Wed, Dec 12, 2018 at 8:06 PM JC Beyler
>> >     <jcbeyler at google.com <mailto:jcbeyler at google.com>> wrote:
>> >          >>
>> >          >>> So did I Alexey but with David & Serguei preferring static,
>> >     it seems
>> >          >>> more reasonable to go down their route :-)
>> >          >>>
>> >          >>> So here is the latest webrev with static instead of an
>> >     anonymous
>> >          >>> namespace:
>> >          >>>
>> >          >>> Webrev:
>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06/
>> >          >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
>> >          >>>
>> >          >>> Let me know what you think, can I get a webrev 06 review?
>> >          >>>
>> >          >>> Thanks!
>> >          >>> Jc
>> >          >>>
>> >          >>> On Wed, Dec 12, 2018 at 3:10 PM Alex Menkov
>> >     <alexey.menkov at oracle.com <mailto:alexey.menkov at oracle.com>>
>> >          >>> wrote:
>> >          >>>
>> >          >>>> 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
>> >     <mailto: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
>> >     <mailto: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>
>> >          >>>> >>>> <mailto: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>
>> >          >>>> >>>>     <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,
>> >          >>>> >>>>
>> >          >>>> >>>>         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>
>> >          >>>> >>>>>         <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,
>> >          >>>> >>>>>
>> >          >>>> >>>>>             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>
>> >          >>>> >>>>>>             <mailto: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>>
>> >          >>>> >>>>>>                 >
>> >     <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:
>> >          >>>> >>>>>>                 >
>> >          >>>> >>>>>>                 >     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>>>
>> >          >>>> >>>>>>                 >      >>>
>> >     <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
>> >     <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>>>
>> >          >>>> >>>>>>                 >  <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
>> >     <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>>>
>> >          >>>> >>>>>>                 >      >>>
>> >     <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
>> >     <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>>>
>> >          >>>> >>>>>>                 <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
>> >     <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>>>
>> >          >>>> >>>>>>                 > >>>>>>
>> >     <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
>> >     <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>>>
>> >          >>>> >>>>>>                 >      >>>>>>>
>> >     <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
>> >     <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>>>
>> >          >>>> >>>>>>                 >      >>>>>>>> <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
>> >     <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>>>
>> >          >>>> >>>>>>                 >      >>>>>>>> <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
>> >     <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>>>
>> >          >>>> >>>>>>                 > >>>>>>>>>
>> >     <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
>> >     <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
>> >          >>>> >>>
>> >          >>>> >
>> >          >>>>
>> >          >>>
>> >          >>>
>> >          >>> --
>> >          >>>
>> >          >>> Thanks,
>> >          >>> Jc
>> >          >>>
>> >          >>
>> >          >>
>> >          >> --
>> >          >>
>> >          >> Thanks,
>> >          >> Jc
>> >          >>
>> >          >
>> >          >
>> >          > --
>> >          >
>> >          > Thanks,
>> >          > Jc
>> >          >
>> >
>> >
>> >          --
>> >
>> >          Thanks,
>> >          Jc
>> >
>> >
>> >
>> >
>> > --
>> >
>> > Thanks,
>> > Jc
>>
>
>
> --
>
> Thanks,
> Jc
>


-- 

Thanks,
Jc


More information about the hotspot-dev mailing list