RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Mon Dec 17 16:43:41 UTC 2018
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
More information about the hotspot-dev
mailing list