RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
Hohensee, Paul
hohensee at amazon.com
Mon Apr 8 16:08:29 UTC 2019
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<mailto: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<mailto: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>
> <mailto: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>
> <mailto: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> <mailto: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>
> <mailto: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> <mailto: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> <mailto: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> <mailto: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>
> <mailto: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>
> <mailto: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>>
> >>>> >>>> <mailto: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>>
> >>>> >>>> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>> <
> >>>> serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>>> wrote:
> >>>> >>>>
> >>>> >>>> Hi Jc,
> >>>> >>>>
> >>>> >>>> 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>>
> >>>> >>>>> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>>
> >>>> >>>>> <serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>>> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>
> <mailto:serguei.spitsyn at oracle.com<mailto:serguei.spitsyn at oracle.com>>>> wrote:
> >>>> >>>>>
> >>>> >>>>> Hi Jc,
> >>>> >>>>>
> >>>> >>>>> 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>>
> >>>> >>>>>> <mailto: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>>>
> >>>> >>>>>> >
> <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:
> >>>> >>>>>> >
> >>>> >>>>>> > 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>>>>
> >>>> >>>>>> > >>>
> <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<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>>>>
> >>>> >>>>>> > <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<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>>>>
> >>>> >>>>>> > >>>
> <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<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>>>>
> >>>> >>>>>> <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<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>>>>
> >>>> >>>>>> > >>>>>>
> <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<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>>>>
> >>>> >>>>>> > >>>>>>>
> <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<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>>>>
> >>>> >>>>>> > >>>>>>>> <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<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>>>>
> >>>> >>>>>> > >>>>>>>> <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<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>>>>
> >>>> >>>>>> > >>>>>>>>>
> <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<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