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