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