Hi Peter,
On 2017-01-06 09:46, Peter Levart wrote:
Hi Claes,
On 01/06/2017 03:36 AM, Claes Redestad wrote:
New webrevs:
http://cr.openjdk.java.net/~redestad/8171855/hotspot.04/
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/
In Module.c, it seems you applied the "techinique" of conditional
allocation (lines 49...57), but forgot to remove the old code (58...62):
49 if (len >= buf_size) {
50 utf_str = malloc(len + 1);
51 if (utf_str == NULL) {
52 JNU_ThrowOutOfMemoryError(env, NULL);
53 return NULL;
54 }
55 } else {
56 utf_str = buf;
57 }
58 utf_str = malloc(len + 1);
59 if (utf_str == NULL) {
60 JNU_ThrowOutOfMemoryError(env, NULL);
61 return NULL;
62 }
Ouch, this must have slipped back in during some final, late night
edits. Thanks for spotting it!
Also, in line 84, you allocate pkgs array only when num_packages != 0:
84 if (num_packages != 0 && (pkgs = calloc(num_packages,
sizeof(char*))) == NULL) {
...but you free it unconditionally (does free(NULL) work as no-op?):
It appears the C specification has said for some time that free(NULL) is
and should be a no-op, but that there might be (old) runtimes that
could crash when you do it. As I don't know if any of the OpenJDK
platforms might be affected or not I've added some checking.
110 free(pkgs);
In multiple methods of Module.c, you repeatedly use the following idiom:
131 pkg_name = GetInternalPackageName(env, pkg, buf,
(jsize)sizeof(buf));
132 if (pkg_name == NULL) {
133 JNU_ThrowOutOfMemoryError(env, NULL);
134 } else {
...but there's no need to call JNU_ThrowOutOfMemoryError if
GetInternalPackageName returns NULL since GetInternalPackageName already
does it.
Yes, this allows for a nice cleanup of that code.
Updated jdk webrev in-place:
http://cr.openjdk.java.net/~redestad/8171855/jdk.04/
Thanks!
/Claes
Otherwise the jdk part looks good.
Regards, Peter
In addition to fixing comments I couldn't resist applying the technique
used in Class.c to get rid of some mallocs in places where we can use a
stack allocated char buf[128] instead, which seems to have a tiny, but
measurable effect.
Thanks!
/Claes
