RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Harsha Wardhana B
harsha.wardhana.b at oracle.com
Tue Sep 6 06:47:17 UTC 2016
Thanks for the review David.
On Tuesday 06 September 2016 11:35 AM, David Holmes wrote:
> Hi Harsha,
>
> On 6/09/2016 3:47 PM, Harsha Wardhana B wrote:
>> Hi David,
>>
>> Please find revised webrev.
>>
>> http://cr.openjdk.java.net/~hb/8161448/webrev.03/
>
> You lost the earlier fix to not throw the exception here:
>
> 202 if (obj == NULL) {
> 203 free(dcmd_info_array);
> 204 JNU_ThrowOutOfMemoryError(env, 0);
> 205 return NULL;
> 206 }
>
> but otherwise this all looks fine. No need to see an updated webrev
> with line 204 deleted.
>
> Thanks for your patience and perseverance on this one.
>
> David
> -----
>
>> -Harsha
>>
>> On Thursday 01 September 2016 03:31 PM, David Holmes wrote:
>>> On 1/09/2016 6:05 PM, Harsha Wardhana B wrote:
>>>> Hi David,
>>>>
>>>> On Thursday 01 September 2016 01:14 PM, David Holmes wrote:
>>>>> Hi Harsha,
>>>>>
>>>>> Sorry these style issues are proving to be so painful, normally there
>>>>> would be more direct guidance from an existing team member.
>>>>>
>>>>> On 30/08/2016 4:51 PM, Harsha Wardhana B wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please find below webrev addressing David's and Dmitry's comments.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~hb/8161448/webrev.02/
>>>>>
>>>>> The idiomatic way we write such macros is as follows:
>>>>>
>>>>> #define EXCEPTION_CHECK_AND_FREE(x) \
>>>>> do { \
>>>>> if ((*env)->ExceptionCheck(env)) { \
>>>>> free(x); \
>>>>> return NULL; \
>>>>> } \
>>>>> } while (false)
>>>> I am aware of this style but I wasn't aware what style should be used
>>>> (BTW last line should be 'while(0)'). I didn't want to do anything
>>>> fancy
>>>> and end up sending one more webrev and hence I followed the
>>>> simplest way
>>>> to write a multi-line macro.
>>>> Now if there isn't anything wrong, I think we should leave it as is
>>>> until coding guidelines are put in place. I am facing same problem in
>>>> another issue where I have sent multiple webrev fixing only nits.
>>>> It is
>>>> turning out to be very painful.
>>>
>>> I understand and it is unfortunate that there has not been more
>>> definitive guidance here. But it is better to adopt established style
>>> and get used to using it. If you form is left in then you need spaces
>>> before trailing \ - another style nit, sorry.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> 109 if (obj == NULL) {
>>>>> 110 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>> 111 }
>>>>>
>>>>> Checking both NULL and for exceptions is redundant as previously
>>>>> pointed out. Either leave this code as it was:
>>>> Yes. it may be redundant but it is harmless. Does this really need
>>>> to be
>>>> changed?
>>>
>>> Yes it breeds confusion to see multiple checks that are unnecessary
>>> (and we're trying to address this with a lot of the JNI using code in
>>> the VM at the moment). In this case it is even worse because without
>>> knowing the exception check must be true and so "return NULL" will
>>> always execute, this code has the appearance of being able to continue
>>> if obj == NULL!
>>>
>>> Sorry. And thanks.
>>>
>>> David
>>> -----
>>>
>>>>>
>>>>> 91 if (obj == NULL) {
>>>>> 92 free(dcmd_arg_info_array);
>>>>> 93 return NULL;
>>>>> 94 }
>>>>>
>>>>> or else just replace it with:
>>>>>
>>>>> 109 EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>>
>>>>> The same for the block here:
>>>>>
>>>>> 201 if (obj == NULL){
>>>>> 202 EXCEPTION_CHECK_AND_FREE(dcmd_info_array);
>>>>> 203 }
>>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> -Harsha
>>>>>>
>>>>>> On Tuesday 30 August 2016 09:46 AM, David Holmes wrote:
>>>>>>> On 30/08/2016 2:06 PM, Harsha Wardhana B wrote:
>>>>>>>> Error checking might distract the main flow of code but it
>>>>>>>> would be
>>>>>>>> far-fetched to call that it obfuscates the code. Ideally error
>>>>>>>> checking
>>>>>>>
>>>>>>> You claimed macros obfuscate so the same word seemed appropriate. I
>>>>>>> don't necessarily equate readability with obfuscation.
>>>>>>>
>>>>>>>> could have been made a function (inline if possible) instead of a
>>>>>>>> macro
>>>>>>>> but since it is context sensitive in this case (have to free
>>>>>>>> variables
>>>>>>>> depending on context) , doing so would obfuscate the code even
>>>>>>>> more.
>>>>>>>>
>>>>>>>> I tried to separate it into a function and the code looks more
>>>>>>>> uglier.
>>>>>>>
>>>>>>> I was envisaging something like:
>>>>>>>
>>>>>>> jstring jname, jdesc,jtype,jdefStr;
>>>>>>> jname =
>>>>>>> (*env)->NewStringUTF(env,dcmd_arg_info_array[i].name);
>>>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>>>> jdesc =
>>>>>>> (*env)->NewStringUTF(env,dcmd_arg_info_array[i].description);
>>>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>>>> jtype =
>>>>>>> (*env)->NewStringUTF(env,dcmd_arg_info_array[i].type);
>>>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>>>> jdefStr = (*env)->NewStringUTF(env,
>>>>>>> dcmd_arg_info_array[i].default_string);
>>>>>>> EXCEPTION_CHECK_AND_FREE(dcmd_arg_info_array);
>>>>>>>
>>>>>>> Of course if this were C++ code instead of C there would be better
>>>>>>> techniques for dealing with the free'ing.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>> -Harsha
>>>>>>>> On Tuesday 30 August 2016 09:26 AM, David Holmes wrote:
>>>>>>>>> On 30/08/2016 1:47 PM, Harsha Wardhana B wrote:
>>>>>>>>>> Hi Dmitry,
>>>>>>>>>>
>>>>>>>>>> I am not sure how using macro will help in this context. As far
>>>>>>>>>> as I
>>>>>>>>>> know, macros must be sparingly used as they are error-prone,
>>>>>>>>>> obfuscate
>>>>>>>>>> the code and hard to debug. Most of best coding practice for
>>>>>>>>>> c/c++
>>>>>>>>>> (inluding Scott Myers Effective C++ ) advocate using macros only
>>>>>>>>>> if it
>>>>>>>>>> is absolutely needed.
>>>>>>>>>
>>>>>>>>> Macros are used extensively throughout hotspot and the JDK native
>>>>>>>>> code. That isn't to say that all such macro uses are good (we
>>>>>>>>> have
>>>>>>>>> bad
>>>>>>>>> code too). However macros make sense where you want to avoid code
>>>>>>>>> duplication and improve readability - as is the case here. It is
>>>>>>>>> quite
>>>>>>>>> common to "hide" error checking logic behind a macro because
>>>>>>>>> it is
>>>>>>>>> the
>>>>>>>>> error-checking logic that obfuscates the code.
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>> -Harsha
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Monday 29 August 2016 02:00 PM, Dmitry Samersoff wrote:
>>>>>>>>>>> Harsha,
>>>>>>>>>>>
>>>>>>>>>>> I'm for macro.
>>>>>>>>>>>
>>>>>>>>>>> You can define a macro right before a place where you uses
>>>>>>>>>>> it and
>>>>>>>>>>> undef
>>>>>>>>>>> it when you don't need it anymore.
>>>>>>>>>>>
>>>>>>>>>>> -Dmitry
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2016-08-25 11:19, Harsha Wardhana B wrote:
>>>>>>>>>>>> Hello All,
>>>>>>>>>>>>
>>>>>>>>>>>> Please find below the revised webrev below.
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~hb/8161448/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> Regards
>>>>>>>>>>>>
>>>>>>>>>>>> Harsha
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Wednesday 24 August 2016 11:48 AM, Harsha Wardhana B wrote:
>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tuesday 23 August 2016 12:41 PM, David Holmes wrote:
>>>>>>>>>>>>>> Hi Harsha,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 22/08/2016 6:30 PM, Harsha Wardhana B wrote:
>>>>>>>>>>>>>>> Hello All,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Please review the below parfait fixes for
>>>>>>>>>>>>>>> DiagnosticCommandImpl.c
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8161448
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> webrev : http://cr.openjdk.java.net/~hb/8161448/webrev.00/
>>>>>>>>>>>>>> Looks functionally correct, but I wouldn't complain if you
>>>>>>>>>>>>>> wanted to
>>>>>>>>>>>>>> use a macro to reduce the duplication and verbosity. Even
>>>>>>>>>>>>>> the:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 109 if (obj == NULL) {
>>>>>>>>>>>>>> 110 free(dcmd_arg_info_array);
>>>>>>>>>>>>>> 111 return NULL;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> can be replaced by an exception-check as that is the only
>>>>>>>>>>>>>> time
>>>>>>>>>>>>>> JNU_NewObjectByName can return NULL.
>>>>>>>>>>>>> I am not sure if using macro here is good practice since it
>>>>>>>>>>>>> will be
>>>>>>>>>>>>> specific to the function and it will encapsulate the local
>>>>>>>>>>>>> variable
>>>>>>>>>>>>> within it. Also, it will reduce code readability. Hence I
>>>>>>>>>>>>> would
>>>>>>>>>>>>> like
>>>>>>>>>>>>> to leave it as is.
>>>>>>>>>>>>>> I also noticed an issue that was flagged in some other JNI
>>>>>>>>>>>>>> code
>>>>>>>>>>>>>> lately:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 213 if (obj == NULL) {
>>>>>>>>>>>>>> 214 free(dcmd_info_array);
>>>>>>>>>>>>>> 215 JNU_ThrowOutOfMemoryError(env, 0);
>>>>>>>>>>>>>> 216 return NULL;
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> If obj == NULL then there is already a pending exception
>>>>>>>>>>>>>> and we
>>>>>>>>>>>>>> should not be throwing OOME.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> Sure. This needs to be fixed.
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Harsha
>>>>>>>>>>>>>>>
>>>>>>>>>>>>> Harsha
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>> Regards
>>>> Harsha
>>
More information about the serviceability-dev
mailing list