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

[Daniel D. Daugherty]

On 4/27/2011 10:10 PM, David Holmes wrote:
> Hi Dan,
>
> 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.


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

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