RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Sep 6 06:57:28 UTC 2016
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
>>
>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the serviceability-dev
mailing list