Hello Serguei, thanks for the comments . > > 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. >
At line 286 of build_id_to_debug_filename we now return NULL (in case malloc cannot alloc memory). So we should check this also at line 314 , or do I miss something ? > > 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? > Good point, I think we better add DeleteLocalRef here in case of the new early returns . > > 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 > > That's a good idea . I added a message . See new webrev : http://cr.openjdk.java.net/~mbaesken/webrevs/8230466.2/ Thanks, Matthias
