RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
David Holmes
david.holmes at oracle.com
Tue Sep 6 06:05:07 UTC 2016
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