RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Harsha Wardhana B
harsha.wardhana.b at oracle.com
Tue Sep 6 05:47:57 UTC 2016
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