RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Tue Jan 22 18:29:21 UTC 2019
Thanks Paul!
Anybody else for the review 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 22, 2019 at 6:10 AM Hohensee, Paul <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 on behalf of 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> 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>
> 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>
> 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>
> >>> 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 wrote:
> >>>> > Agreed.
> >>>> >
> >>>> > Thanks,
> >>>> > Serguei
> >>>> >
> >>>> >
> >>>> > On 12/12/18 13:52, David Holmes wrote:
> >>>> >> FWIW I think namespaces are overkill in all of this test code
> and
> >>>> just
> >>>> >> obfuscates things - the declaration is easily missed. A static
> >>>> >> variable in a .cpp is clearly a global variable to the file.
> >>>> >>
> >>>> >> Cheers,
> >>>> >> David
> >>>> >>
> >>>> >>
> >>>> >>
> >>>> >> On 13/12/2018 5:37 am, serguei.spitsyn at oracle.com wrote:
> >>>> >>> Hi Jc,
> >>>> >>>
> >>>> >>>
> >>>> >>> On 12/11/18 21:16, JC Beyler wrote:
> >>>> >>>> Hi all,
> >>>> >>>>
> >>>> >>>> Here is the new webrev with the TEST.groups change. Serguei,
> let
> >>>> me
> >>>> >>>> know if I convinced you with the static vs anonymous
> namespaces or
> >>>> >>>> if you'd still rather have a "static" for now :-)
> >>>> >>>
> >>>> >>>
> >>>> >>> What do you think about this post? :
> >>>> >>>
> >>>>
> https://stackoverflow.com/questions/11623451/static-vs-non-static-variables-in-namespace
> >>>> >>>
> >>>> >>>
> >>>> >>>>
> >>>> >>>> Webrev:
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.05/
> >>>> >>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.05/>
> >>>> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>
> >>>> >>> The update looks fine.
> >>>> >>>
> >>>> >>> Thanks,
> >>>> >>> Serguei
> >>>> >>>
> >>>> >>>
> >>>> >>> Thanks,
> >>>> >>> Serguei
> >>>> >>>
> >>>> >>>>
> >>>> >>>> Thanks again for the reviews!
> >>>> >>>> Jc
> >>>> >>>>
> >>>> >>>> On Mon, Dec 10, 2018 at 3:10 PM JC Beyler <
> jcbeyler at google.com
> >>>> >>>> <mailto:jcbeyler at google.com>> wrote:
> >>>> >>>>
> >>>> >>>> Hi Serguei,
> >>>> >>>>
> >>>> >>>> Yes basically it is equivalent :) I can put them in but
> they
> >>>> are
> >>>> >>>> not required. The norm actually wanted to deprecate it
> but then
> >>>> >>>> remembered that C compatibility would require the static
> >>>> key-word
> >>>> >>>> for this case [1]
> >>>> >>>>
> >>>> >>>> So, really, they are not required here and will amount
> to the
> >>>> same
> >>>> >>>> thing: only that file can refer to them and you cannot
> get to
> >>>> them
> >>>> >>>> without a globally available method to return a pointer
> to them
> >>>> >>>> (ie same as a static variable in C).
> >>>> >>>>
> >>>> >>>> I can put static if it makes it easier to see but, by
> being in
> >>>> an
> >>>> >>>> anonymous namespace they are only available for the
> file's
> >>>> >>>> translation unit. For example:
> >>>> >>>>
> >>>> >>>> $ cat main.cpp
> >>>> >>>>
> >>>> >>>> int totally_global;
> >>>> >>>> static int explictly_static;
> >>>> >>>>
> >>>> >>>> namespace {
> >>>> >>>> int implicitly_static;
> >>>> >>>> }
> >>>> >>>>
> >>>> >>>> void foo();
> >>>> >>>> int main() {
> >>>> >>>> foo();
> >>>> >>>> }
> >>>> >>>>
> >>>> >>>> $ g++ -O3 main.cpp -c
> >>>> >>>> $ nm main.o
> >>>> >>>> U _GLOBAL_OFFSET_TABLE_
> >>>> >>>> 0000000000000000 T main
> >>>> >>>> 0000000000000000 B totally_global
> >>>> >>>> U _Z3foov
> >>>> >>>>
> >>>> >>>> As you can see, the static and anonymous namespace
> variables
> >>>> are
> >>>> >>>> not in the file due to not being used. If you were to
> use them,
> >>>> >>>> you'd see them show up as something like:
> >>>> >>>> 0000000000000008 b _ZL17explicitly_static
> >>>> >>>> 0000000000000004 b _ZN12_GLOBAL__N_117implicitly_staticE
> >>>> >>>>
> >>>> >>>> Where again, it shows that it is mangling the names so
> that no
> >>>> >>>> external usage can happen without tinkering.
> >>>> >>>>
> >>>> >>>> Hopefully that helps :-),
> >>>> >>>> Jc
> >>>> >>>>
> >>>> >>>> [1]
> >>>> >>>>
> http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012
> >>>> >>>>
> >>>> >>>>
> >>>> >>>> On Mon, Dec 10, 2018 at 2:04 PM
> serguei.spitsyn at oracle.com
> >>>> >>>> <mailto:serguei.spitsyn at oracle.com> <
> >>>> serguei.spitsyn at oracle.com
> >>>> >>>> <mailto:serguei.spitsyn at oracle.com>> wrote:
> >>>> >>>>
> >>>> >>>> Hi Jc,
> >>>> >>>>
> >>>> >>>> I had little experience with the C++ namespaces.
> >>>> >>>> My understanding is that static in this context
> should mean
> >>>> >>>> internal linkage.
> >>>> >>>>
> >>>> >>>> Thanks,
> >>>> >>>> Serguei
> >>>> >>>>
> >>>> >>>>
> >>>> >>>> On 12/10/18 13:57, JC Beyler wrote:
> >>>> >>>>> Hi Serguei,
> >>>> >>>>>
> >>>> >>>>> The variables and functions are in a anonymous
> namespace;
> >>>> my
> >>>> >>>>> understanding of C++ is that this is equivalent to
> >>>> putting it
> >>>> >>>>> as static.Hence, I didn't add them there. Does that
> make
> >>>> >>>>> sense?
> >>>> >>>>>
> >>>> >>>>> Thanks!
> >>>> >>>>> Jc
> >>>> >>>>>
> >>>> >>>>> On Mon, Dec 10, 2018 at 1:33 PM
> >>>> serguei.spitsyn at oracle.com
> >>>> >>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>> <serguei.spitsyn at oracle.com
> >>>> >>>>> <mailto:serguei.spitsyn at oracle.com>> wrote:
> >>>> >>>>>
> >>>> >>>>> Hi Jc,
> >>>> >>>>>
> >>>> >>>>> It looks good in general.
> >>>> >>>>> One question though.
> >>>> >>>>>
> >>>> >>>>>
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03a_04/test/hotspot/jtreg/vmTestbase/nsk/share/ExceptionCheckingJniEnv/exceptionjni001/exceptionjni001.cpp.html
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> I wonder if the variables and functions have to
> be
> >>>> static.
> >>>> >>>>>
> >>>> >>>>> Thanks,
> >>>> >>>>> Serguei
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> On 12/5/18 11:36, JC Beyler wrote:
> >>>> >>>>>> Hi all,
> >>>> >>>>>>
> >>>> >>>>>> My apologies to having to come back for another
> >>>> review
> >>>> >>>>>> for this change: I ran into a snag when trying
> to
> >>>> pull
> >>>> >>>>>> the latest changes compared to the base I was
> working
> >>>> >>>>>> on. I basically forgot that there was an issue
> with
> >>>> >>>>>> snprintf and that I had solved it via
> JDK-8213622.
> >>>> >>>>>>
> >>>> >>>>>> Could I have a new review of this webrev:
> >>>> >>>>>> Webrev:
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.04/
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.04/
> >
> >>>> >>>>>> Bug:
> >>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> Incremental from the port of webrev.03 that got
> >>>> LGTMs:
> >>>> >>>>>>
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a_04/
> >>>> >>>>>> <
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03a_04/>
> >>>> >>>>>>
> >>>> >>>>>> A few comments on this because it took me a
> while to
> >>>> get
> >>>> >>>>>> things in a state I thought was good:
> >>>> >>>>>> - I had to implement an itoa method, do we
> have
> >>>> >>>>>> something like that in the test base (remember
> that
> >>>> >>>>>> JDK-8213622 could not use sprintf due to being
> in the
> >>>> >>>>>> test code)?
> >>>> >>>>>>
> >>>> >>>>>> - The differences here compared to the one
> you all
> >>>> >>>>>> reviewed are:
> >>>> >>>>>> - I found that adding to the
> strlen/memcpy
> >>>> error
> >>>> >>>>>> prone and thought that I would try to make it
> less
> >>>> so.
> >>>> >>>>>> If you want to compare, I extended the
> strlen/memcpy
> >>>> >>>>>> with the new format to show you if you prefer
> [1]
> >>>> >>>>>> - Note that the diff between the
> "old
> >>>> >>>>>> extended way from [1]" to the webrev.04 can be
> found
> >>>> >>>>>> in [2]
> >>>> >>>>>>
> >>>> >>>>>> - I added a test to test the exception
> wrapper
> >>>> in
> >>>> >>>>>> tests :); I'm not sure it is deemed useful or
> not but
> >>>> >>>>>> helped me assure myself that I was not doing
> things
> >>>> >>>>>> wrong; you can find the base test file here
> [3];
> >>>> should
> >>>> >>>>>> we have this or not? (I know that normally we
> don't
> >>>> add
> >>>> >>>>>> tests to vmTestbase but thought this might be
> an
> >>>> >>>>>> exception)
> >>>> >>>>>>
> >>>> >>>>>> Thanks for your help and my apologies for the
> snag,
> >>>> >>>>>> Jc
> >>>> >>>>>>
> >>>> >>>>>> [1]:
> >>>> >>>>>>
> >>>>
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
> >>>> >>>>>>
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03a/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> [2]:
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03a_04
> >>>> >>>>>> <
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03a_04>
> >>>> >>>>>> [3]
> >>>> >>>>>>
> >>>>
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/ExceptionCheckingJniEnv/exceptionjni001/exceptionjni001.cpp.html
> >>>> >>>>>>
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.04/test/hotspot/jtreg/vmTestbase/nsk/share/ExceptionCheckingJniEnv/exceptionjni001/exceptionjni001.cpp.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>> On Mon, Dec 3, 2018 at 11:29 PM David Holmes
> >>>> >>>>>> <david.holmes at oracle.com
> >>>> >>>>>> <mailto:david.holmes at oracle.com>> wrote:
> >>>> >>>>>>
> >>>> >>>>>> Looks fine to me.
> >>>> >>>>>>
> >>>> >>>>>> Thanks,
> >>>> >>>>>> David
> >>>> >>>>>>
> >>>> >>>>>> On 4/12/2018 4:04 pm, JC Beyler wrote:
> >>>> >>>>>> > Hi both,
> >>>> >>>>>> >
> >>>> >>>>>> > Thanks for the reviews! Since Serguei
> did not
> >>>> >>>>>> insist on get_basename, I
> >>>> >>>>>> > went for get_dirname since the method is
> a
> >>>> local
> >>>> >>>>>> static method and won't
> >>>> >>>>>> > have its name start spreading, I think
> it's ok
> >>>> too.
> >>>> >>>>>> >
> >>>> >>>>>> > For the naming of the local variable,
> the idea
> >>>> >>>>>> initially was to use the
> >>>> >>>>>> > same name as the local variable for
> JNIEnv
> >>>> already
> >>>> >>>>>> used to reduce the
> >>>> >>>>>> > code change. Since I'm now adding the
> line
> >>>> macro
> >>>> >>>>>> at the end anyway, this
> >>>> >>>>>> > does not matter anymore so I converged
> all
> >>>> local
> >>>> >>>>>> variables to "jni".
> >>>> >>>>>> >
> >>>> >>>>>> > So, without further ado, here is the new
> >>>> version:
> >>>> >>>>>> > Webrev:
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.03/
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.03/
> >
> >>>> >>>>>> > Bug:
> >>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> >
> >>>> >>>>>> > This passes the various tests changed by
> the
> >>>> >>>>>> webrev on my dev machine.
> >>>> >>>>>> >
> >>>> >>>>>> > Let me know what you think,
> >>>> >>>>>> > Jc
> >>>> >>>>>> >
> >>>> >>>>>> > On Mon, Dec 3, 2018 at 8:40 PM
> >>>> >>>>>> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>>>> <serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>> wrote:
> >>>> >>>>>> >
> >>>> >>>>>> > On 12/3/18 20:15, Chris Plummer
> wrote:
> >>>> >>>>>> > > Hi JC,
> >>>> >>>>>> > >
> >>>> >>>>>> > > Overall it looks good. A few
> naming nits
> >>>> >>>>>> thought:
> >>>> >>>>>> > >
> >>>> >>>>>> > > In bi01t001.cpp, why have you
> declared
> >>>> the
> >>>> >>>>>> > ExceptionCheckingJniEnvPtr
> >>>> >>>>>> > > using jni_env(jni). Elsewhere you
> use
> >>>> >>>>>> jni(jni_env) and rename the
> >>>> >>>>>> > > method argument passed in from
> jni to
> >>>> >>>>>> jni_env.
> >>>> >>>>>> > >
> >>>> >>>>>> > > Related to this, I also noticed
> in some
> >>>> >>>>>> files that already are using
> >>>> >>>>>> > > ExceptionCheckingJniEnvPtr, such
> as
> >>>> >>>>>> CharArrayCriticalLocker.cpp, you
> >>>> >>>>>> > > delcared it as env(jni_env). So
> that
> >>>> means
> >>>> >>>>>> there are 3 different
> >>>> >>>>>> > names
> >>>> >>>>>> > > you have used for the
> >>>> >>>>>> ExceptionCheckingJniEnvPtr local variable.
> >>>> >>>>>> > They
> >>>> >>>>>> > > should be consistent.
> >>>> >>>>>> > >
> >>>> >>>>>> > > Also, can you rename
> get_basename() to
> >>>> >>>>>> get_dirname()? I know Serguei
> >>>> >>>>>> > > suggested get_basename() a while
> back,
> >>>> but
> >>>> >>>>>> unless "basename" is
> >>>> >>>>>> > > commonly used for this purpose, I
> think
> >>>> >>>>>> "dirname" is more self
> >>>> >>>>>> > > explanatory.
> >>>> >>>>>> >
> >>>> >>>>>> > In general, I'm Okay with
> get_dirname().
> >>>> >>>>>> > Just to mention dirname can be both
> short
> >>>> or
> >>>> >>>>>> full, so it is a little
> >>>> >>>>>> > confusing as well.
> >>>> >>>>>> > It is the reason why the
> get_basename() was
> >>>> >>>>>> suggested.
> >>>> >>>>>> > However, I do not insist on
> get_basename()
> >>>> nor
> >>>> >>>>>> get_full_dirname(). :)
> >>>> >>>>>> >
> >>>> >>>>>> > Thanks,
> >>>> >>>>>> > Serguei
> >>>> >>>>>> >
> >>>> >>>>>> >
> >>>> >>>>>> > > thanks,
> >>>> >>>>>> > >
> >>>> >>>>>> > > Chris
> >>>> >>>>>> > >
> >>>> >>>>>> > > On 12/2/18 10:29 PM, David Holmes
> wrote:
> >>>> >>>>>> > >> Hi Jc,
> >>>> >>>>>> > >>
> >>>> >>>>>> > >> I've been lurking on this one
> and have
> >>>> had
> >>>> >>>>>> a look through. I'm okay
> >>>> >>>>>> > >> with the FatalError approach for
> the
> >>>> tests
> >>>> >>>>>> - we don't expect
> >>>> >>>>>> > anything
> >>>> >>>>>> > >> to go wrong in a well written
> test in a
> >>>> >>>>>> correctly functioning VM.
> >>>> >>>>>> > >>
> >>>> >>>>>> > >> Thanks,
> >>>> >>>>>> > >> David
> >>>> >>>>>> > >>
> >>>> >>>>>> > >>
> >>>> >>>>>> > >>
> >>>> >>>>>> > >> On 3/12/2018 3:24 pm, JC Beyler
> wrote:
> >>>> >>>>>> > >>> Hi all,
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Would someone on the GC or
> runtime
> >>>> team
> >>>> >>>>>> be motivated to give
> >>>> >>>>>> > this a
> >>>> >>>>>> > >>> review? :)
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> It would be much appreciated!
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Webrev:
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/
> >
> >>>> >>>>>> > >>> Bug:
> >>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Thanks for your help,
> >>>> >>>>>> > >>> Jc
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> On Tue, Nov 27, 2018 at 4:36 PM
> JC
> >>>> Beyler
> >>>> >>>>>> <jcbeyler at google.com <mailto:
> jcbeyler at google.com
> >>>> >
> >>>> >>>>>> > <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>>
> >>>> >>>>>> > >>> <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>
> >>>> >>>>>> <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>>>> wrote:
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Hi Chris,
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Yes I was waiting for
> another
> >>>> review
> >>>> >>>>>> since you had explicitly
> >>>> >>>>>> > >>> asked :)
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> And sounds good that when
> someone
> >>>> >>>>>> from GC or runtime gives a
> >>>> >>>>>> > >>> review,
> >>>> >>>>>> > >>> I'll wait for your full
> review on
> >>>> the
> >>>> >>>>>> webrev.02!
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Thanks again for your help,
> >>>> >>>>>> > >>> Jc
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> On Tue, Nov 27, 2018 at
> 12:48 PM
> >>>> >>>>>> Chris Plummer
> >>>> >>>>>> > >>> <chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>>>
> >>>> >>>>>> > wrote:
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Hi JC,
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> I think it would be
> good to
> >>>> get a
> >>>> >>>>>> review from the gc or
> >>>> >>>>>> > runtime
> >>>> >>>>>> > >>> teams, since this also
> affects
> >>>> >>>>>> their tests.
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Also, once we are
> settled on
> >>>> this
> >>>> >>>>>> FatalError approach,
> >>>> >>>>>> > I still
> >>>> >>>>>> > >>> need to give your
> webrev-02 a
> >>>> >>>>>> full review. I only
> >>>> >>>>>> > skimmed over
> >>>> >>>>>> > >>> parts of it (I did look
> at all
> >>>> >>>>>> the changes in webrevo-01).
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> thanks,
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Chris
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> On 11/27/18 8:58 AM,
> >>>> >>>>>> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>>>> > >>> <mailto:
> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>> wrote:
> >>>> >>>>>> > >>>> Hi Jc,
> >>>> >>>>>> > >>>>
> >>>> >>>>>> > >>>> I've already reviewed
> this
> >>>> too.
> >>>> >>>>>> > >>>>
> >>>> >>>>>> > >>>> Thanks,
> >>>> >>>>>> > >>>> Serguei
> >>>> >>>>>> > >>>>
> >>>> >>>>>> > >>>>
> >>>> >>>>>> > >>>> On 11/27/18 06:56, JC
> Beyler
> >>>> >>>>>> wrote:
> >>>> >>>>>> > >>>>> Thanks Chris,
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> Anybody else motivated to
> look at
> >>>> this
> >>>> >>>>>> and review it? :)
> >>>> >>>>>> > >>>>> Jc
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> On Mon, Nov 26, 2018
> at
> >>>> 1:26 PM
> >>>> >>>>>> Chris Plummer
> >>>> >>>>>> > >>>>> <chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>
> >>>> >>>>>> <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>>>
> >>>> >>>>>> > >>>>> wrote:
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> Hi JC,
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> I'm ok with the FatalError
> approach,
> >>>> >>>>>> but would
> >>>> >>>>>> > like to
> >>>> >>>>>> > >>>>> hear opinions from others
> also.
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> thanks,
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> Chris
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> On 11/21/18 8:19 AM, JC Beyler
> >>>> wrote:
> >>>> >>>>>> > >>>>>> Hi Chris,
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Thanks for
> taking the
> >>>> time
> >>>> >>>>>> to look at it and yes you
> >>>> >>>>>> > >>>>>> have raised
> exactly why
> >>>> >>>>>> the webrev is between two
> >>>> >>>>>> > >>>>>> worlds: in cases
> where
> >>>> a
> >>>> >>>>>> fatal error on failure is
> >>>> >>>>>> > >>>>>> wanted, should we
> >>>> simplify
> >>>> >>>>>> the code to remove
> >>>> >>>>>> > the return
> >>>> >>>>>> > >>>>>> tests since we
> do them
> >>>> >>>>>> internally? Now that I've
> >>>> >>>>>> > looked
> >>>> >>>>>> > >>>>>> around for
> non-fatal
> >>>> >>>>>> cases, I think the answer
> >>>> >>>>>> > is yes,
> >>>> >>>>>> > >>>>>> it simplifies
> the code
> >>>> >>>>>> while maintaining the checks.
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> I looked a bit
> and it
> >>>> >>>>>> seems that I can't find
> >>>> >>>>>> > easily a
> >>>> >>>>>> > >>>>>> case where the
> test
> >>>> >>>>>> accepts a JNI failure to
> >>>> >>>>>> > then move
> >>>> >>>>>> > >>>>>> on. Therefore,
> perhaps,
> >>>> >>>>>> for now, the fail with a
> >>>> >>>>>> > Fatal
> >>>> >>>>>> > >>>>>> is enough and we
> can
> >>>> work
> >>>> >>>>>> on the tests to clean
> >>>> >>>>>> > them up?
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> That means that
> this is
> >>>> >>>>>> the new webrev with only
> >>>> >>>>>> > Fatal
> >>>> >>>>>> > >>>>>> and cleans up the
> >>>> tests so
> >>>> >>>>>> that it is no longer in
> >>>> >>>>>> > >>>>>> between two
> worlds:
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Webrev:
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/
> >
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/
> >
> >>>> >>>>>> > >>>>>> Bug:
> >>>> >>>>>> >
> >>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> (This passes
> testing
> >>>> on my
> >>>> >>>>>> dev machine for all the
> >>>> >>>>>> > >>>>>> modified tests)
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> with the example
> you
> >>>> >>>>>> provided, it now looks like:
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >>>> >>>>>>
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.02/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Where it does,
> to me at
> >>>> >>>>>> least, seem cleaner and less
> >>>> >>>>>> > >>>>>> "noisy".
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Let me know what
> you
> >>>> think,
> >>>> >>>>>> > >>>>>> Jc
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> On Tue, Nov 20,
> 2018 at
> >>>> >>>>>> 9:33 PM Chris Plummer
> >>>> >>>>>> > >>>>>> <
> >>>> chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>
> >>>> >>>>>> > >>>>>> <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>>> wrote:
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Hi JC,
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Sorry about
> the
> >>>> delay.
> >>>> >>>>>> I had to go back an
> >>>> >>>>>> > look at
> >>>> >>>>>> > >>>>>> the initial
> 8210842
> >>>> >>>>>> webrev and RFR thread to see
> >>>> >>>>>> > >>>>>> what this was
> >>>> >>>>>> initially all about.
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> In general
> the
> >>>> changes
> >>>> >>>>>> look good.
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> I don't have
> a good
> >>>> >>>>>> answer to your
> >>>> >>>>>> > >>>>>> FatalError/NonFatalError
> question. It
> >>>> makes
> >>>> >>>>>> > the code
> >>>> >>>>>> > >>>>>> a lot
> cleaner to
> >>>> use
> >>>> >>>>>> FatalError, but then it
> >>>> >>>>>> > is a
> >>>> >>>>>> > >>>>>> behavior
> change,
> >>>> and
> >>>> >>>>>> you also need to deal with
> >>>> >>>>>> > >>>>>> tests that
> >>>> >>>>>> intentionally induce errors (do
> >>>> >>>>>> > you have
> >>>> >>>>>> > >>>>>> an example of
> >>>> that).
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> In any case,
> right
> >>>> now
> >>>> >>>>>> your webrev seems to be
> >>>> >>>>>> > >>>>>> between two
> worlds.
> >>>> >>>>>> You are producing
> >>>> >>>>>> > FatalError,
> >>>> >>>>>> > >>>>>> but still
> checking
> >>>> >>>>>> results. Here's a good
> >>>> >>>>>> > example:
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >>>> >>>>>>
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> <
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/ap04t003.cpp.frames.html
> >
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> I'm not sure
> if
> >>>> this
> >>>> >>>>>> is just a temporary
> >>>> >>>>>> > state until
> >>>> >>>>>> > >>>>>> it was
> decided
> >>>> which
> >>>> >>>>>> approach to take.
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> thanks,
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> Chris
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> On 11/20/18
> 2:14
> >>>> PM,
> >>>> >>>>>> JC Beyler wrote:
> >>>> >>>>>> > >>>>>>> Hi all,
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Chris
> thought it
> >>>> made
> >>>> >>>>>> sense to have more
> >>>> >>>>>> > eyes on
> >>>> >>>>>> > >>>>>>> this change
> than
> >>>> just
> >>>> >>>>>> serviceability as it will
> >>>> >>>>>> > >>>>>>> modify to
> tests
> >>>> that
> >>>> >>>>>> are not only
> >>>> >>>>>> > serviceability
> >>>> >>>>>> > >>>>>>> tests so
> I've
> >>>> moved
> >>>> >>>>>> this to conversation
> >>>> >>>>>> > here :)
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> For
> convenience,
> >>>> I've
> >>>> >>>>>> copy-pasted the
> >>>> >>>>>> > initial RFR:
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Could I
> have a
> >>>> review
> >>>> >>>>>> for the extension and
> >>>> >>>>>> > usage
> >>>> >>>>>> > >>>>>>> of the
> >>>> >>>>>> ExceptionJniWrapper. This adds lines and
> >>>> >>>>>> > >>>>>>> filenames
> to the
> >>>> end
> >>>> >>>>>> of the wrapper JNI
> >>>> >>>>>> > methods,
> >>>> >>>>>> > >>>>>>> adds
> tracing, and
> >>>> >>>>>> throws an error if need
> >>>> >>>>>> > be. I've
> >>>> >>>>>> > >>>>>>> ported the
> gc/lock
> >>>> >>>>>> files to use the new
> >>>> >>>>>> > >>>>>>>
> TRACE_JNI_CALL
> >>>> add-on
> >>>> >>>>>> and I've ported a few
> >>>> >>>>>> > of the
> >>>> >>>>>> > >>>>>>> tests that
> were
> >>>> >>>>>> already changed for the
> >>>> >>>>>> > assignment
> >>>> >>>>>> > >>>>>>> webrev for
> >>>> >>>>>> JDK-8212884.
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Webrev:
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.01
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.01>
> >>>> >>>>>> > >>>>>>> Bug:
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> For
> illustration,
> >>>> if
> >>>> >>>>>> I force an error to the
> >>>> >>>>>> > >>>>>>>
> AP04/ap04t03 test
> >>>> and
> >>>> >>>>>> set the verbosity on,
> >>>> >>>>>> > I get
> >>>> >>>>>> > >>>>>>> something
> like:
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> >> Calling
> JNI
> >>>> method
> >>>> >>>>>> FindClass from
> >>>> >>>>>> > >>>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>> >> Calling
> with
> >>>> these
> >>>> >>>>>> parameter(s):
> >>>> >>>>>> > >>>>>>> java/lang/Threadd
> >>>> >>>>>> > >>>>>>> Wait for
> thread
> >>>> to
> >>>> >>>>>> finish
> >>>> >>>>>> > >>>>>>> << Called
> JNI
> >>>> method
> >>>> >>>>>> FindClass from
> >>>> >>>>>> > >>>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>> Exception in
> >>>> thread
> >>>> >>>>>> "Thread-0"
> >>>> >>>>>> > >>>>>>>
> java.lang.NoClassDefFoundError:
> >>>> >>>>>> > java/lang/Threadd
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> >>>> >>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Method)
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Caused by:
> >>>> >>>>>> java.lang.ClassNotFoundException:
> >>>> >>>>>> > >>>>>>>
> java.lang.Threadd
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>>
> java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
> >>>> >>>>>> > >>>>>>> ...
> 3 more
> >>>> >>>>>> > >>>>>>> FATAL ERROR
> in
> >>>> native
> >>>> >>>>>> method: JNI method
> >>>> >>>>>> > FindClass
> >>>> >>>>>> > >>>>>>> : internal
> error
> >>>> from
> >>>> >>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> >>>> >>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Method)
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> at
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> Questions/comments I
> >>>> >>>>>> have about this are:
> >>>> >>>>>> > >>>>>>> - Do we
> want to
> >>>> >>>>>> force fatal errors when a JNI
> >>>> >>>>>> > >>>>>>> call fails
> in
> >>>> >>>>>> general? Most of these tests
> >>>> >>>>>> > do the
> >>>> >>>>>> > >>>>>>> right thing
> and
> >>>> test
> >>>> >>>>>> the return of the JNI
> >>>> >>>>>> > calls,
> >>>> >>>>>> > >>>>>>> for example:
> >>>> >>>>>> > >>>>>>>
> thrClass =
> >>>> >>>>>> > jni->FindClass("java/lang/Threadd",
> >>>> >>>>>> > >>>>>>>
> TRACE_JNI_CALL);
> >>>> >>>>>> > >>>>>>> if
> (thrClass
> >>>> ==
> >>>> >>>>>> NULL) {
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> but now the
> >>>> wrapper
> >>>> >>>>>> actually would do a
> >>>> >>>>>> > fatal if
> >>>> >>>>>> > >>>>>>> the
> FindClass call
> >>>> >>>>>> would return a nullptr,
> >>>> >>>>>> > so we
> >>>> >>>>>> > >>>>>>> could
> remove that
> >>>> >>>>>> test altogether. What do you
> >>>> >>>>>> > >>>>>>> think?
> >>>> >>>>>> > >>>>>>> - I
> prefer to
> >>>> >>>>>> leave them as the tests then
> >>>> >>>>>> > >>>>>>> become
> closer to
> >>>> what
> >>>> >>>>>> real users would have in
> >>>> >>>>>> > >>>>>>> their code
> and is
> >>>> the
> >>>> >>>>>> "recommended" way of
> >>>> >>>>>> > doing it
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> - The
> >>>> alternative
> >>>> >>>>>> is to use the
> >>>> >>>>>> > NonFatalError I
> >>>> >>>>>> > >>>>>>> added which
> then
> >>>> just
> >>>> >>>>>> prints out that something
> >>>> >>>>>> > >>>>>>> went wrong,
> >>>> letting
> >>>> >>>>>> the test continue. Question
> >>>> >>>>>> > >>>>>>> will be what
> >>>> should
> >>>> >>>>>> be the default? The
> >>>> >>>>>> > fatal or
> >>>> >>>>>> > >>>>>>> the
> non-fatal
> >>>> error
> >>>> >>>>>> handling?
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> On a
> different
> >>>> >>>>>> subject:
> >>>> >>>>>> > >>>>>>> - On the
> new
> >>>> tests,
> >>>> >>>>>> I've removed the
> >>>> >>>>>> > >>>>>>>
> NSK_JNI_VERIFY
> >>>> since
> >>>> >>>>>> the JNI wrapper
> >>>> >>>>>> > handles the
> >>>> >>>>>> > >>>>>>> tracing and
> the
> >>>> >>>>>> verify in almost the same
> >>>> >>>>>> > way; only
> >>>> >>>>>> > >>>>>>> difference
> I can
> >>>> >>>>>> really tell is that the
> >>>> >>>>>> > complain
> >>>> >>>>>> > >>>>>>> method from
> NSK
> >>>> has a
> >>>> >>>>>> max complain before
> >>>> >>>>>> > stopping
> >>>> >>>>>> > >>>>>>> to
> "complain"; I
> >>>> have
> >>>> >>>>>> not added that part
> >>>> >>>>>> > of the
> >>>> >>>>>> > >>>>>>> code in this
> >>>> webrev
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Once we
> decide on
> >>>> >>>>>> these, I can continue on the
> >>>> >>>>>> > >>>>>>> files from
> >>>> >>>>>> JDK-8212884 and then do both the
> >>>> >>>>>> > >>>>>>> assignment
> in an
> >>>> if
> >>>> >>>>>> extraction followed-by this
> >>>> >>>>>> > >>>>>>> type of
> webrev in
> >>>> an
> >>>> >>>>>> easier fashion.
> >>>> >>>>>> > Depending on
> >>>> >>>>>> > >>>>>>> decisions
> here,
> >>>> >>>>>> NSK*VERIFY can be deprecated as
> >>>> >>>>>> > >>>>>>> well as we
> go
> >>>> forward.
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Thanks!
> >>>> >>>>>> > >>>>>>> Jc
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> On Mon, Nov
> 19,
> >>>> 2018
> >>>> >>>>>> at 11:34 AM Chris Plummer
> >>>> >>>>>> > >>>>>>> <
> >>>> chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>
> >>>> >>>>>> > >>>>>>> <mailto:
> chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>
> >>>> >>>>>> > <mailto:chris.plummer at oracle.com
> >>>> >>>>>> <mailto:chris.plummer at oracle.com>>>> wrote:
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> On
> 11/19/18
> >>>> 10:07
> >>>> >>>>>> AM, JC Beyler wrote:
> >>>> >>>>>> > >>>>>>>> Hi all,
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> @David/Chris:
> >>>> >>>>>> should I then push this
> >>>> >>>>>> > RFR to
> >>>> >>>>>> > >>>>>>>> the
> hotspot
> >>>> >>>>>> mailing or the runtime
> >>>> >>>>>> > one? For
> >>>> >>>>>> > >>>>>>>> what
> it's
> >>>> worth,
> >>>> >>>>>> a lot of the tests
> >>>> >>>>>> > under the
> >>>> >>>>>> > >>>>>>>>
> vmTestbase
> >>>> are
> >>>> >>>>>> jvmti so the review also
> >>>> >>>>>> > >>>>>>>> affects
> >>>> >>>>>> serviceability; it just turns
> >>>> >>>>>> > out I
> >>>> >>>>>> > >>>>>>>>
> started with
> >>>> the
> >>>> >>>>>> GC originally and
> >>>> >>>>>> > then hit
> >>>> >>>>>> > >>>>>>>> some
> other
> >>>> tests
> >>>> >>>>>> I had touched via the
> >>>> >>>>>> > >>>>>>>>
> assignment
> >>>> >>>>>> extraction.
> >>>> >>>>>> > >>>>>>> I think
> >>>> hotspot
> >>>> >>>>>> would be best.
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> Chris
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> @Serguei:
> >>>> Done
> >>>> >>>>>> for the method
> >>>> >>>>>> > renaming, for
> >>>> >>>>>> > >>>>>>>> the
> indent,
> >>>> are
> >>>> >>>>>> you talking about
> >>>> >>>>>> > going from
> >>>> >>>>>> > >>>>>>>> the
> 8-indent
> >>>> to
> >>>> >>>>>> 4-indent? If so, would
> >>>> >>>>>> > it not
> >>>> >>>>>> > >>>>>>>> just be
> >>>> better
> >>>> >>>>>> to do a new JBS bug and
> >>>> >>>>>> > do the
> >>>> >>>>>> > >>>>>>>> whole
> files
> >>>> in
> >>>> >>>>>> one go? I ask because
> >>>> >>>>>> > >>>>>>>>
> otherwise, it
> >>>> >>>>>> will look a bit weird to
> >>>> >>>>>> > have
> >>>> >>>>>> > >>>>>>>> parts
> of the
> >>>> >>>>>> file as 8-indent and others
> >>>> >>>>>> > >>>>>>>> 4-indent?
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> Thanks
> for
> >>>> >>>>>> looking at it!
> >>>> >>>>>> > >>>>>>>> Jc
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> On
> Mon, Nov
> >>>> 19,
> >>>> >>>>>> 2018 at 1:25 AM
> >>>> >>>>>> > >>>>>>>> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>>>> > >>>>>>>> <mailto:
> >>>> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>>
> >>>> >>>>>> > >>>>>>>> <serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>
> >>>> >>>>>> > >>>>>>>> <mailto:
> >>>> serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>
> >>>> >>>>>> > <mailto:serguei.spitsyn at oracle.com
> >>>> >>>>>> <mailto:serguei.spitsyn at oracle.com>>>> wrote:
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> Hi
> Jc,
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> We
> have
> >>>> to
> >>>> >>>>>> start this review
> >>>> >>>>>> > anyway. :)
> >>>> >>>>>> > >>>>>>>> It
> looks
> >>>> >>>>>> good to me in general.
> >>>> >>>>>> > >>>>>>>>
> Thank you
> >>>> >>>>>> for your consistency in this
> >>>> >>>>>> > >>>>>>>>
> >>>> refactoring!
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> Some
> >>>> minor
> >>>> >>>>>> comments.
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> +static
> >>>> >>>>>> const char*
> >>>> >>>>>> > remove_folders(const
> >>>> >>>>>> > >>>>>>>>
> char*
> >>>> >>>>>> fullname) { I'd suggest to
> >>>> >>>>>> > rename
> >>>> >>>>>> > >>>>>>>> the
> >>>> function
> >>>> >>>>>> name to something
> >>>> >>>>>> > traditional
> >>>> >>>>>> > >>>>>>>>
> like
> >>>> >>>>>> get_basename. Otherwise, it
> >>>> >>>>>> > sounds
> >>>> >>>>>> > >>>>>>>>
> like this
> >>>> >>>>>> function has to really
> >>>> >>>>>> > remove
> >>>> >>>>>> > >>>>>>>>
> folders.
> >>>> :)
> >>>> >>>>>> Also, all *Locker.cpp have
> >>>> >>>>>> > >>>>>>>>
> wrong
> >>>> indent
> >>>> >>>>>> in the bodies of if
> >>>> >>>>>> > and while
> >>>> >>>>>> > >>>>>>>>
> >>>> statements.
> >>>> >>>>>> Could this be fixed
> >>>> >>>>>> > with the
> >>>> >>>>>> > >>>>>>>>
> >>>> refactoring?
> >>>> >>>>>> I did not look on how
> >>>> >>>>>> > this
> >>>> >>>>>> > >>>>>>>>
> impacts
> >>>> the
> >>>> >>>>>> tests other than
> >>>> >>>>>> > >>>>>>>> serviceability.
> >>>> Thanks,
> >>>> >>>>>> Serguei
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> On
> >>>> 11/16/18
> >>>> >>>>>> 19:43, JC Beyler wrote:
> >>>> >>>>>> > >>>>>>>>> Hi all,
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> Anybody
> >>>> >>>>>> motivated to review this? :)
> >>>> >>>>>> > >>>>>>>>> Jc
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> On
> Wed, Nov
> >>>> 7,
> >>>> >>>>>> 2018 at 9:53 PM JC
> >>>> >>>>>> > Beyler
> >>>> >>>>>> > >>>>>>>>> <
> jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>
> >>>> >>>>>> > <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>>
> >>>> >>>>>> > >>>>>>>>> <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>
> >>>> >>>>>> > <mailto:jcbeyler at google.com
> >>>> >>>>>> <mailto:jcbeyler at google.com>>>> wrote:
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> Hi
> all,
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Could I
> >>>> have
> >>>> >>>>>> a review for the
> >>>> >>>>>> > >>>>>>>>>
> extension
> >>>> >>>>>> and usage of the
> >>>> >>>>>> > >>>>>>>>> ExceptionJniWrapper. This
> >>>> >>>>>> > adds lines
> >>>> >>>>>> > >>>>>>>>> and
> >>>> >>>>>> filenames to the end of the
> >>>> >>>>>> > >>>>>>>>>
> wrapper
> >>>> JNI
> >>>> >>>>>> methods, adds
> >>>> >>>>>> > tracing,
> >>>> >>>>>> > >>>>>>>>> and
> >>>> throws
> >>>> >>>>>> an error if need
> >>>> >>>>>> > be. I've
> >>>> >>>>>> > >>>>>>>>>
> ported
> >>>> the
> >>>> >>>>>> gc/lock files to
> >>>> >>>>>> > use the
> >>>> >>>>>> > >>>>>>>>> new
> >>>> >>>>>> TRACE_JNI_CALL add-on and
> >>>> >>>>>> > I've
> >>>> >>>>>> > >>>>>>>>>
> ported a
> >>>> few
> >>>> >>>>>> of the tests
> >>>> >>>>>> > that were
> >>>> >>>>>> > >>>>>>>>>
> already
> >>>> >>>>>> changed for the
> >>>> >>>>>> > assignment
> >>>> >>>>>> > >>>>>>>>>
> webrev
> >>>> for
> >>>> >>>>>> JDK-8212884.
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Webrev:
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.00/
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/
> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> <http://cr.openjdk.java.net/%7Ejcbeyler/8213501/webrev.00/
> >
> >>>> >>>>>> > >>>>>>>>>
> Bug:
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> For
> >>>> >>>>>> illustration, if I force
> >>>> >>>>>> > an error
> >>>> >>>>>> > >>>>>>>>> to
> the
> >>>> >>>>>> AP04/ap04t03 test and
> >>>> >>>>>> > set the
> >>>> >>>>>> > >>>>>>>>>
> verbosity
> >>>> >>>>>> on, I get something
> >>>> >>>>>> > like:
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> >>
> >>>> Calling
> >>>> >>>>>> JNI method
> >>>> >>>>>> > FindClass from
> >>>> >>>>>> > >>>>>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>>>> >>
> >>>> Calling
> >>>> >>>>>> with these
> >>>> >>>>>> > parameter(s):
> >>>> >>>>>> > >>>>>>>>> java/lang/Threadd
> >>>> >>>>>> > >>>>>>>>>
> Wait for
> >>>> >>>>>> thread to finish
> >>>> >>>>>> > >>>>>>>>> <<
> Called
> >>>> >>>>>> JNI method
> >>>> >>>>>> > FindClass from
> >>>> >>>>>> > >>>>>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>>>>
> >>>> Exception in
> >>>> >>>>>> thread "Thread-0"
> >>>> >>>>>> > >>>>>>>>> java.lang.NoClassDefFoundError:
> >>>> >>>>>> > >>>>>>>>> java/lang/Threadd
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> >>>> >>>>>>
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Method)
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Caused
> >>>> by:
> >>>> >>>>>> > >>>>>>>>>
> java.lang.ClassNotFoundException:
> >>>> >>>>>> > >>>>>>>>> java.lang.Threadd
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
> >>>> >>>>>> > >>>>>>>>>
> ... 3
> >>>> more
> >>>> >>>>>> > >>>>>>>>>
> FATAL
> >>>> ERROR
> >>>> >>>>>> in native method: JNI
> >>>> >>>>>> > >>>>>>>>>
> method
> >>>> >>>>>> FindClass : internal error
> >>>> >>>>>> > >>>>>>>>>
> from
> >>>> >>>>>> ap04t003.cpp:343
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
> >>>> >>>>>>
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Method)
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
> >>>>
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> at
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> >
> >>>> >>>>>>
> >>>>
> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
> >>>> >>>>>>
> >>>> >>>>>> >
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> Questions/comments I have about
> >>>> >>>>>> > >>>>>>>>> this are:
> >>>> >>>>>> > >>>>>>>>>
> - Do we
> >>>> >>>>>> want to force fatal
> >>>> >>>>>> > errors
> >>>> >>>>>> > >>>>>>>>>
> when a
> >>>> JNI
> >>>> >>>>>> call fails in general?
> >>>> >>>>>> > >>>>>>>>>
> Most of
> >>>> >>>>>> these tests do the right
> >>>> >>>>>> > >>>>>>>>>
> thing and
> >>>> >>>>>> test the return of
> >>>> >>>>>> > the JNI
> >>>> >>>>>> > >>>>>>>>>
> calls,
> >>>> for
> >>>> >>>>>> example:
> >>>> >>>>>> > >>>>>>>>>
> thrClass
> >>>> =
> >>>> >>>>>> > >>>>>>>>>
> jni->FindClass("java/lang/Threadd",
> >>>> >>>>>> > >>>>>>>>> TRACE_JNI_CALL);
> >>>> >>>>>> > >>>>>>>>>
> if
> >>>> >>>>>> (thrClass == NULL) {
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> but now
> >>>> the
> >>>> >>>>>> wrapper actually
> >>>> >>>>>> > would do
> >>>> >>>>>> > >>>>>>>>> a
> fatal
> >>>> if
> >>>> >>>>>> the FindClass call
> >>>> >>>>>> > would
> >>>> >>>>>> > >>>>>>>>>
> return a
> >>>> >>>>>> nullptr, so we could
> >>>> >>>>>> > remove
> >>>> >>>>>> > >>>>>>>>>
> that test
> >>>> >>>>>> altogether. What do
> >>>> >>>>>> > you
> >>>> >>>>>> > >>>>>>>>> think?
> >>>> >>>>>> > >>>>>>>>>
> - I
> >>>> >>>>>> prefer to leave them
> >>>> >>>>>> > as the
> >>>> >>>>>> > >>>>>>>>>
> tests
> >>>> then
> >>>> >>>>>> become closer to
> >>>> >>>>>> > what real
> >>>> >>>>>> > >>>>>>>>>
> users
> >>>> would
> >>>> >>>>>> have in their
> >>>> >>>>>> > code and is
> >>>> >>>>>> > >>>>>>>>> the
> >>>> >>>>>> "recommended" way of doing it
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> - The
> >>>> >>>>>> alternative is to
> >>>> >>>>>> > use the
> >>>> >>>>>> > >>>>>>>>> NonFatalError I
> >>>> added
> >>>> >>>>>> which
> >>>> >>>>>> > then just
> >>>> >>>>>> > >>>>>>>>>
> prints
> >>>> out
> >>>> >>>>>> that something
> >>>> >>>>>> > went wrong,
> >>>> >>>>>> > >>>>>>>>>
> letting
> >>>> the
> >>>> >>>>>> test continue.
> >>>> >>>>>> > Question
> >>>> >>>>>> > >>>>>>>>>
> will be
> >>>> what
> >>>> >>>>>> should be the
> >>>> >>>>>> > default?
> >>>> >>>>>> > >>>>>>>>> The
> >>>> fatal or
> >>>> >>>>>> the non-fatal error
> >>>> >>>>>> > >>>>>>>>>
> handling?
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> On
> a
> >>>> >>>>>> different subject:
> >>>> >>>>>> > >>>>>>>>>
> - On
> >>>> the
> >>>> >>>>>> new tests, I've
> >>>> >>>>>> > removed
> >>>> >>>>>> > >>>>>>>>> the
> >>>> >>>>>> NSK_JNI_VERIFY since the JNI
> >>>> >>>>>> > >>>>>>>>>
> wrapper
> >>>> >>>>>> handles the tracing
> >>>> >>>>>> > and the
> >>>> >>>>>> > >>>>>>>>>
> verify in
> >>>> >>>>>> almost the same
> >>>> >>>>>> > way; only
> >>>> >>>>>> > >>>>>>>>>
> >>>> difference I
> >>>> >>>>>> can really tell
> >>>> >>>>>> > is that
> >>>> >>>>>> > >>>>>>>>> the
> >>>> complain
> >>>> >>>>>> method from NSK
> >>>> >>>>>> > has a
> >>>> >>>>>> > >>>>>>>>> max
> >>>> complain
> >>>> >>>>>> before stopping to
> >>>> >>>>>> > >>>>>>>>>
> >>>> "complain";
> >>>> >>>>>> I have not added that
> >>>> >>>>>> > >>>>>>>>>
> part of
> >>>> the
> >>>> >>>>>> code in this webrev
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Once we
> >>>> >>>>>> decide on these, I can
> >>>> >>>>>> > >>>>>>>>>
> continue
> >>>> on
> >>>> >>>>>> the files from
> >>>> >>>>>> > >>>>>>>>>
> >>>> JDK-8212884
> >>>> >>>>>> and then do both the
> >>>> >>>>>> > >>>>>>>>>
> >>>> assignment
> >>>> >>>>>> in an if extraction
> >>>> >>>>>> > >>>>>>>>>
> >>>> followed-by
> >>>> >>>>>> this type of
> >>>> >>>>>> > webrev in an
> >>>> >>>>>> > >>>>>>>>>
> easier
> >>>> >>>>>> fashion. Depending on
> >>>> >>>>>> > >>>>>>>>>
> decisions
> >>>> >>>>>> here, NSK*VERIFY can be
> >>>> >>>>>> > >>>>>>>>>
> >>>> deprecated
> >>>> >>>>>> as well as we go
> >>>> >>>>>> > forward.
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> Thank you
> >>>> >>>>>> for the
> >>>> >>>>>> > reviews/comments :)
> >>>> >>>>>> > >>>>>>>>> Jc
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>>
> >>>> >>>>>> > >>>>>>>>> --
> >>>> >>>>>> > >>>>>>>>> Thanks,
> >>>> >>>>>> > >>>>>>>>> Jc
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>>
> >>>> >>>>>> > >>>>>>>> --
> >>>> >>>>>> > >>>>>>>> Thanks,
> >>>> >>>>>> > >>>>>>>> Jc
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>>
> >>>> >>>>>> > >>>>>>> --
> >>>> >>>>>> > >>>>>>> Thanks,
> >>>> >>>>>> > >>>>>>> Jc
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>>
> >>>> >>>>>> > >>>>>> --
> >>>> >>>>>> > >>>>>> Thanks,
> >>>> >>>>>> > >>>>>> Jc
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>>
> >>>> >>>>>> > >>>>> --
> >>>> >>>>>> > >>>>> Thanks,
> >>>> >>>>>> > >>>>> Jc
> >>>> >>>>>> > >>>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> --
> >>>> >>>>>> > >>> Thanks,
> >>>> >>>>>> > >>> Jc
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> --
> >>>> >>>>>> > >>>
> >>>> >>>>>> > >>> Thanks,
> >>>> >>>>>> > >>> Jc
> >>>> >>>>>> > >
> >>>> >>>>>> > >
> >>>> >>>>>> >
> >>>> >>>>>> >
> >>>> >>>>>> >
> >>>> >>>>>> > --
> >>>> >>>>>> >
> >>>> >>>>>> > Thanks,
> >>>> >>>>>> > Jc
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>>
> >>>> >>>>>> --
> >>>> >>>>>> Thanks,
> >>>> >>>>>> Jc
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>>
> >>>> >>>>> --
> >>>> >>>>> Thanks,
> >>>> >>>>> Jc
> >>>> >>>>
> >>>> >>>>
> >>>> >>>>
> >>>> >>>> --
> >>>> >>>> Thanks,
> >>>> >>>> Jc
> >>>> >>>>
> >>>> >>>>
> >>>> >>>>
> >>>> >>>> --
> >>>> >>>>
> >>>> >>>> Thanks,
> >>>> >>>> Jc
> >>>> >>>
> >>>> >
> >>>>
> >>>
> >>>
> >>> --
> >>>
> >>> Thanks,
> >>> Jc
> >>>
> >>
> >>
> >> --
> >>
> >> Thanks,
> >> Jc
> >>
> >
> >
> > --
> >
> > Thanks,
> > Jc
> >
>
>
> --
>
> Thanks,
> Jc
>
>
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list