code review for JVM/TI DynamicCodeGenerated event fix (7039447)

[David Holmes]

Daniel D. Daugherty said the following on 04/28/11 23:37:
> On 4/27/2011 11:07 PM, David Holmes wrote:
>> Daniel D. Daugherty said the following on 04/28/11 14:21:
>>> On 4/27/2011 10:10 PM, David Holmes wrote:
>>>> Daniel D. Daugherty said the following on 04/28/11 13:56:
>>>>> David, thanks for the review.
>>>>>
>>>>> Dmitry and I have been chatting on the side about various
>>>>> null name handling styles. Feel free to chime in...
>>>>
>>>> Ok :) I was thinking simply do:
>>>>
>>>>  event._event_data.dynamic_code_generated.name =
>>>>            os::strdup(name != NULL ? name : "unknown_code");
>>>
>>> The incoming "name" shouldn't be NULL here so we
>>> would never end up duping "unknown_code".
>>>
>>>>
>>>> and then all you need is an unconditional free:
>>>>
>>>> !     case TYPE_DYNAMIC_CODE_GENERATED: {
>>>
>>> You've lost the check for strdup() failing and
>>> returning NULL so we no longer pass "unknown_code"
>>> to the post_*() function. That'll fire the new
>>> assertion.
>>
>> Sorry my misunderstanding. So the above would become the somewhat less 
>> elegeant:
>>
>> char* tmp;
>> event._event_data.dynamic_code_generated.name =
>>    ((tmp = os::strdup(name)) != NULL ? tmp : "unknown_code");
> 
> The above would result in "unknown_code" being passed to
> the os::free() call below.

Uggghhh! I withdraw my misguided suggestion. I wanted to avoid the need 
to check if the name was one which needed to be freed, but given we use 
"unknown_code" only when strdup fails then it will never be a string 
that needs freeing.

Just stick with what you have - it got three thumbs up. ;-)

Cheers,
David

> 
>>
>>>>         JvmtiExport::post_dynamic_code_generated_internal(
>>>>           _event_data.dynamic_code_generated.name,
>>>>           _event_data.dynamic_code_generated.code_begin,
>>>>           _event_data.dynamic_code_generated.code_end);
>>>> +         os::free(_event_data.dynamic_code_generated.name);
>>>
>>> In cases where strdup() failed, this free() call would
>>> be passed NULL. Dmitry pointed out (on the side) that
>>> the C89 spec says that is legal, but I have a memory
>>> that some of the malloc checkers complain about that...
>>
>> It was never my intent to pass a NULL through, but free(NULL) has been 
>> defined as a no-op since K&R days if not earlier. Any malloc checker 
>> that complains about that is a pile of junk in my view. ;-)
> 
> You and Dmitry are on the same page about free(NULL) being OK.
> 
> I'm going to take another look at what I can do to clean this
> up a bit...
> 
> Dan
> 
> 
> 
>>
>> Cheers,
>> David
>>
>>> Dan
>>>
>>>
>>>>         break;
>>>> +     }
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 4/27/2011 5:11 PM, David Holmes wrote:
>>>>>> Hi Dan,
>>>>>>
>>>>>> I was going to make a comment about the handling of a null name, 
>>>>>> but you have two thumbs up already and it really is just a style 
>>>>>> thing so ...
>>>>>>
>>>>>> Looks good to me. :)
>>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>>
>>>>>> Daniel D. Daugherty said the following on 04/28/11 05:46:
>>>>>>> Greetings,
>>>>>>>
>>>>>>> I have a fix for a bug in JVM/TI DynamicCodeGenerated event posting:
>>>>>>>
>>>>>>>    7039447 2/1 java profiling is broken in build 139 (garbage in
>>>>>>>                function name)
>>>>>>>
>>>>>>> The bug has actually been tracked back to HSX-21-B04/JDK7-B134, but
>>>>>>> the failure mode can be intermittent. Here is the webrev URL:
>>>>>>>
>>>>>>>    http://cr.openjdk.java.net/~dcubed/7039447-webrev/0/
>>>>>>>
>>>>>>> Here is my proposed commit message:
>>>>>>>
>>>>>>> 7039447: 2/1 java profiling is broken in build 139 (garbage in 
>>>>>>> function name)
>>>>>>> Summary: The name in a deferred JVM/TI DynamicCodeGenerated event 
>>>>>>> needs to be explicitly saved.
>>>>>>> Reviewed-by:
>>>>>>>
>>>>>>>
>>>>>>> I'm targeting this fix at HSX-21-B12/JDK7-B142; I just missed the
>>>>>>> RT_Baseline cutoff for HSX-21-B11/JDK7-B141. Since we are getting
>>>>>>> down to the wire on JDK7, I would like at least three reviewers.
>>>>>>>
>>>>>>> I'm in the process of running the following test suites (the
>>>>>>> JPDA parts of the Serviceability stack):
>>>>>>>
>>>>>>>    nsk.jvmti, nsk.jvmti_unit,
>>>>>>>    nsk.jdwp,
>>>>>>>    nsk.jdi, SDK_JDI, SDK_JLI,
>>>>>>>    nsk.hprof, nsk.jdb,
>>>>>>>    nsk.sajdi,
>>>>>>>    vm.heapdump, SDK_MISC_ATTACH, SDK_MISC_JVMSTAT, SDK_MISC_TOOLS
>>>>>>>
>>>>>>> on the following configs:
>>>>>>>
>>>>>>>    {Solaris X86, WinXP} x {Client VM} x {product, fastdebug} x 
>>>>>>> {Xmixed, Xcomp}
>>>>>>>
>>>>>>> So far preliminary results show no regressions and no new failures.
>>>>>>>
>>>>>>> I've also provided JPRT test bits to the Analyzer team and they
>>>>>>> have confirmed that the issue is resolved.
>>>>>>>
>>>>>>> Thanks, in advance, for any reviews.
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>