From 15d4fe3c66b4bc1453f82c075634e325e3008162 Mon Sep 17 00:00:00 2001 From: Julien Desfossez Date: Tue, 16 Oct 2012 14:25:33 -0400 Subject: [PATCH] Fix: free all the metadata-related memory When opening a trace, the parser allocates various data structures to represent internally the metadata. This patch frees all this memory when a trace is closed. When babeltrace was originally developed, it was meant to be mainly a trace converter, so relying on the operating system to free memory was "acceptable", although a bad practice. Now that libbabeltrace allows reading the CTF format from an external application, it becomes clear that fixing those memory leaks is required. As of this commit, valgrind invoked in the following way on babeltrace does not report any memory leak: G_SLICE=always-malloc G_DEBUG=gc-friendly valgrind --leak-check=full Let's keep it that way :-) ! Signed-off-by: Julien Desfossez Signed-off-by: Mathieu Desnoyers --- formats/ctf/ctf.c | 29 +---- formats/ctf/metadata/ctf-ast.h | 1 + formats/ctf/metadata/ctf-parser.y | 1 + .../metadata/ctf-visitor-generate-io-struct.c | 123 +++++++++++++++++- types/sequence.c | 2 +- types/struct.c | 1 + types/types.c | 5 +- types/variant.c | 4 +- 8 files changed, 136 insertions(+), 30 deletions(-) diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c index f1fdba17..e59e3573 100644 --- a/formats/ctf/ctf.c +++ b/formats/ctf/ctf.c @@ -1267,6 +1267,7 @@ int create_stream_packet_index(struct ctf_trace *td, packet_index.timestamp_begin = 0; packet_index.timestamp_end = 0; packet_index.events_discarded = 0; + packet_index.events_discarded_len = 0; /* read and check header, set stream id (and check) */ if (file_stream->parent.trace_packet_header) { @@ -1903,35 +1904,13 @@ void ctf_close_trace(struct trace_descriptor *tdp) continue; for (j = 0; j < stream->streams->len; j++) { struct ctf_file_stream *file_stream; - file_stream = container_of(g_ptr_array_index(stream->streams, j), struct ctf_file_stream, parent); + file_stream = container_of(g_ptr_array_index(stream->streams, j), + struct ctf_file_stream, parent); ctf_close_file_stream(file_stream); } - - } - g_ptr_array_free(td->streams, TRUE); - } - - if (td->event_declarations) { - for (i = 0; i < td->event_declarations->len; i++) { - struct bt_ctf_event_decl *event; - - event = g_ptr_array_index(td->event_declarations, i); - if (event->context_decl) - g_ptr_array_free(event->context_decl, TRUE); - if (event->fields_decl) - g_ptr_array_free(event->fields_decl, TRUE); - if (event->packet_header_decl) - g_ptr_array_free(event->packet_header_decl, TRUE); - if (event->event_context_decl) - g_ptr_array_free(event->event_context_decl, TRUE); - if (event->event_header_decl) - g_ptr_array_free(event->event_header_decl, TRUE); - if (event->packet_context_decl) - g_ptr_array_free(event->packet_context_decl, TRUE); - g_free(event); } - g_ptr_array_free(td->event_declarations, TRUE); } + ctf_destroy_metadata(td); closedir(td->dir); g_free(td); } diff --git a/formats/ctf/metadata/ctf-ast.h b/formats/ctf/metadata/ctf-ast.h index f3107fd2..d5a0544a 100644 --- a/formats/ctf/metadata/ctf-ast.h +++ b/formats/ctf/metadata/ctf-ast.h @@ -307,5 +307,6 @@ int ctf_visitor_semantic_check(FILE *fd, int depth, struct ctf_node *node); int ctf_visitor_parent_links(FILE *fd, int depth, struct ctf_node *node); int ctf_visitor_construct_metadata(FILE *fd, int depth, struct ctf_node *node, struct ctf_trace *trace, int byte_order); +int ctf_destroy_metadata(struct ctf_trace *trace); #endif /* _CTF_AST_H */ diff --git a/formats/ctf/metadata/ctf-parser.y b/formats/ctf/metadata/ctf-parser.y index 7f91365d..c59f2e75 100644 --- a/formats/ctf/metadata/ctf-parser.y +++ b/formats/ctf/metadata/ctf-parser.y @@ -860,6 +860,7 @@ static void ctf_ast_free(struct ctf_ast *ast) bt_list_for_each_entry_safe(node, tmp, &ast->allocated_nodes, gc) free(node); + free(ast); } int ctf_scanner_append_ast(struct ctf_scanner *scanner) diff --git a/formats/ctf/metadata/ctf-visitor-generate-io-struct.c b/formats/ctf/metadata/ctf-visitor-generate-io-struct.c index de998611..0b93b723 100644 --- a/formats/ctf/metadata/ctf-visitor-generate-io-struct.c +++ b/formats/ctf/metadata/ctf-visitor-generate-io-struct.c @@ -508,6 +508,7 @@ struct declaration *ctf_type_declarator_visit(FILE *fd, int depth, fprintf(fd, "[error] %s: cannot create array declaration.\n", __func__); return NULL; } + declaration_unref(nested_declaration); declaration = &array_declaration->p; break; } @@ -520,9 +521,12 @@ struct declaration *ctf_type_declarator_visit(FILE *fd, int depth, sequence_declaration = sequence_declaration_new(length_name, nested_declaration, declaration_scope); if (!sequence_declaration) { fprintf(fd, "[error] %s: cannot create sequence declaration.\n", __func__); + g_free(length_name); return NULL; } + declaration_unref(nested_declaration); declaration = &sequence_declaration->p; + g_free(length_name); break; } default: @@ -571,6 +575,7 @@ int ctf_struct_type_declarators_visit(FILE *fd, int depth, struct_declaration_add_field(struct_declaration, g_quark_to_string(field_name), field_declaration); + declaration_unref(field_declaration); } return 0; } @@ -604,10 +609,10 @@ int ctf_variant_type_declarators_visit(FILE *fd, int depth, return -EINVAL; } - untagged_variant_declaration_add_field(untagged_variant_declaration, g_quark_to_string(field_name), field_declaration); + declaration_unref(field_declaration); } return 0; } @@ -647,6 +652,7 @@ int ctf_typedef_visit(FILE *fd, int depth, struct declaration_scope *scope, type_declaration->declaration_free(type_declaration); return ret; } + declaration_unref(type_declaration); } return 0; } @@ -711,6 +717,7 @@ int ctf_typealias_visit(FILE *fd, int depth, struct declaration_scope *scope, err = register_declaration(alias_q, type_declaration, scope); if (err) goto error; + declaration_unref(type_declaration); return 0; error: @@ -828,6 +835,7 @@ struct declaration *ctf_declaration_struct_visit(FILE *fd, struct_declaration = lookup_struct_declaration(g_quark_from_string(name), declaration_scope); + declaration_ref(&struct_declaration->p); return &struct_declaration->p; } else { uint64_t min_align_value = 0; @@ -894,6 +902,7 @@ struct declaration *ctf_declaration_variant_visit(FILE *fd, untagged_variant_declaration = lookup_variant_declaration(g_quark_from_string(name), declaration_scope); + declaration_ref(&untagged_variant_declaration->p); } else { /* For unnamed variant, create type */ /* For named variant (with body), create type and add to declaration scope */ @@ -1054,6 +1063,7 @@ struct declaration *ctf_declaration_enum_visit(FILE *fd, int depth, enum_declaration = lookup_enum_declaration(g_quark_from_string(name), declaration_scope); + declaration_ref(&enum_declaration->p); return &enum_declaration->p; } else { /* For unnamed enum, create type */ @@ -1107,6 +1117,7 @@ struct declaration *ctf_declaration_enum_visit(FILE *fd, int depth, enum_declaration, declaration_scope); assert(!ret); + declaration_unref(&enum_declaration->p); } return &enum_declaration->p; } @@ -1134,6 +1145,7 @@ struct declaration *ctf_declaration_type_specifier_visit(FILE *fd, int depth, id_q = g_quark_from_string(str_c); g_free(str_c); declaration = lookup_declaration(id_q, declaration_scope); + declaration_ref(declaration); return declaration; } @@ -2644,6 +2656,7 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->procname, right, TRACER_ENV_LEN); env->procname[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.procname = \"%s\"\n", env->procname); + g_free(right); } else if (!strcmp(left, "hostname")) { char *right; @@ -2659,6 +2672,7 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->hostname, right, TRACER_ENV_LEN); env->hostname[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.hostname = \"%s\"\n", env->hostname); + g_free(right); } else if (!strcmp(left, "domain")) { char *right; @@ -2674,6 +2688,7 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->domain, right, TRACER_ENV_LEN); env->domain[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.domain = \"%s\"\n", env->domain); + g_free(right); } else if (!strcmp(left, "sysname")) { char *right; @@ -2689,6 +2704,7 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->sysname, right, TRACER_ENV_LEN); env->sysname[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.sysname = \"%s\"\n", env->sysname); + g_free(right); } else if (!strcmp(left, "kernel_release")) { char *right; @@ -2704,6 +2720,7 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->release, right, TRACER_ENV_LEN); env->release[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.release = \"%s\"\n", env->release); + g_free(right); } else if (!strcmp(left, "kernel_version")) { char *right; @@ -2719,12 +2736,14 @@ int ctf_env_declaration_visit(FILE *fd, int depth, struct ctf_node *node, strncpy(env->version, right, TRACER_ENV_LEN); env->version[TRACER_ENV_LEN - 1] = '\0'; printf_verbose("env.version = \"%s\"\n", env->version); + g_free(right); } else { if (is_unary_string(&node->u.ctf_expression.right)) { char *right; right = concatenate_unary_strings(&node->u.ctf_expression.right); printf_verbose("env.%s = \"%s\"\n", left, right); + g_free(right); } else if (is_unary_unsigned(&node->u.ctf_expression.right)) { uint64_t v; int ret; @@ -2822,7 +2841,6 @@ int ctf_root_declaration_visit(FILE *fd, int depth, struct ctf_node *node, struc return 0; } -/* TODO: missing metadata "destroy" (memory leak) */ int ctf_visitor_construct_metadata(FILE *fd, int depth, struct ctf_node *node, struct ctf_trace *trace, int byte_order) { @@ -2938,3 +2956,104 @@ error: g_hash_table_destroy(trace->clocks); return ret; } + +int ctf_destroy_metadata(struct ctf_trace *trace) +{ + int i, j, k; + struct ctf_file_stream *metadata_stream; + + if (trace->streams) { + for (i = 0; i < trace->streams->len; i++) { + struct ctf_stream_declaration *stream; + + stream = g_ptr_array_index(trace->streams, i); + if (!stream) + continue; + for (j = 0; j < stream->streams->len; j++) { + struct ctf_stream_definition *stream_def; + + stream_def = g_ptr_array_index(stream->streams, j); + if (!stream_def) + continue; + for (k = 0; k < stream_def->events_by_id->len; k++) { + struct ctf_event_definition *event; + + event = g_ptr_array_index(stream_def->events_by_id, k); + if (!event) + continue; + if (&event->event_fields->p) + definition_unref(&event->event_fields->p); + if (&event->event_context->p) + definition_unref(&event->event_context->p); + g_free(event); + } + if (&stream_def->trace_packet_header->p) + definition_unref(&stream_def->trace_packet_header->p); + if (&stream_def->stream_event_header->p) + definition_unref(&stream_def->stream_event_header->p); + if (&stream_def->stream_packet_context->p) + definition_unref(&stream_def->stream_packet_context->p); + if (&stream_def->stream_event_context->p) + definition_unref(&stream_def->stream_event_context->p); + g_ptr_array_free(stream_def->events_by_id, TRUE); + g_free(stream_def); + } + if (stream->event_header_decl) + declaration_unref(&stream->event_header_decl->p); + if (stream->event_context_decl) + declaration_unref(&stream->event_context_decl->p); + if (stream->packet_context_decl) + declaration_unref(&stream->packet_context_decl->p); + g_ptr_array_free(stream->streams, TRUE); + g_ptr_array_free(stream->events_by_id, TRUE); + g_hash_table_destroy(stream->event_quark_to_id); + free_declaration_scope(stream->declaration_scope); + g_free(stream); + } + g_ptr_array_free(trace->streams, TRUE); + } + + if (trace->event_declarations) { + for (i = 0; i < trace->event_declarations->len; i++) { + struct bt_ctf_event_decl *event_decl; + struct ctf_event_declaration *event; + + event_decl = g_ptr_array_index(trace->event_declarations, i); + if (event_decl->context_decl) + g_ptr_array_free(event_decl->context_decl, TRUE); + if (event_decl->fields_decl) + g_ptr_array_free(event_decl->fields_decl, TRUE); + if (event_decl->packet_header_decl) + g_ptr_array_free(event_decl->packet_header_decl, TRUE); + if (event_decl->event_context_decl) + g_ptr_array_free(event_decl->event_context_decl, TRUE); + if (event_decl->event_header_decl) + g_ptr_array_free(event_decl->event_header_decl, TRUE); + if (event_decl->packet_context_decl) + g_ptr_array_free(event_decl->packet_context_decl, TRUE); + + event = &event_decl->parent; + if (event->fields_decl) + declaration_unref(&event->fields_decl->p); + if (event->context_decl) + declaration_unref(&event->context_decl->p); + free_declaration_scope(event->declaration_scope); + + g_free(event); + } + g_ptr_array_free(trace->event_declarations, TRUE); + } + if (trace->packet_header_decl) + declaration_unref(&trace->packet_header_decl->p); + + free_declaration_scope(trace->root_declaration_scope); + free_declaration_scope(trace->declaration_scope); + + g_hash_table_destroy(trace->callsites); + g_hash_table_destroy(trace->clocks); + + metadata_stream = container_of(trace->metadata, struct ctf_file_stream, parent); + g_free(metadata_stream); + + return 0; +} diff --git a/types/sequence.c b/types/sequence.c index bd49d2a6..81270fac 100644 --- a/types/sequence.c +++ b/types/sequence.c @@ -206,8 +206,8 @@ void _sequence_definition_free(struct definition *definition) field = g_ptr_array_index(sequence->elems, i); field->declaration->definition_free(field); } + (void) g_ptr_array_free(sequence->elems, TRUE); } - (void) g_ptr_array_free(sequence->elems, TRUE); definition_unref(len_definition); free_definition_scope(sequence->p.scope); declaration_unref(sequence->p.declaration); diff --git a/types/struct.c b/types/struct.c index f360407c..22689c1c 100644 --- a/types/struct.c +++ b/types/struct.c @@ -169,6 +169,7 @@ void _struct_definition_free(struct definition *definition) } free_definition_scope(_struct->p.scope); declaration_unref(_struct->p.declaration); + g_ptr_array_free(_struct->fields, TRUE); g_free(_struct); } diff --git a/types/types.c b/types/types.c index 450c93ba..f74e730f 100644 --- a/types/types.c +++ b/types/types.c @@ -32,10 +32,13 @@ GQuark prefix_quark(const char *prefix, GQuark quark) { GQuark nq; GString *str; + char *quark_str; str = g_string_new(prefix); g_string_append(str, g_quark_to_string(quark)); - nq = g_quark_from_string(g_string_free(str, FALSE)); + quark_str = g_string_free(str, FALSE); + nq = g_quark_from_string(quark_str); + g_free(quark_str); return nq; } diff --git a/types/variant.c b/types/variant.c index faf70dde..660fb581 100644 --- a/types/variant.c +++ b/types/variant.c @@ -90,8 +90,9 @@ void _variant_declaration_free(struct declaration *declaration) struct declaration_variant *variant_declaration = container_of(declaration, struct declaration_variant, p); - _untagged_variant_declaration_free(&variant_declaration->untagged_variant->p); + declaration_unref(&variant_declaration->untagged_variant->p); g_array_free(variant_declaration->tag_name, TRUE); + g_free(variant_declaration); } struct declaration_variant * @@ -244,6 +245,7 @@ void _variant_definition_free(struct definition *definition) definition_unref(variant->enum_tag); free_definition_scope(variant->p.scope); declaration_unref(variant->p.declaration); + g_ptr_array_free(variant->fields, TRUE); g_free(variant); } -- 2.34.1