From patchwork Mon Aug 24 11:23:03 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 509993 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 EE3DC1402A0 for ; Mon, 24 Aug 2015 21:23:18 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=PdLTEOjy; dkim-atps=neutral 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:subject:date:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=bs0UsMRT6lFlyPG/ loW2UaOB+qPTM3Dz7KsNEYX+b/Ncl02G4/y+Td6QhDgMddOeZa5ESbZXTNr5YD6N Tn3Okak0qirVbNXChyW9584tPdZ7xgNjyJtEwYzANQGHxG+a5nCivz4jiSgHoUVk bmvKw2rZ0NPgCwqmP60Nt15V2uc= 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:subject:date:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=QXl20WGfT2MfpdmKG5TlkO /OQSo=; b=PdLTEOjyMvquKD0t8zBbKGb2csmvEDHFLdOrg+9JRP2z/zG2R0KpUt sYGZHCKFDeWkKgKx4M4HwhEo/hTDsCGb8KXnqzpOOv36eVDVq/ATUOwoYpvWNJ2j 9pzq2YUIo92XPY4NcbuPLCfK8XtVKLCZNBIpLhN0sxgjOVal5wHcY= Received: (qmail 43592 invoked by alias); 24 Aug 2015 11:23:12 -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 43578 invoked by uid 89); 24 Aug 2015 11:23:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 24 Aug 2015 11:23:10 +0000 Received: from cam-owa2.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-35-X46lRCvvQyaZZfG2Ean5Sg-1; Mon, 24 Aug 2015 12:23:03 +0100 Received: from localhost ([10.1.2.79]) by cam-owa2.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Mon, 24 Aug 2015 12:23:03 +0100 From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Subject: Fix reference to freed data in df-scan.c Date: Mon, 24 Aug 2015 12:23:03 +0100 Message-ID: <87k2sl6ico.fsf@e105548-lin.cambridge.arm.com> User-Agent: Gnus/5.130012 (Ma Gnus v0.12) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 X-MC-Unique: X46lRCvvQyaZZfG2Ean5Sg-1 While experimenting with some allocation changes I noticed that df_insn_rescan frees a df_insn_info and implicitly requires alloc-pool to give back the same data on reallocation: bool the_same = df_insn_refs_verify (&collection_rec, bb, insn, false); /* If there's no change, return false. */ if (the_same) { df_free_collection_rec (&collection_rec); if (dump_file) fprintf (dump_file, "verify found no changes in insn with uid = %d.\n", uid); return false; } if (dump_file) fprintf (dump_file, "rescanning insn with uid = %d.\n", uid); /* There's change - we need to delete the existing info. Since the insn isn't moved, we can salvage its LUID. */ luid = DF_INSN_LUID (insn); df_insn_info_delete (uid); df_insn_create_insn_record (insn); DF_INSN_LUID (insn) = luid; We build up in collection_rec the list of references that INSN should have, then exit early if the df info already matches. Otherwise we tear down the old df_insn_info, allocate a new one, and copy the references in collection_rec to it. The problem is that the references in collection_rec refer to the old (freed) df_insn_info, so things break if alloc pool gives back a different address. The patch avoids the unnecessary free and reallocation. In principle it should also be a slight compile-time optimisation, but (as expected) the difference is far too small to be measurable. Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * df-scan.c (df_insn_info_init_fields): New function, split out from... (df_insn_create_insn_record): ...here. (df_insn_info_free_fields): New function, split out from... (df_insn_info_delete): ...here. (df_insn_rescan): Use the new functions instead of freeing and reallocating the df_insn_info. diff --git a/gcc/df-scan.c b/gcc/df-scan.c index 93c2eae..259c959 100644 --- a/gcc/df-scan.c +++ b/gcc/df-scan.c @@ -809,6 +809,14 @@ df_reg_chain_unlink (df_ref ref) df_free_ref (ref); } +/* Initialize INSN_INFO to describe INSN. */ + +static void +df_insn_info_init_fields (df_insn_info *insn_info, rtx_insn *insn) +{ + memset (insn_info, 0, sizeof (struct df_insn_info)); + insn_info->insn = insn; +} /* Create the insn record for INSN. If there was one there, zero it out. */ @@ -827,8 +835,7 @@ df_insn_create_insn_record (rtx_insn *insn) insn_rec = problem_data->insn_pool->allocate (); DF_INSN_INFO_SET (insn, insn_rec); } - memset (insn_rec, 0, sizeof (struct df_insn_info)); - insn_rec->insn = insn; + df_insn_info_init_fields (insn_rec, insn); return insn_rec; } @@ -876,6 +883,29 @@ df_mw_hardreg_chain_delete (struct df_mw_hardreg *hardregs) } } +/* Remove the contents of INSN_INFO (but don't free INSN_INFO itself). */ + +static void +df_insn_info_free_fields (df_insn_info *insn_info) +{ + /* In general, notes do not have the insn_info fields + initialized. However, combine deletes insns by changing them + to notes. How clever. So we cannot just check if it is a + valid insn before short circuiting this code, we need to see + if we actually initialized it. */ + df_mw_hardreg_chain_delete (insn_info->mw_hardregs); + + if (df_chain) + { + df_ref_chain_delete_du_chain (insn_info->defs); + df_ref_chain_delete_du_chain (insn_info->uses); + df_ref_chain_delete_du_chain (insn_info->eq_uses); + } + + df_ref_chain_delete (insn_info->defs); + df_ref_chain_delete (insn_info->uses); + df_ref_chain_delete (insn_info->eq_uses); +} /* Delete all of the refs information from the insn with UID. Internal helper for df_insn_delete, df_insn_rescan, and other @@ -895,24 +925,7 @@ df_insn_info_delete (unsigned int uid) struct df_scan_problem_data *problem_data = (struct df_scan_problem_data *) df_scan->problem_data; - /* In general, notes do not have the insn_info fields - initialized. However, combine deletes insns by changing them - to notes. How clever. So we cannot just check if it is a - valid insn before short circuiting this code, we need to see - if we actually initialized it. */ - df_mw_hardreg_chain_delete (insn_info->mw_hardregs); - - if (df_chain) - { - df_ref_chain_delete_du_chain (insn_info->defs); - df_ref_chain_delete_du_chain (insn_info->uses); - df_ref_chain_delete_du_chain (insn_info->eq_uses); - } - - df_ref_chain_delete (insn_info->defs); - df_ref_chain_delete (insn_info->uses); - df_ref_chain_delete (insn_info->eq_uses); - + df_insn_info_free_fields (insn_info); problem_data->insn_pool->remove (insn_info); DF_INSN_UID_SET (uid, NULL); } @@ -1075,8 +1088,8 @@ df_insn_rescan (rtx_insn *insn) /* There's change - we need to delete the existing info. Since the insn isn't moved, we can salvage its LUID. */ luid = DF_INSN_LUID (insn); - df_insn_info_delete (uid); - df_insn_create_insn_record (insn); + df_insn_info_free_fields (insn_info); + df_insn_info_init_fields (insn_info, insn); DF_INSN_LUID (insn) = luid; } else