On 02/21/2020 00:09, Baesken, Matthias wrote:
Also I see GetStringUTFChars(str, JNI_FALSE). This look bad as well -
2nd arg is a pointer, so it should be NULL or nullptr.
Hi looks like there is another one here, do you think these JNI_FALSE params
would really cause trouble ? Not even the compiler warns here ...
src/java.desktop/unix/native/libawt_xawt/xawt/XlibWrapper.c:824: cname =
(char *) (*env)->GetStringUTFChars(env, jstr, JNI_FALSE);
This doesn't cause troubles just because both NULL & JNI_FALSE are
defines (and are 0). but specify JNI_FALSE as pointer value is confusing.
--alex
Best regards, Matthias
Hi Matthias,
Looks good in general, but I think it makes sense to fix #2 cases (at
least I see them in LinuxDebuggerLocal). If GetStringUTFChars fails, the
code will crash.
Also I see GetStringUTFChars(str, JNI_FALSE). This look bad as well -
2nd arg is a pointer, so it should be NULL or nullptr.
As for #1 and #3 - AFAIU they are both right ways.
If GetStringUTFChars fails, it throws OOM and return NULL.
And one more thing to consider.
LinuxDebuggerLocal_attach0 function looks terrible - 7
ReleaseStringUTFChars calls for 2 GetStringUTFChars.
Maybe it make sense to introduce simple wrapper like AutoJavaString in
src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp
It would make the code simpler and less error prone.
--alex
On 02/20/2020 00:53, Baesken, Matthias wrote:
Hi Alex / Christoph, thanks for the reviews.
New webrev :
http://cr.openjdk.java.net/~mbaesken/webrevs/8239462.1/
- includes LinuxDebuggerLocal.cpp
- adds a blank Christoph wanted to have
A question (hopefully not a stupid one 😉 ):
At most places in the coding, GetStringUTFChars success is
1. handled by checking NULL , like this :
const char *s = (*env)->GetStringUTFChars(env, p, NULL);
if (s == NULL) {
// handle failure
}
2.At some places , success / failure is not handled at all .
3.Here (e.g. LinuxDebuggerLocal.cpp) success / failure check is done by
if (env->ExceptionOccurred()) { ... }
Which one is the best / right way to do it (most likely not 2.) ?
Best regards, Matthias
Looks like
src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.cpp
has similar issues. It would be nice to fix them as well.
--alex
On 02/19/2020 06:21, Baesken, Matthias wrote:
Hello, please review this small change .
We miss at a few places ReleaseStringUTFChars calls in the native
jdk.hotspot.agent coding.
This happens in case of early returns .
Bug/webrev :
https://bugs.openjdk.java.net/browse/JDK-8239462
http://cr.openjdk.java.net/~mbaesken/webrevs/8239462.0/
Thanks, Matthias
