RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Thu Dec 13 04:06:00 UTC 2018
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
More information about the hotspot-dev
mailing list