From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14693 invoked by alias); 4 May 2017 19:03:24 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 14649 invoked by uid 89); 4 May 2017 19:03:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No 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,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=comfortable, acting 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, 04 May 2017 19:03:21 +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 73B074E8AB; Thu, 4 May 2017 19:03:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 73B074E8AB Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=law@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 73B074E8AB Received: from localhost.localdomain (ovpn-116-241.phx2.redhat.com [10.3.116.241]) by smtp.corp.redhat.com (Postfix) with ESMTP id 18C5560F83; Thu, 4 May 2017 19:03:21 +0000 (UTC) Subject: Re: PR80613 To: Prathamesh Kulkarni , gcc Patches , Richard Biener References: From: Jeff Law Message-ID: Date: Thu, 04 May 2017 19:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00343.txt.bz2 On 05/04/2017 10:00 AM, Prathamesh Kulkarni wrote: > Hi, > As mentioned in PR, the issue is that cddce1 marks the call to > __builtin_strdup as necessary: > marking necessary through .MEM_6 stmt p_7 = __builtin_strdup (&d); > > and since p_7 doesn't get added to worklist in propagate_necessity() > because it's used only within free(), it's treated as "dead" > and wrongly gets released. > The patch fixes that by adding strdup/strndup in corresponding condition > in eliminate_unnecessary_stmts(). > > Another issue, was that my previous patch failed to remove multiple > calls to strdup: > char *f(char **tt) > { > char *t = *tt; > char *p; > > p = __builtin_strdup (t); > p = __builtin_strdup (t); > return p; > } > > That's fixed in patch by adding strdup/strndup to another > corresponding condition in propagate_necessity() so that only one > instance of strdup would be kept. > > Bootstrapped+tested on x86_64-unknown-linux-gnu. > Cross-testing on arm*-*-* and aarch64*-*-* in progress. > OK to commit if testing passes ? > > Thanks > Prathamesh > > > pr80613-1.txt > > > 2017-05-04 Prathamesh Kulkarni > > PR tree-optimization/80613 > * tree-ssa-dce.c (propagate_necessity): Add cases for BUILT_IN_STRDUP > and BUILT_IN_STRNDUP. > * (eliminate_unnecessary_stmts): Likewise. > > testsuite/ > * gcc.dg/tree-ssa/pr80613-1.c: New test-case. > * gcc.dg/tree-ssa/pr80613-2.c: New test-case. So I'm comfortable with the change to eliminate_unnecessary_stmts as well as the associated testcase pr80613-1.c. GIven that addresses the core of the bug, I'd go ahead and install that part immediately. I'm still trying to understand the code in propagate_necessity. > > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c > new file mode 100644 > index 00000000000..56176427922 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-1.c > @@ -0,0 +1,13 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +char *a(int); > +int b; > + > +void c() { > + for (;;) { > + char d = *a(b); > + char *e = __builtin_strdup (&d); > + __builtin_free(e); > + } > +} > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c > new file mode 100644 > index 00000000000..c58cc08d6c5 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr80613-2.c > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-cddce1" } */ > + > +/* There should only be one instance of __builtin_strdup after cddce1. */ > + > +char *f(char **tt) > +{ > + char *t = *tt; > + char *p; > + > + p = __builtin_strdup (t); > + p = __builtin_strdup (t); > + return p; > +} > + > +/* { dg-final { scan-tree-dump-times "__builtin_strdup" 1 "cddce1" } } */ > diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c > index e17659df91f..7c05f981307 100644 > --- a/gcc/tree-ssa-dce.c > +++ b/gcc/tree-ssa-dce.c > @@ -852,7 +852,9 @@ propagate_necessity (bool aggressive) > == BUILT_IN_ALLOCA_WITH_ALIGN) > || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE > || DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE > - || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED)) > + || DECL_FUNCTION_CODE (callee) == BUILT_IN_ASSUME_ALIGNED > + || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRDUP > + || DECL_FUNCTION_CODE (callee) == BUILT_IN_STRNDUP)) > continue; What I'm struggling with is that str[n]dup read from the memory pointed to by their incoming argument, so ISTM they are not "merely acting are barriers or that only store to memory" and thus shouldn't be treated like memset, malloc & friends. Otherwise couldn't we end up incorrectly removing a store to memory that is only read by a live strdup? So while I agree you ought to be able to remove the first call to strdup in the second testcase, I'm not sure the approach you've used works correctly. jeff