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

[Karen Kinnear]

Thank you very much. It is safer to err on the side of paranoia.

thanks,
Karen

On 04/28/11 10:38, Daniel D. Daugherty wrote:
> 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();
>   }
>