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

[Daniel D. Daugherty]

On 4/28/2011 8:06 AM, Karen Kinnear wrote:
> Sorry, I didn't know you were going to redo this. I too have memories
> of os::free of null having issues. And I also have memories of code 
> checking
> tools complaining about calls to os::free of null.
>
> So, if you could either go back to the way it was, or only call
> os::free if it is not null I would be much more comfortable.

I did a quick survey of calls to os::free() in src/share/vm/*/*
and found that there are many places that check for NULL before
calling os::free(). Of course, there are also places that don't.

The general style in JVM/TI code is to check for NULL before
calling os::free() unless you're sure that the variable is
non-NULL. Being the paranoid person that I am, I'm going to
tweak this "simple" fix one more time...

I forgot to mention that the previous version and this version
allow me to drop the changes to jvmtiImpl.hpp.

This version still avoids assigning a static string to the
"name" field so Dmitry should be happy about that. However,
I'm adding a check for non-NULL before calling os::free()
which both Dmitry and David H insist is not necessary. In
this case, I'm going to fall back on the argument that
checking for NULL before call os::free() is consistent with
the style of paranoia used in JVM/TI.

Yes, I will make a new webrev available just to "dot the i's
and cross the t's".

Dan


--- a/src/share/vm/prims/jvmtiImpl.cpp  Sat Apr 23 00:33:38 2011 -0400
+++ b/src/share/vm/prims/jvmtiImpl.cpp  Thu Apr 28 08:23:56 2011 -0600
@@ -38,6 +38,7 @@
 #include "runtime/handles.inline.hpp"
 #include "runtime/interfaceSupport.hpp"
 #include "runtime/javaCalls.hpp"
+#include "runtime/os.hpp"
 #include "runtime/serviceThread.hpp"
 #include "runtime/signature.hpp"
 #include "runtime/vframe.hpp"
@@ -939,10 +940,15 @@
   nmethodLocker::lock_nmethod(nm, true /* zombie_ok */);
   return event;
 }
+
 JvmtiDeferredEvent JvmtiDeferredEvent::dynamic_code_generated_event(
       const char* name, const void* code_begin, const void* code_end) {
   JvmtiDeferredEvent event = 
JvmtiDeferredEvent(TYPE_DYNAMIC_CODE_GENERATED);
-  event._event_data.dynamic_code_generated.name = name;
+  // Need to make a copy of the name since we don't know how long
+  // the event poster will keep it around after we enqueue the
+  // deferred event and return. strdup() failure is handled in
+  // the post() routine below.
+  event._event_data.dynamic_code_generated.name = os::strdup(name);
   event._event_data.dynamic_code_generated.code_begin = code_begin;
   event._event_data.dynamic_code_generated.code_end = code_end;
   return event;
@@ -968,12 +974,19 @@
       nmethodLocker::unlock_nmethod(nm);
       break;
     }
-    case TYPE_DYNAMIC_CODE_GENERATED:
+    case TYPE_DYNAMIC_CODE_GENERATED: {
       JvmtiExport::post_dynamic_code_generated_internal(
-        _event_data.dynamic_code_generated.name,
+        // if strdup failed give the event a default name
+        (_event_data.dynamic_code_generated.name == NULL)
+          ? "unknown_code" : _event_data.dynamic_code_generated.name,
         _event_data.dynamic_code_generated.code_begin,
         _event_data.dynamic_code_generated.code_end);
+      if (_event_data.dynamic_code_generated.name != NULL) {
+        // release our copy
+        os::free((void *)_event_data.dynamic_code_generated.name);
+      }
       break;
+    }
     default:
       ShouldNotReachHere();
   }