RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Harsha Wardhana B
harsha.wardhana.b at oracle.com
Tue Sep 6 06:59:00 UTC 2016
Thanks for the review, Dmitry.
Harsha
On Tuesday 06 September 2016 12:27 PM, Dmitry Samersoff wrote:
> Harsha,
>
> OK. The fix looks good for me.
>
> -Dmitry
>
> On 2016-09-06 09:49, Harsha Wardhana B wrote:
>> Dmitry,
>>
>> The scope of the issue was to fix parfait warnings though I have gone to
>> some extent to refactor the file.
>>
>> Agree that macro can be used, but we have to stop here and raise a new
>> issue to carry-on with the refactoring process.
>>
>> Thanks
>>
>> Harsha
>>
>> On Tuesday 06 September 2016 12:03 PM, Dmitry Samersoff wrote:
>>> Harsha,
>>>
>>> EXCEPTION_CHECK_AND_FREE macro could be used at ll.76 after FindClass
>>>
>>> -Dmitry
>>>
>>> On 2016-09-06 08:47, Harsha Wardhana B wrote:
>>>> Hi David,
>>>>
>>>> Please find revised webrev.
>>>>
>>>> http://cr.openjdk.java.net/~hb/8161448/webrev.03/
>>>>
>>>> -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