From patchwork Thu Aug 2 20:56:17 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 952959 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-483062-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="Ts59h7Cc"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41hLsj6MC3z9s0R for ; Fri, 3 Aug 2018 06:11:20 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; q=dns; s=default; b=MirDAshvQGkA 7FPCHzviMU4xBuhTYjpqxIzoKsKKWwnDiAcX3IEXWQVpU+aMx1o7c8jZNp850aIv k6jpWBEkF2kbBQ8l70H8dnVR8l+36x0ambG1pahde/GY5K8V/elGmjm+hVcItsq3 j4twSQIkblT5UDMPTB1w0i8/Ihb6qnU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id; s=default; bh=cb40ElIqpeVRTVJ0if KUiXSJEno=; b=Ts59h7CclIyEURp+h89mHsGI74VuVn2Zigr5TDGxmvW6z0KIaD LQjjupKrZNnmJHbYuyn5lJH+UkOC7hdJsITm2CJvHdWN6WmRdIYFFAxV2sZpaZbe s72uhH4Rc85epAnhC5j4g7q85hg9IYtgonBhsyWUgsJ06+JoiOqG+GQLQ= Received: (qmail 119692 invoked by alias); 2 Aug 2018 20:11:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 119671 invoked by uid 89); 2 Aug 2018 20:11:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=owned, spans X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 02 Aug 2018 20:11:09 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 68F47811A7 for ; Thu, 2 Aug 2018 20:11:08 +0000 (UTC) Received: from c64.redhat.com (ovpn-112-32.phx2.redhat.com [10.3.112.32]) by smtp.corp.redhat.com (Postfix) with ESMTP id 68D836250E; Thu, 2 Aug 2018 20:11:07 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] Fix memory leak of pretty_printer prefixes Date: Thu, 2 Aug 2018 16:56:17 -0400 Message-Id: <1533243377-19866-1-git-send-email-dmalcolm@redhat.com> X-IsSubscribed: yes We were rather sloppy about handling the ownership of prefixes for pretty_printer, and this lead to a memory leak for any time a diagnostic_show_locus call emits multiple line spans. This showed up in "make selftest-valgrind" as: 3,976 bytes in 28 blocks are definitely lost in loss record 632 of 669 at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x1F08227: xmalloc (xmalloc.c:147) by 0x1F083E6: xvasprintf (xvasprintf.c:58) by 0x1E7EC7D: build_message_string(char const*, ...) (diagnostic.c:78) by 0x1E7F438: diagnostic_get_location_text(diagnostic_context*, expanded_location) (diagnostic.c:328) by 0x1E7FD54: default_diagnostic_start_span_fn(diagnostic_context*, expanded_location) (diagnostic.c:626) by 0x1EB3508: selftest::test_diagnostic_context::start_span_cb(diagnostic_context*, expanded_location) (selftest-diagnostic.c:57) by 0x1E89215: diagnostic_show_locus(diagnostic_context*, rich_location*, diagnostic_t) (diagnostic-show-locus.c:1992) by 0x1E8ECAD: selftest::test_fixit_insert_containing_newline_2(selftest::line_table_case const&) (diagnostic-show-locus.c:3044) by 0x1EB0606: selftest::for_each_line_table_case(void (*)(selftest::line_table_case const&)) (input.c:3525) by 0x1E8F3F5: selftest::diagnostic_show_locus_c_tests() (diagnostic-show-locus.c:3164) by 0x1E010BF: selftest::run_tests() (selftest-run-tests.c:88) 4,004 bytes in 28 blocks are definitely lost in loss record 633 of 669 at 0x4A0645D: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x1F08227: xmalloc (xmalloc.c:147) by 0x1F083E6: xvasprintf (xvasprintf.c:58) by 0x1E7EC7D: build_message_string(char const*, ...) (diagnostic.c:78) by 0x1E7F438: diagnostic_get_location_text(diagnostic_context*, expanded_location) (diagnostic.c:328) by 0x1E7FD54: default_diagnostic_start_span_fn(diagnostic_context*, expanded_location) (diagnostic.c:626) by 0x1EB3508: selftest::test_diagnostic_context::start_span_cb(diagnostic_context*, expanded_location) (selftest-diagnostic.c:57) by 0x1E89215: diagnostic_show_locus(diagnostic_context*, rich_location*, diagnostic_t) (diagnostic-show-locus.c:1992) by 0x1E8B373: selftest::test_diagnostic_show_locus_fixit_lines(selftest::line_table_case const&) (diagnostic-show-locus.c:2500) by 0x1EB0606: selftest::for_each_line_table_case(void (*)(selftest::line_table_case const&)) (input.c:3525) by 0x1E8F3B9: selftest::diagnostic_show_locus_c_tests() (diagnostic-show-locus.c:3159) by 0x1E010BF: selftest::run_tests() (selftest-run-tests.c:88) This patch fixes the leaks by ensuring that the pretty_printer "owns" the prefix if it's non-NULL, freeing it in the dtor and in pp_set_prefix. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; manually verified "make selftest-valgrind" output (one unrelated leak remains). Committed to trunk as r263275. gcc/cp/ChangeLog: * error.c (cxx_print_error_function): Duplicate "file" before passing it to pp_set_prefix. (cp_print_error_function): Use pp_take_prefix when saving the existing prefix. gcc/ChangeLog: * diagnostic-show-locus.c (diagnostic_show_locus): Use pp_take_prefix when saving the existing prefix. * diagnostic.c (diagnostic_append_note): Likewise. * langhooks.c (lhd_print_error_function): Likewise. * pretty-print.c (pp_set_prefix): Drop the "const" from "prefix" param's type. Free the existing prefix. (pp_take_prefix): New function. (pretty_printer::pretty_printer): Drop the prefix parameter. Rename the length parameter to match the comment. (pretty_printer::~pretty_printer): Free the prefix. * pretty-print.h (pretty_printer::pretty_printer): Drop the prefix parameter. (struct pretty_printer): Drop the "const" from "prefix" field's type and clarify memory management. (pp_set_prefix): Drop the "const" from the 2nd param. (pp_take_prefix): New decl. --- gcc/cp/error.c | 9 +++++++-- gcc/diagnostic-show-locus.c | 2 +- gcc/diagnostic.c | 3 +-- gcc/langhooks.c | 2 +- gcc/pretty-print.c | 31 +++++++++++++++++++++++-------- gcc/pretty-print.h | 14 ++++++++------ 6 files changed, 41 insertions(+), 20 deletions(-) diff --git a/gcc/cp/error.c b/gcc/cp/error.c index a12fbc5..c49f4d7 100644 --- a/gcc/cp/error.c +++ b/gcc/cp/error.c @@ -3287,8 +3287,13 @@ void cxx_print_error_function (diagnostic_context *context, const char *file, diagnostic_info *diagnostic) { + char *prefix; + if (file) + prefix = xstrdup (file); + else + prefix = NULL; lhd_print_error_function (context, file, diagnostic); - pp_set_prefix (context->printer, file); + pp_set_prefix (context->printer, prefix); maybe_print_instantiation_context (context); } @@ -3316,7 +3321,7 @@ cp_print_error_function (diagnostic_context *context, return; if (diagnostic_last_function_changed (context, diagnostic)) { - const char *old_prefix = context->printer->prefix; + char *old_prefix = pp_take_prefix (context->printer); const char *file = LOCATION_FILE (diagnostic_location (diagnostic)); tree abstract_origin = diagnostic_abstract_origin (diagnostic); char *new_prefix = (file && abstract_origin == NULL) diff --git a/gcc/diagnostic-show-locus.c b/gcc/diagnostic-show-locus.c index f188ee9..cf9e5c2 100644 --- a/gcc/diagnostic-show-locus.c +++ b/gcc/diagnostic-show-locus.c @@ -1978,7 +1978,7 @@ diagnostic_show_locus (diagnostic_context * context, context->last_location = loc; - const char *saved_prefix = pp_get_prefix (context->printer); + char *saved_prefix = pp_take_prefix (context->printer); pp_set_prefix (context->printer, NULL); layout layout (context, richloc, diagnostic_kind); diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c index e22c17b..c61e0c4 100644 --- a/gcc/diagnostic.c +++ b/gcc/diagnostic.c @@ -1063,7 +1063,6 @@ diagnostic_append_note (diagnostic_context *context, { diagnostic_info diagnostic; va_list ap; - const char *saved_prefix; rich_location richloc (line_table, location); va_start (ap, gmsgid); @@ -1073,7 +1072,7 @@ diagnostic_append_note (diagnostic_context *context, va_end (ap); return; } - saved_prefix = pp_get_prefix (context->printer); + char *saved_prefix = pp_take_prefix (context->printer); pp_set_prefix (context->printer, diagnostic_build_prefix (context, &diagnostic)); pp_format (context->printer, &diagnostic.message); diff --git a/gcc/langhooks.c b/gcc/langhooks.c index c7b5cf9..4e6179f 100644 --- a/gcc/langhooks.c +++ b/gcc/langhooks.c @@ -369,7 +369,7 @@ lhd_print_error_function (diagnostic_context *context, const char *file, { if (diagnostic_last_function_changed (context, diagnostic)) { - const char *old_prefix = context->printer->prefix; + char *old_prefix = pp_take_prefix (context->printer); tree abstract_origin = diagnostic_abstract_origin (diagnostic); char *new_prefix = (file && abstract_origin == NULL) ? file_name_as_prefix (context, file) : NULL; diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c index dc7791a..736af8f 100644 --- a/gcc/pretty-print.c +++ b/gcc/pretty-print.c @@ -1482,23 +1482,38 @@ pp_clear_output_area (pretty_printer *pp) pp_buffer (pp)->line_length = 0; } -/* Set PREFIX for PRETTY-PRINTER. */ +/* Set PREFIX for PRETTY-PRINTER, taking ownership of PREFIX, which + will eventually be free-ed. */ + void -pp_set_prefix (pretty_printer *pp, const char *prefix) +pp_set_prefix (pretty_printer *pp, char *prefix) { + free (pp->prefix); pp->prefix = prefix; pp_set_real_maximum_length (pp); pp->emitted_prefix = false; pp_indentation (pp) = 0; } +/* Take ownership of PP's prefix, setting it to NULL. + This allows clients to save, overide, and then restore an existing + prefix, without it being free-ed. */ + +char * +pp_take_prefix (pretty_printer *pp) +{ + char *result = pp->prefix; + pp->prefix = NULL; + return result; +} + /* Free PRETTY-PRINTER's prefix, a previously malloc()'d string. */ void pp_destroy_prefix (pretty_printer *pp) { if (pp->prefix != NULL) { - free (CONST_CAST (char *, pp->prefix)); + free (pp->prefix); pp->prefix = NULL; } } @@ -1535,10 +1550,9 @@ pp_emit_prefix (pretty_printer *pp) } } -/* Construct a PRETTY-PRINTER with PREFIX and of MAXIMUM_LENGTH - characters per line. */ +/* Construct a PRETTY-PRINTER of MAXIMUM_LENGTH characters per line. */ -pretty_printer::pretty_printer (const char *p, int l) +pretty_printer::pretty_printer (int maximum_length) : buffer (new (XCNEW (output_buffer)) output_buffer ()), prefix (), padding (pp_none), @@ -1552,10 +1566,10 @@ pretty_printer::pretty_printer (const char *p, int l) translate_identifiers (true), show_color () { - pp_line_cutoff (this) = l; + pp_line_cutoff (this) = maximum_length; /* By default, we emit prefixes once per message. */ pp_prefixing_rule (this) = DIAGNOSTICS_SHOW_PREFIX_ONCE; - pp_set_prefix (this, p); + pp_set_prefix (this, NULL); } pretty_printer::~pretty_printer () @@ -1564,6 +1578,7 @@ pretty_printer::~pretty_printer () delete m_format_postprocessor; buffer->~output_buffer (); XDELETE (buffer); + free (prefix); } /* Append a string delimited by START and END to the output area of diff --git a/gcc/pretty-print.h b/gcc/pretty-print.h index 56abe03..0d67e30 100644 --- a/gcc/pretty-print.h +++ b/gcc/pretty-print.h @@ -215,17 +215,18 @@ class format_postprocessor and add additional fields they need. */ struct pretty_printer { - // Default construct a pretty printer with specified prefix - // and a maximum line length cut off limit. - explicit pretty_printer (const char* = NULL, int = 0); + /* Default construct a pretty printer with specified + maximum line length cut off limit. */ + explicit pretty_printer (int = 0); virtual ~pretty_printer (); /* Where we print external representation of ENTITY. */ output_buffer *buffer; - /* The prefix for each new line. */ - const char *prefix; + /* The prefix for each new line. If non-NULL, this is "owned" by the + pretty_printer, and will eventually be free-ed. */ + char *prefix; /* Where to put whitespace around the entity being formatted. */ pp_padding padding; @@ -338,7 +339,8 @@ pp_get_prefix (const pretty_printer *pp) { return pp->prefix; } #define pp_buffer(PP) (PP)->buffer extern void pp_set_line_maximum_length (pretty_printer *, int); -extern void pp_set_prefix (pretty_printer *, const char *); +extern void pp_set_prefix (pretty_printer *, char *); +extern char *pp_take_prefix (pretty_printer *); extern void pp_destroy_prefix (pretty_printer *); extern int pp_remaining_character_count_for_line (pretty_printer *); extern void pp_clear_output_area (pretty_printer *);