Hi Matthias,
It looks good in general but I have some minor comments below.
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c.frames.html
279 build_id_to_debug_filename (size_t size, unsigned char *data)
280 {
. . .
283 filename = malloc(strlen (debug_file_directory) + (sizeof
"/.build-id/" - 1) + 1
284 + 2 * size + (sizeof ".debug" - 1) + 1);
285 if (filename == NULL) {
286 return NULL;
287 }
. . .
312 char *filename
313 = (build_id_to_debug_filename (note->n_descsz, bytes));
314 if (filename == NULL) {
315 return NULL;
316 }
There is no need to check filename for NULL at the line 314 as the function
build_id_to_debug_filename with new check at the line 285 never returns
NULL.
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m.frames.html
354 array = (*env)->NewByteArray(env, numBytes);
. . .
376 if (pages == NULL) {
377 return NULL;
378 }
379 mapped = calloc(pageCount, sizeof(int));
380 if (mapped == NULL) {
381 free(pages);
382 return NULL;
383 }
Just a question:
We do not release the array allocated at line 354 because this local
reference
will be auto-released when returning to Java. Is this correct?
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/src/jdk.hotspot.agent/macosx/native/libsaproc/symtab.c.frames.html
69 if (is_debug()) {
70 DBT rkey, rvalue; 71 char* tmp = (char
*)malloc(strlen(symtab->symbols[i].name) + 1);
72 if (tmp != NULL) {
73 strcpy(tmp, symtab->symbols[i].name);
74 rkey.data = tmp;
75 rkey.size = strlen(tmp) + 1;
76 (*symtab->hash_table->get)(symtab->hash_table, &rkey, &rvalue, 0);
77 // we may get a copy back so compare contents
78 symtab_symbol *res = (symtab_symbol *)rvalue.data;
79 if (strcmp(res->name, symtab->symbols[i].name) ||
80 res->offset != symtab->symbols[i].offset ||
81 res->size != symtab->symbols[i].size) {
82 print_debug("error to get hash_table value!\n");
83 }
84 free(tmp);
85 }
If malloc returns NULL then this debugging part will be we silently skipped.
In other such cases there is an attempt to print a debug message.
For instance:
140 symtab = (symtab_t *)malloc(sizeof(symtab_t));
141 if (symtab == NULL) {
142 print_debug("out of memory: allocating symtab\n");
143 return NULL;
144 }
I understand that print_debug can fail with out of memory as well.
But it depends on its implementation.
Thanks,
Serguei
On 9/4/19 00:28, Baesken, Matthias wrote:
Hello Yasumasa and Chris, thanks for your input .
Here is a new webrev , without the unneeded memset-calls after calloc .
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.1/
Hope everyone is happy with this now 😉 !
Best regards, Matthias
Hi Matthias,
src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:
```
405 // guarantee(symtab == NULL, "multiple symtab");
406 symtab = (struct symtab*)calloc(1, sizeof(struct symtab));
407 if (symtab == NULL) {
408 goto quit;
409 }
410 memset(symtab, 0, sizeof(struct symtab));
```
Why do you call memset() to clear symtab in L410?
symtab is allocated via calloc() in L406, so symtab would already cleared.
Thanks,
Yasumasa (ysuenaga)
On 2019/09/03 18:14, David Holmes wrote:
Hi Matthias,
Re-directing to serviceability-dev.
David
On 3/09/2019 5:42 pm, Baesken, Matthias wrote:
Hello, please review the following small fix .
In jdk.hotspot.agent native code (linux / macosx) we miss to check the
result of malloc/calloc a few times .
This should be adjusted.
Additionally I added initialization to the symtab array in symtab.c (by
calling memset to make sure we have a defined state ) .
One question (was not really sure about this one so I did not change it so
far) :
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.0/src/jdk.hotspot.
agent/macosx/native/libsaproc/symtab.c.frames.html
359 void destroy_symtab(symtab_t* symtab) {
360 if (!symtab) return;
361 free(symtab->strs);
362 free(symtab->symbols);
363 free(symtab);
364 }
Here we miss to close symtab->hash_table (opened by dbopen) , is it
needed (haven't used dbopen much - maybe someone can comment on
this)?
bug/webrev :
https://bugs.openjdk.java.net/browse/JDK-8230466
http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.0/
Thanks and best regards, Matthias
