RFR(XXS): 8203346: JFR: Inconsistent signature of jfr_add_string_constant

[Markus Gronlund]

Here is an update (with some fixes to a few small things in the vicinity as well):

# HG changeset patch
# User mgronlun
# Date 1526557216 -7200
#      Thu May 17 13:40:16 2018 +0200
# Node ID aa550bc8a5ddd1f0caa6d7e639c04b66b5cb8a2d
# Parent  8e4fcfb4cfe48e6b481c6aa1751d916014a826af
8203346: JFR: Inconsistent signature of jfr_add_string_constant
Reviewed-by:

diff --git a/src/hotspot/share/jfr/jni/jfrJniMethod.cpp b/src/hotspot/share/jfr/jni/jfrJniMethod.cpp
--- a/src/hotspot/share/jfr/jni/jfrJniMethod.cpp
+++ b/src/hotspot/share/jfr/jni/jfrJniMethod.cpp
@@ -298,11 +298,11 @@
 JVM_END

 JVM_ENTRY_NO_ENV(jboolean, jfr_add_string_constant(JNIEnv* env, jclass jvm, jboolean epoch, jlong id, jstring string))
-  return JfrStringPool::add(epoch == JNI_TRUE, id, string, thread);
+  return JfrStringPool::add(epoch == JNI_TRUE, id, string, thread) ? JNI_TRUE : JNI_FALSE;
 JVM_END

 JVM_ENTRY_NO_ENV(void, jfr_set_force_instrumentation(JNIEnv* env, jobject jvm, jboolean force_instrumentation))
-  JfrEventClassTransformer::set_force_instrumentation(force_instrumentation == JNI_TRUE ? true : false);
+  JfrEventClassTransformer::set_force_instrumentation(force_instrumentation == JNI_TRUE);
 JVM_END

 JVM_ENTRY_NO_ENV(void, jfr_emit_old_object_samples(JNIEnv* env, jobject jvm, jlong cutoff_ticks, jboolean emit_all))
diff --git a/src/hotspot/share/jfr/jni/jfrJniMethod.hpp b/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
--- a/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
+++ b/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
@@ -117,7 +117,7 @@

 jlong JNICALL jfr_get_epoch_address(JNIEnv* env, jobject jvm);

-jlong JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, jlong gen, jlong id, jstring string);
+jboolean JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, jboolean epoch, jlong id, jstring string);

 void JNICALL jfr_uncaught_exception(JNIEnv* env, jobject jvm, jobject thread, jthrowable throwable);

Thanks
Markus



-----Original Message-----
From: Markus Gronlund 
Sent: den 17 maj 2018 13:14
To: Aleksey Shipilev <shade at redhat.com>; hotspot-dev at openjdk.java.net; Doerr, Martin <martin.doerr at sap.com>
Subject: RE: RFR(XXS): 8203346: JFR: Inconsistent signature of jfr_add_string_constant

" Ugh. I still wonder why the return type is "jlong" in native parts. Should be jboolean too?"

Yes, same thing there, looks like the .hpp declaration did not get properly updated when moving from a jlong generational counter to a boolean epoch.

Will fix it as well.

Thanks
Markus


-----Original Message-----
From: Aleksey Shipilev [mailto:shade at redhat.com]
Sent: den 17 maj 2018 13:06
To: Markus Gronlund <markus.gronlund at oracle.com>; hotspot-dev at openjdk.java.net; Doerr, Martin <martin.doerr at sap.com>
Subject: Re: RFR(XXS): 8203346: JFR: Inconsistent signature of jfr_add_string_constant

On 05/17/2018 12:56 PM, Aleksey Shipilev wrote:
> On 05/17/2018 12:52 PM, Aleksey Shipilev wrote:
>> On 05/17/2018 12:40 PM, Markus Gronlund wrote:
>>> Please see this tiny fix for the following bug:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8203346
>>> diff --git a/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
>>> b/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
>>> --- a/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
>>> +++ b/src/hotspot/share/jfr/jni/jfrJniMethod.hpp
>>>
>>> @@ -117,7 +117,7 @@
>>> jlong JNICALL jfr_get_epoch_address(JNIEnv* env, jobject jvm);
>>>
>>> -jlong JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, 
>>> jlong gen, jlong id, jstring string);
>>> +jlong JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, 
>>> +jboolean epoch, jlong id, jstring string);
>>>
>>> void JNICALL jfr_uncaught_exception(JNIEnv* env, jobject jvm, 
>>> jobject thread, jthrowable throwable);
>>
>> Looks good, but see:
>>
>> $ ack jfr_add_string_constant src/hotspot/
>>
>> src/hotspot/share/jfr/jni/jfrJniMethod.cpp
>> 300:JVM_ENTRY_NO_ENV(jboolean, jfr_add_string_constant(JNIEnv* env, 
>> jclass jvm, jboolean epoch, jlong id, jstring string))
>>
>> src/hotspot/share/jfr/jni/jfrJniMethod.hpp
>> 120:jlong JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, 
>> jlong gen, jlong id, jstring string);
>>
>> src/hotspot/share/jfr/jni/jfrJniMethodRegistration.cpp
>> 76:      (char*)"addStringConstant", (char*)"(ZJLjava/lang/String;)Z", (void*)jfr_add_string_constant,
>>
>>
>> In jfrJniMethodRegistration.cpp, the signature should now be "(ZZLjava/lang/String;)Z"
> 
> No, wait, I confused myself. Java method indeed is (boolean, long, 
> String), so current signature is correct.
> 
>    public static native boolean addStringConstant(boolean epoch, long 
> id, String s);

Ugh. I still wonder why the return type is "jlong" in native parts. Should be jboolean too?

-Aleksey