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

[David Holmes]

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");

>>         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. ;-)

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
>>>>>
>>>>>