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