RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
Jean Christophe Beyler
jcbeyler at google.com
Thu Apr 11 01:18:26 UTC 2019
Hi Paul,
Exactly :). Actually you are right, when trying to figure out build errors
in windows/solaris, I moved declarations around.
I've moved them back and the incremental is thus:
http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.06_08/
And the full new version is:
http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.08/
This has passed the submit repo (thanks serguei ;-)).
Could I get another LGTM for this and then we can move forward ? :)
Thanks for your help,
Jc
On Mon, Apr 8, 2019 at 9:11 AM Hohensee, Paul <hohensee at amazon.com> wrote:
> Almost all copyright changes, plus a bit of code reorg, but no real
> implementation changes except in ExceptionCheckingJniEnv.cpp, which are
> fine. A quibble: I’d have left declarations located at first use instead of
> moving them up to the beginning of their methods, but I’m not doctrinaire,
> so they’re fine too.
>
>
>
> Paul
>
>
>
> *From: *Jean Christophe Beyler <jcbeyler at google.com>
> *Date: *Tuesday, April 2, 2019 at 10:00 AM
> *To: *Alex Menkov <alexey.menkov at oracle.com>
> *Cc: *"Hohensee, Paul" <hohensee at amazon.com>, hotspot-dev Source
> Developers <hotspot-dev at openjdk.java.net>
> *Subject: *Re: RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few
> tests
>
>
>
> 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
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list