RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Tue Jan 8 18:05:32 UTC 2019
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
More information about the hotspot-dev
mailing list