RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
David Holmes
david.holmes at oracle.com
Thu Sep 1 10:01:29 UTC 2016
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