RFR (L) 8213501 : Deploy ExceptionJniWrapper for a few tests
JC Beyler
jcbeyler at google.com
Wed Dec 12 05:16:59 UTC 2018
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 :-)
Webrev: http://cr.openjdk.java.net/~jcbeyler/8213501/webrev.05/
Bug: https://bugs.openjdk.java.net/browse/JDK-8213501
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
More information about the hotspot-dev
mailing list