RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Wed Dec 12 21:47:37 UTC 2018
On Wed, Dec 12, 2018 at 11:37 AM serguei.spitsyn at oracle.com <
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
>
That is a different problem here because the namespace is not anonymous.
Because it is not an anonymous namespace and because the variable is not
declared static, the variable can be defined outside of the scope of the
namespace by doing something like:
namespace foo {
extern int bar;
}
But if you declared the namespace as anonymous, that is no longer possible
and you'd get an undefined reference. Also this is different because the
stack overflow post was about putting this declaration in a .h and being
included in two different files :-).
Let me know if there are any other questions and if someone else could
review the newest webrev, that would be awesome :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.05/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
Thanks,
Jc
>
>
>
> Webrev: http://cr.openjdk.java.net/~jcbeyler/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> 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 <
>> 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 <
>>> 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/
>>>> 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/
>>>>
>>>> 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
>>>> [2]: http://cr.openjdk.java.net/~jcbeyler/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
>>>>
>>>> On Mon, Dec 3, 2018 at 11:29 PM David Holmes <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/
>>>>> > 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> <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/
>>>>> > >>> 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>>>
>>>>> 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>>>
>>>>> > 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>> 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>>>
>>>>> > >>>>> 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/>
>>>>> > >>>>>> 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
>>>>> >
>>>>> >
>>>>> > >>>>>>
>>>>> > >>>>>>
>>>>> > >>>>>> 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>>> 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
>>>>> >
>>>>> >
>>>>> > >>>>>>
>>>>> > >>>>>>
>>>>> > >>>>>> 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>
>>>>> > >>>>>>> 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>>> 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>>
>>>>> > >>>>>>>> <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>>> 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/>
>>>>> > >>>>>>>>> Bug:
>>>>> > >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213501
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> For illustration, if I
>>>>> force
>>>>> > an error
>>>>> > >>>>>>>>> to the AP04/ap04t03 test
>>>>> and
>>>>> > set the
>>>>> > >>>>>>>>> verbosity on, I get
>>>>> something
>>>>> > like:
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> >> Calling JNI method
>>>>> > FindClass from
>>>>> > >>>>>>>>> ap04t003.cpp:343
>>>>> > >>>>>>>>> >> Calling with these
>>>>> > parameter(s):
>>>>> > >>>>>>>>> java/lang/Threadd
>>>>> > >>>>>>>>> Wait for thread to finish
>>>>> > >>>>>>>>> << Called JNI method
>>>>> > FindClass from
>>>>> > >>>>>>>>> ap04t003.cpp:343
>>>>> > >>>>>>>>> Exception in thread
>>>>> "Thread-0"
>>>>> > >>>>>>>>> java.lang.NoClassDefFoundError:
>>>>> > >>>>>>>>> java/lang/Threadd
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Method)
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Caused by:
>>>>> > >>>>>>>>> java.lang.ClassNotFoundException:
>>>>> > >>>>>>>>> java.lang.Threadd
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:583)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> > java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:521)
>>>>> > >>>>>>>>> ... 3 more
>>>>> > >>>>>>>>> FATAL ERROR in native
>>>>> method: JNI
>>>>> > >>>>>>>>> method FindClass :
>>>>> internal error
>>>>> > >>>>>>>>> from ap04t003.cpp:343
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003.runIterateOverHeap(Native
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Method)
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003HeapIterator.runIteration(ap04t003.java:140)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> at
>>>>> > >>>>>>>>>
>>>>> >
>>>>> nsk.jvmti.scenarios.allocation.AP04.ap04t003Thread.run(ap04t003.java:201)
>>>>> >
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Questions/comments I have
>>>>> about
>>>>> > >>>>>>>>> this are:
>>>>> > >>>>>>>>> - Do we want to force
>>>>> fatal
>>>>> > errors
>>>>> > >>>>>>>>> when a JNI call fails in
>>>>> general?
>>>>> > >>>>>>>>> Most of these tests do
>>>>> the right
>>>>> > >>>>>>>>> thing and test the return
>>>>> of
>>>>> > the JNI
>>>>> > >>>>>>>>> calls, for example:
>>>>> > >>>>>>>>> thrClass =
>>>>> > >>>>>>>>> jni->FindClass("java/lang/Threadd",
>>>>> > >>>>>>>>> TRACE_JNI_CALL);
>>>>> > >>>>>>>>> if (thrClass == NULL)
>>>>> {
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> but now the wrapper
>>>>> actually
>>>>> > would do
>>>>> > >>>>>>>>> a fatal if the FindClass
>>>>> call
>>>>> > would
>>>>> > >>>>>>>>> return a nullptr, so we
>>>>> could
>>>>> > remove
>>>>> > >>>>>>>>> that test altogether.
>>>>> What do
>>>>> > you
>>>>> > >>>>>>>>> think?
>>>>> > >>>>>>>>> - I prefer to leave
>>>>> them
>>>>> > as the
>>>>> > >>>>>>>>> tests then become closer
>>>>> to
>>>>> > what real
>>>>> > >>>>>>>>> users would have in their
>>>>> > code and is
>>>>> > >>>>>>>>> the "recommended" way of
>>>>> doing it
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> - The alternative is to
>>>>> > use the
>>>>> > >>>>>>>>> NonFatalError I added
>>>>> which
>>>>> > then just
>>>>> > >>>>>>>>> prints out that something
>>>>> > went wrong,
>>>>> > >>>>>>>>> letting the test continue.
>>>>> > Question
>>>>> > >>>>>>>>> will be what should be the
>>>>> > default?
>>>>> > >>>>>>>>> The fatal or the
>>>>> non-fatal error
>>>>> > >>>>>>>>> handling?
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> On a different subject:
>>>>> > >>>>>>>>> - On the new tests, I've
>>>>> > removed
>>>>> > >>>>>>>>> the NSK_JNI_VERIFY since
>>>>> the JNI
>>>>> > >>>>>>>>> wrapper handles the
>>>>> tracing
>>>>> > and the
>>>>> > >>>>>>>>> verify in almost the same
>>>>> > way; only
>>>>> > >>>>>>>>> difference I can really
>>>>> tell
>>>>> > is that
>>>>> > >>>>>>>>> the complain method from
>>>>> NSK
>>>>> > has a
>>>>> > >>>>>>>>> max complain before
>>>>> stopping to
>>>>> > >>>>>>>>> "complain"; I have not
>>>>> added that
>>>>> > >>>>>>>>> part of the code in this
>>>>> webrev
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Once we decide on these,
>>>>> I can
>>>>> > >>>>>>>>> continue on the files from
>>>>> > >>>>>>>>> JDK-8212884 and then do
>>>>> both the
>>>>> > >>>>>>>>> assignment in an if
>>>>> extraction
>>>>> > >>>>>>>>> followed-by this type of
>>>>> > webrev in an
>>>>> > >>>>>>>>> easier fashion. Depending
>>>>> on
>>>>> > >>>>>>>>> decisions here,
>>>>> NSK*VERIFY can be
>>>>> > >>>>>>>>> deprecated as well as we
>>>>> go
>>>>> > forward.
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> Thank you for the
>>>>> > reviews/comments :)
>>>>> > >>>>>>>>> Jc
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>>
>>>>> > >>>>>>>>> --
>>>>> > >>>>>>>>> Thanks,
>>>>> > >>>>>>>>> Jc
>>>>> > >>>>>>>>
>>>>> > >>>>>>>>
>>>>> > >>>>>>>>
>>>>> > >>>>>>>> --
>>>>> > >>>>>>>> Thanks,
>>>>> > >>>>>>>> Jc
>>>>> > >>>>>>>
>>>>> > >>>>>>>
>>>>> > >>>>>>>
>>>>> > >>>>>>>
>>>>> > >>>>>>> --
>>>>> > >>>>>>> Thanks,
>>>>> > >>>>>>> Jc
>>>>> > >>>>>>
>>>>> > >>>>>>
>>>>> > >>>>>>
>>>>> > >>>>>>
>>>>> > >>>>>> --
>>>>> > >>>>>> Thanks,
>>>>> > >>>>>> Jc
>>>>> > >>>>>
>>>>> > >>>>>
>>>>> > >>>>>
>>>>> > >>>>>
>>>>> > >>>>> --
>>>>> > >>>>> Thanks,
>>>>> > >>>>> Jc
>>>>> > >>>>
>>>>> > >>>
>>>>> > >>>
>>>>> > >>>
>>>>> > >>> --
>>>>> > >>> Thanks,
>>>>> > >>> Jc
>>>>> > >>>
>>>>> > >>>
>>>>> > >>>
>>>>> > >>> --
>>>>> > >>>
>>>>> > >>> Thanks,
>>>>> > >>> Jc
>>>>> > >
>>>>> > >
>>>>> >
>>>>> >
>>>>> >
>>>>> > --
>>>>> >
>>>>> > Thanks,
>>>>> > Jc
>>>>>
>>>>
>>>>
>>>> --
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>>
>>>>
>>>
>>> --
>>>
>>> Thanks,
>>> Jc
>>>
>>>
>>>
>>
>> --
>>
>> Thanks,
>> Jc
>>
>
>
> --
>
> Thanks,
> Jc
>
>
>
--
Thanks,
Jc
More information about the hotspot-dev
mailing list