RFR (M) 8213003: Remove more assignments in ifs for vmTestbase/[a-s]

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Tue Nov 6 20:23:04 UTC 2018


Okay.
I'm not sure I fully understandd what is current plan.

My view is that we can do the following steps:
  1) Jc can push what was already reviewed.
     With this change we will miss extra tracing for JNI calls and results.
  2) Work on using the ExceptionCheckingJni that will restore
     this tracing but in a different fashion.

Is it correct?

Thanks,
Serguei

On 11/6/18 11:57 AM, serguei.spitsyn at oracle.com wrote:
>
> On 11/6/18 11:14 AM, Chris Plummer wrote:
>> Hi JC,
>>
>> The exception changes looked ok to me, but it would be good to get a 
>> second approval before moving forward with them since they are pretty 
>> significant.
>
> The exception changes need to be discussed after a separate RFR is posted.
>
> Thanks,
> Serguei
>
>> thanks,
>>
>> Chris
>>
>> On 11/2/18 9:09 PM, JC Beyler wrote:
>>> Hi Chris,
>>>
>>> Yes that is correct, the webrev in this email thread would be 
>>> postponed and done differently.
>>>
>>> Most likely we'd have to do smaller changes to extend 
>>> ExceptionCheckingJni and work on replacing the NSK*VERIFY macros by 
>>> using the ExceptionCheckingJni wrapper using something similar to 
>>> the change I show in http://cr.openjdk.java.net/~jcbeyler/exception/ 
>>> <http://cr.openjdk.java.net/%7Ejcbeyler/exception/>.
>>>
>>> I guess the question becomes really: are the changes in 
>>> http://cr.openjdk.java.net/~jcbeyler/exception/ 
>>> <http://cr.openjdk.java.net/%7Ejcbeyler/exception/> seem in theory 
>>> at least reasonable? Then I could start working on it but it will 
>>> most likely be a slower going process.
>>>
>>> Let me know,
>>> Jc
>>>
>>> On Fri, Nov 2, 2018 at 8:44 PM Chris Plummer 
>>> <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>> wrote:
>>>
>>>     Hi JC,
>>>
>>>     So assuming the change to move the assignment outside of the
>>>     conditional is already in place, you are changing:
>>>
>>>                    debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
>>>
>>>     to
>>>
>>>              ExceptionCheckingJniEnvPtr jni_wrapper(jni);
>>>              ...
>>>                  debugeeClass =
>>>     jni_wrapper->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
>>>
>>>     But none of your ExceptionCheckingJni changes are pushed yet, right?
>>>
>>>     thanks,
>>>
>>>     Chris
>>>
>>>     On 11/2/18 10:44 AM, JC Beyler wrote:
>>>>     Hi Chris,
>>>>
>>>>     Here is what it would "look like", there is a bit more clean up
>>>>     to make it true for all methods, handling the "verbose" flag,
>>>>     etc but it should help see what I'm talking about:
>>>>     http://cr.openjdk.java.net/~jcbeyler/exception/
>>>>     <http://cr.openjdk.java.net/%7Ejcbeyler/exception/>
>>>>
>>>>     Basically:
>>>>        - The test now no longer needs the NSK_JNI_VERIFY but
>>>>     requires a TRACE_JNI_CALL as last argument to the JNI calls if
>>>>     you want
>>>>     http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html
>>>>     <http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp.udiff.html>
>>>>           Note: I left the nsk_setVerboseMode call though this
>>>>     version does not care about that flag but I would do it to be
>>>>     conforming of course
>>>>
>>>>        - The wrapper class would have a new macro TRACE_JNI_CALL
>>>>     and the methods would have new parameters for the line and file
>>>>     that are defaulted to -1 and 0 so that we can know if they are
>>>>     used or not:
>>>>     http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html
>>>>     <http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.hpp.udiff.html>
>>>>
>>>>        - Finally, the real change comes from here:
>>>>     http://cr.openjdk.java.net/~jcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html
>>>>     <http://cr.openjdk.java.net/%7Ejcbeyler/exception/test/hotspot/jtreg/vmTestbase/nsk/share/jni/ExceptionCheckingJniEnv.cpp.udiff.html>
>>>>
>>>>          Where we do this:
>>>>             a) add a new constructor for the JNIVerifier class for
>>>>     having the parameter of the JNI call be passed in for printing
>>>>     the value of, this will sadly have to be duplicated by the
>>>>     number of different argument numbers since variadic templates
>>>>     is C++11 onwards but that is not a huge problem since it is
>>>>     contained to this file
>>>>             b) at JNIVerifier construction, we print it out the
>>>>     "before" call and this should be protected by the verbose flag
>>>>     like the nsk_ltrace/nsk_verify methods do it for compatibility
>>>>             c) at JNIVerifier construction, we now can call the
>>>>     print parameter methods to pass the values of the call to be
>>>>     printed out
>>>>               - again sadly without c++11 we will have to have a
>>>>     few of these here but that is limited to this part of the code
>>>>     so should be fine
>>>>             d) the JNI wrapper methods get augmented by the
>>>>     line/file parameters which default to -1 and 0
>>>>             e) the constructor is now also passing in the JNI
>>>>     method parameters
>>>>
>>>>     It's mostly boiler plate code but allows us to go from:
>>>>     -            if (!NSK_JNI_VERIFY(jni, (debugeeClass =
>>>>     - jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
>>>>     + debugeeClass = jni_wrapper->FindClass(DEBUGEE_CLASS_NAME,
>>>>     TRACE_JNI_CALL);
>>>>     +            if (debugeeClass != NULL) {
>>>>
>>>>     And print out:
>>>>     >> Calling JNI method FindClass from
>>>>     /usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>     >> Calling with these parameter(s):
>>>>     nsk/jvmti/SetTag/settag001
>>>>     << Called JNI method FindClass from
>>>>     /usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>     FATAL ERROR in native method: JNI method FindClass : Return is
>>>>     NULL from
>>>>     /usr/local/google/home/jcbeyler/mercurial/jdk12-verify/test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>
>>>>     With the >> and << lines being the "equivalent" to the trace
>>>>     methods and the "FATAL ERROR being the equivalent to the
>>>>     NSK_COMPLAIN done in the verify method.
>>>>
>>>>     Let me know what you think,
>>>>     Jc
>>>>
>>>>             d) at JNIVerifier destruction, we can print out the
>>>>     "called the method"
>>>>
>>>>
>>>>
>>>>     On Thu, Nov 1, 2018 at 1:24 PM Chris Plummer
>>>>     <chris.plummer at oracle.com <mailto:chris.plummer at oracle.com>> wrote:
>>>>
>>>>         Hi JC,
>>>>
>>>>         I would need to see a webrev with the ExceptionCheckingJni
>>>>         support, new macros, and a few example uses. You don't need
>>>>         to fix everything. I just want to get a better feel for the
>>>>         changes and what the implementation would look like.
>>>>
>>>>         thanks,
>>>>
>>>>         Chris
>>>>
>>>>         On 11/1/18 1:03 PM, JC Beyler wrote:
>>>>>         Hi Chris,
>>>>>
>>>>>         Thanks for going over my email :)
>>>>>
>>>>>         So I skipped over the tracing part for clarity and
>>>>>         striving to be brief. But if we can accept:
>>>>>                    debugeeClass =
>>>>>         jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
>>>>>
>>>>>         and use a mechanism such as
>>>>>                    debugeeClass =
>>>>>         jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
>>>>>
>>>>>         it is actually easy to print out things like:
>>>>>         >> JNI FindClass with the following parameters from
>>>>>         test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:
>>>>>         >> "nsk/jvmti/SetTag/settag001"
>>>>>         ...
>>>>>         << JNI FindClass from
>>>>>         test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69:
>>>>>
>>>>>         Before the actual call to the method, allowing to have the
>>>>>         full NSK_*_VERIFY system. The hardest question is do we
>>>>>         accept to go from:
>>>>>>           if (!NSK_JNI_VERIFY(jni, (debugeeClass =
>>>>>>         jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
>>>>>>         nsk_jvmti_setFailStatus();
>>>>>>               return;
>>>>>>           }
>>>>>
>>>>>         to:
>>>>>
>>>>>         #define TRACE_JNI_CALL __LINE__, __FILE__
>>>>>         ...
>>>>>
>>>>>          ExceptionCheckingJniEnvPtr jni_wrapper(jni);
>>>>>         ...
>>>>>                    debugeeClass =
>>>>>         jni->FindClass(DEBUGEE_CLASS_NAME, TRACE_JNI_CALL);
>>>>>
>>>>>         What I would do if this is accepted is postpone the
>>>>>         assignment extraction and move the code to migrating the
>>>>>         NSK*VERIFY macros to using the exception wrapper and then
>>>>>         moving the assignments will be easier and a nop from a
>>>>>         debug printout point of view.
>>>>>
>>>>>         Thanks,
>>>>>         Jc
>>>>>
>>>>>
>>>>>
>>>>>         On Thu, Nov 1, 2018 at 12:01 PM Chris Plummer
>>>>>         <chris.plummer at oracle.com
>>>>>         <mailto:chris.plummer at oracle.com>> wrote:
>>>>>
>>>>>             On 11/1/18 9:53 AM, JC Beyler wrote:
>>>>>>             Hi all,
>>>>>>
>>>>>>             Sorry this is a bit of a long reply to myself but
>>>>>>             I've tried to see what we have, what we could have,
>>>>>>             is it better. Here is my analysis of how to keep the
>>>>>>             information we currently provide when a test fails at
>>>>>>             a NSK*VERIFY point and how we could maintain the
>>>>>>             level of detail (and even expand) while still
>>>>>>             cleaning up the macros/assignments.
>>>>>>
>>>>>>             I've been looking at this and am going to go through
>>>>>>             examples to provide alternatives :). For this, I'll
>>>>>>             use the SetTag test and will be forcing a failure on
>>>>>>             the line:
>>>>>>
>>>>>>             if (!NSK_JNI_VERIFY(jni, (debugeeClass =
>>>>>>             jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL)) {
>>>>>>             nsk_jvmti_setFailStatus();
>>>>>>                 return;
>>>>>>             }
>>>>>>
>>>>>>             Without a change and with the verbose system set up
>>>>>>             for the test, the error message is:
>>>>>>
>>>>>>             The following fake exception stacktrace is for
>>>>>>             failuire analysis.
>>>>>>             nsk.share.Fake_Exception_for_RULE_Creation:
>>>>>>             (settag001.cpp:65) (debugeeClass =
>>>>>>             jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
>>>>>>             at nsk_lvcomplain(nsk_tools.cpp:172)
>>>>>>             # ERROR: settag001.cpp, 65: (debugeeClass =
>>>>>>             jni->FindClass(DEBUGEE_CLASS_NAME)) != NULL
>>>>>>             #  verified JNI assertion is FALSE
>>>>>>             # ERROR: agent_tools.cpp, 282: Debuggee status sync
>>>>>>             aborted because agent thread has finished
>>>>>             I think you are missing something here. This appears
>>>>>             to be the output from nsk_jni_lverify() when the
>>>>>             condition indicates failure. However, I don't see the
>>>>>             verbose tracing from nsk_ltrace(), which I think is
>>>>>             more relevant because that's were you are likely to
>>>>>             want to see the actual JNI or JVMTI call being made.
>>>>>
>>>>>             #define NSK_JNI_VERIFY(jni, action)  \
>>>>>              (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
>>>>>             \
>>>>>             nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
>>>>>
>>>>>>
>>>>>>             (Note the typo for failuire, I created JDK-8213246 to
>>>>>>             fix it).
>>>>>>
>>>>>>             With my proposed change to remove the assignment, the
>>>>>>             code becomes:
>>>>>>             debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME);
>>>>>>             if (!NSK_JNI_VERIFY(jni, debuggeeClass != NULL)) {
>>>>>>             nsk_jvmti_setFailStatus();
>>>>>>                   return;
>>>>>>               }
>>>>>>
>>>>>>             The following fake exception stacktrace is for
>>>>>>             failuire analysis.
>>>>>>             nsk.share.Fake_Exception_for_RULE_Creation:
>>>>>>             (settag001.cpp:66) debugeeClass != NULL
>>>>>>             at nsk_lvcomplain(nsk_tools.cpp:172)
>>>>>>             # ERROR: settag001.cpp, 66: debugeeClass != NULL
>>>>>>             #  verified JNI assertion is FALSE
>>>>>>             # ERROR: agent_tools.cpp, 282: Debuggee status sync
>>>>>>             aborted because agent thread has finished
>>>>>>             STDERR:
>>>>>>
>>>>>>             In this case, we do lose that the failure seems to
>>>>>>             have happened in FindClass, we need to look at the code.
>>>>>             I think losing the actual call info in the trace for
>>>>>             failures is fine. All we really need is the line
>>>>>             number info. I don't think anyone is going to be
>>>>>             trying to debug the root cause based on trace message,
>>>>>             which is no different than the source that can be
>>>>>             viewed given the line number info.
>>>>>
>>>>>             But back to my point above, if you enable tracing,
>>>>>             seeing which JVMTI and JNI calls are being made in the
>>>>>             trace output could be useful. You could scan all logs
>>>>>             and see what JVMTI and JNI calls were made, and how
>>>>>             often. Possibly useful for coverage analysis of the
>>>>>             tests. However, I don't consider this critical, and am
>>>>>             not opposed to losing it in order to make the code
>>>>>             more readable.
>>>>>
>>>>>             In any case, a solution would be to have a wrapper
>>>>>             script like NSK_JNI_VERIFY, but just for the purpose
>>>>>             of tracing, not error checking:
>>>>>
>>>>>                         debugeeClass =
>>>>>             NSK_JNI_WRAPPER(jni->FindClass(DEBUGEE_CLASS_NAME));
>>>>>                         if (!NSK_JNI_VERIFY(jni, debuggeeClass !=
>>>>>             NULL)) {
>>>>>             nsk_jvmti_setFailStatus();
>>>>>                             return;
>>>>>                         }
>>>>>
>>>>>             And NSK_JNI_WRAPPER would just to the tracing and
>>>>>             execute the passed in action.
>>>>>>
>>>>>>             However, the ExceptionJNIWrapper that I implemented a
>>>>>>             little while ago will provide this information:
>>>>>>             FATAL ERROR in native method: FindClass : Return is NULL
>>>>>             As I mentioned above, I don't consider the API that
>>>>>             failed to be all that useful in error output as long
>>>>>             as we have the line number info. At best maybe it
>>>>>             helps identify two separate failures as being the same
>>>>>             if the line numbers were to change.
>>>>>>
>>>>>>             which is not sufficient to really figure out where
>>>>>>             this happened and what were the arguments. But we
>>>>>>             could extend the ExceptionJNIWrapper to have each JNI
>>>>>>             call
>>>>>>             allow two optional arguments and do this:
>>>>>>
>>>>>>             ...
>>>>>>             // For tracing purposes.
>>>>>>             #define TRACE_JNI_CALL __LINE__, __FILE__
>>>>>>             ...
>>>>>>
>>>>>>              ExceptionCheckingJniEnvPtr jni_wrapper(jni);
>>>>>>             ...
>>>>>>              debugeeClass = jni->FindClass(DEBUGEE_CLASS_NAME,
>>>>>>             TRACE_JNI_CALL);
>>>>>>
>>>>>>             Where now the JNI call looks "almost" like non-test
>>>>>>             code and a real JNI call but with just that optional
>>>>>>             macro added. Then the ExceptionCheckingJniEnvPtr can
>>>>>>             be modified to print out:
>>>>>>
>>>>>>             FATAL ERROR in native method: JNI method FindClass :
>>>>>>             Return is NULL from
>>>>>>             test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>>>
>>>>>>             Which now contains all the same information except
>>>>>>             what the line looks like. I'm not sure that is
>>>>>>             useful, instead, if we wanted to go one step further,
>>>>>>             we could add prints to all parameters that are passed
>>>>>>             in the JNI methods via the wrapper, which would
>>>>>>             then look like something like:
>>>>>>
>>>>>>             JNI method FindClass was called from
>>>>>>             test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>>>             with these parameters:
>>>>>>             "nsk/jvmti/SetTag/settag001"
>>>>>>             FATAL ERROR in native method: JNI method FindClass :
>>>>>>             Return is NULL from
>>>>>>             test/hotspot/jtreg/vmTestbase/nsk/jvmti/SetTag/settag001/settag001.cpp:69
>>>>>             I'm not familiar with you ExceptionJNIWrapper changes.
>>>>>             The error output above looks fine, but as I mentioned
>>>>>             earlier, I'm ok with just the source line number info.
>>>>>
>>>>>             thanks,
>>>>>
>>>>>             Chris
>>>>>>
>>>>>>             Let me know what you think.
>>>>>>
>>>>>>             Thanks,
>>>>>>             Jc
>>>>>>
>>>>>>
>>>>>>             On Wed, Oct 31, 2018 at 1:54 PM JC Beyler
>>>>>>             <jcbeyler at google.com <mailto:jcbeyler at google.com>> wrote:
>>>>>>
>>>>>>                 @Claes: you are right, I did not do a grep such
>>>>>>                 as "if.* =$"; by adding the space instead of the
>>>>>>                 $, I missed a few :)
>>>>>>
>>>>>>                 I've been meaning to ask if we could deprecate
>>>>>>                 these anyway. Are these really being used? And if
>>>>>>                 so, are we saying everything here is useful:
>>>>>>                    - Line/File + JNI call?
>>>>>>
>>>>>>                 I ask because I'd like to deprecate the
>>>>>>                 NSK_VERIFY macros but the LINE/FILE is a bit
>>>>>>                 annoying to deprecate.
>>>>>>
>>>>>>                 I'd rather be able to remove them entirely but we
>>>>>>                 could do an alternative and migrate the
>>>>>>                 NSK_VERIFY to:
>>>>>>
>>>>>>                    testedFieldID
>>>>>>                 = jni->GetStaticFieldID(testedClass, FIELD_NAME,
>>>>>>                 FIELD_SIGNATURE);
>>>>>>                     if (!testedFieldID) {
>>>>>>
>>>>>>                 where the print out of the jni method and
>>>>>>                 argument values can still be done in the JNI
>>>>>>                 wrapper I added (ExceptionCheckingJniEnv.cpp) so
>>>>>>                 we'd have the printout of the calls but not the
>>>>>>                 line and filename from where the call was done.
>>>>>>
>>>>>>                 If lines and filenames are really important, then
>>>>>>                 we could do something like:
>>>>>>                     testedFieldID =
>>>>>>                 NSK_TRACE(jni->GetStaticFieldID(testedClass,
>>>>>>                 FIELD_NAME, FIELD_SIGNATURE));
>>>>>>                     if (!testedFieldID) {
>>>>>>
>>>>>>                 Which would add a line for which line/file is
>>>>>>                 doing the JNI call. The verify part can go into
>>>>>>                 the wrapper I was talking about
>>>>>>                 (ExceptionCheckingJniEnv.cpp).
>>>>>>
>>>>>>                 Thanks,
>>>>>>                 Jc
>>>>>>
>>>>>>
>>>>>>                 On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer
>>>>>>                 <chris.plummer at oracle.com
>>>>>>                 <mailto:chris.plummer at oracle.com>> wrote:
>>>>>>
>>>>>>                     On 10/31/18 11:30 AM,
>>>>>>                     serguei.spitsyn at oracle.com
>>>>>>                     <mailto:serguei.spitsyn at oracle.com> wrote:
>>>>>>                     > It prints the FILE/LINE which is enough to
>>>>>>                     identify the error point.
>>>>>>                     Yes, but it also prints the JNI calls.
>>>>>>                     > But I agree that doing the assignments
>>>>>>                     within the NSK_JNI_VERIFY was
>>>>>>                     > intentional as it gives more context.
>>>>>>                     > From the other hand it make the code ugly
>>>>>>                     and less readable.
>>>>>>                     > Not sure, what direction is better to go.
>>>>>>                     Agreed. Somewhat of a tossup now as to
>>>>>>                     whether or not this change should
>>>>>>                     be done. I'm fine either way.
>>>>>>
>>>>>>                     Chris
>>>>>>                     >
>>>>>>                     > Thanks,
>>>>>>                     > Serguei
>>>>>>                     >
>>>>>>                     >
>>>>>>                     > On 10/31/18 11:21, Chris Plummer wrote:
>>>>>>                     >> Hi Claes,
>>>>>>                     >>
>>>>>>                     >> It's good that you brought this up.
>>>>>>                     NSK_JNI_VERIFY is always "on",
>>>>>>                     >> but moving the assignment outside of it
>>>>>>                     does slightly change the
>>>>>>                     >> behavior of the macro (although the code
>>>>>>                     still executes "correctly"):
>>>>>>                     >>
>>>>>>                     >> /**
>>>>>>                     >>  * Execute action with JNI call, check
>>>>>>                     result for true and
>>>>>>                     >>  * pending exception and complain error if
>>>>>>                     required.
>>>>>>                     >>  * Also trace action execution if tracing
>>>>>>                     mode enabled.
>>>>>>                     >>  */
>>>>>>                     >> #define NSK_JNI_VERIFY(jni, action)  \
>>>>>>                     >>
>>>>>>                     (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action),
>>>>>>                     \
>>>>>>                     >>
>>>>>>                     nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
>>>>>>                     >>
>>>>>>                     >> So you will no longer see the JNI call in
>>>>>>                     the trace or error output.
>>>>>>                     >> You will just see the comparison to the
>>>>>>                     JNI result, which gives no
>>>>>>                     >> context as to the source of the result. So
>>>>>>                     I'm now starting to think
>>>>>>                     >> that doing the assignments within the
>>>>>>                     NSK_JNI_VERIFY was intentional
>>>>>>                     >> so we would see the JNI call in the trace
>>>>>>                     output.
>>>>>>                     >>
>>>>>>                     >> Chris
>>>>>>                     >>
>>>>>>                     >> On 10/31/18 3:16 AM, Claes Redestad wrote:
>>>>>>                     >>> Hi,
>>>>>>                     >>>
>>>>>>                     >>> here's a case that your script seems to miss:
>>>>>>                     >>>
>>>>>>                     >>>
>>>>>>                     http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
>>>>>>
>>>>>>                     >>>
>>>>>>                     >>>
>>>>>>                     >>>      if (!NSK_JNI_VERIFY(jni,
>>>>>>                     (testedFieldID =
>>>>>>                     >>> jni->GetStaticFieldID(testedClass,
>>>>>>                     FIELD_NAME,
>>>>>>                     >>> FIELD_SIGNATURE)) != NULL))
>>>>>>                     >>>
>>>>>>                     >>> I'd note that with some of your changes
>>>>>>                     here you're unconditionally
>>>>>>                     >>> executing logic outside of NSK_JNI_VERIFY
>>>>>>                     macros. Does care need be
>>>>>>                     >>> taken to wrap the code blocks in
>>>>>>                     equivalent macros/ifdefs? Or are
>>>>>>                     >>> the NSK_JNI_VERIFY macros always-on in
>>>>>>                     these tests (and essentially
>>>>>>                     >>> pointless)?
>>>>>>                     >>>
>>>>>>                     >>> /Claes
>>>>>>                     >>>
>>>>>>                     >>> On 2018-10-31 05:42, JC Beyler wrote:
>>>>>>                     >>>> Hi all,
>>>>>>                     >>>>
>>>>>>                     >>>> I worked on updating my script and
>>>>>>                     handling more assignments so I
>>>>>>                     >>>> redid a second pass on the same files to
>>>>>>                     get all the cases. Now, on
>>>>>>                     >>>> those files the regex "if.* = " no
>>>>>>                     longer shows any cases we would
>>>>>>                     >>>> want to fix.
>>>>>>                     >>>>
>>>>>>                     >>>> Could I get a review for this webrev:
>>>>>>                     >>>> Webrev:
>>>>>>                     http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
>>>>>>                     <http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/>
>>>>>>                     >>>> Bug:
>>>>>>                     https://bugs.openjdk.java.net/browse/JDK-8213003
>>>>>>                     >>>>
>>>>>>                     >>>> I tested this on my dev machine by
>>>>>>                     passing all the tests that were
>>>>>>                     >>>> modified.
>>>>>>                     >>>>
>>>>>>                     >>>> Thanks!
>>>>>>                     >>>> Jc
>>>>>>                     >>>
>>>>>>                     >>
>>>>>>                     >>
>>>>>>                     >
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>                 -- 
>>>>>>
>>>>>>                 Thanks,
>>>>>>                 Jc
>>>>>>
>>>>>>
>>>>>>
>>>>>>             -- 
>>>>>>
>>>>>>             Thanks,
>>>>>>             Jc
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>         -- 
>>>>>
>>>>>         Thanks,
>>>>>         Jc
>>>>
>>>>
>>>>
>>>>
>>>>     -- 
>>>>
>>>>     Thanks,
>>>>     Jc
>>>
>>>
>>>
>>>
>>> -- 
>>>
>>> Thanks,
>>> Jc
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181106/0237ccfa/attachment-0001.html>


More information about the serviceability-dev mailing list