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

[Daniel D. Daugherty]

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.


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