From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72534 invoked by alias); 12 Feb 2016 18:12:44 -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 72519 invoked by uid 89); 12 Feb 2016 18:12:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=HContent-Transfer-Encoding:8bit 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 12 Feb 2016 18:12:42 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 344D7A2C0A for ; Fri, 12 Feb 2016 18:12:41 +0000 (UTC) Received: from vpn-229-40.phx2.redhat.com (vpn-229-40.phx2.redhat.com [10.3.229.40]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1CICe6P019409; Fri, 12 Feb 2016 13:12:40 -0500 Message-ID: <1455300759.29792.18.camel@redhat.com> Subject: Re: [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x From: David Malcolm To: Jeff Law , gcc-patches@gcc.gnu.org Date: Fri, 12 Feb 2016 18:12:00 -0000 In-Reply-To: <56BD8F06.6050709@redhat.com> References: <1455253960-23499-1-git-send-email-dmalcolm@redhat.com> <56BD8F06.6050709@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2016-02/txt/msg00897.txt.bz2 On Fri, 2016-02-12 at 00:51 -0700, Jeff Law wrote: > On 02/11/2016 10:12 PM, David Malcolm wrote: > > In r227188 I introduced driver::finalize () which resets > > all state within gcc.c, allowing the driver code to embedded > > inside libgccjit and run repeatedly in-process. > > > > Running this on s390x revealed a bug in this cleanup: > > I was cleaning up "specs" via: > > XDELETEVEC (specs); > > and this was crashing on s390x with: > > free(): invalid pointer: 0x000003fffdf30568 *** > > > > Closer investigation revealed that specs == static_specs, > > a statically allocated array. > > > > What's happening is that set_specs sets "specs" to static_specs > > the first time it's called, and any calls to set_specs () with > > a name that isn't in static_specs cause a new spec_list node > > to be dynamically-allocated and put at the front of the list; > > "specs" > > then equals that. > > > > All of my testing had been on x86_64, which inserts exactly one > > spec > > with a name not in the static array, hence the: > > XDELETEVEC (specs); > > happened to work. > > > > On s390x, the only names in the "specs" file are those in the > > static > > array, so specs == static_specs, and the bogus cleanup code > > attempts > > to free a statically-allocated array. > > > > The following patch reworks this part of the cleanup, walking any > > list > > of specs, freeing the dynamically-allocated nodes until the > > statically-allocated ones are reached. > > > > driver::finalize is only called by libgccjit, not by the main > > driver > > program, so this should be relatively low-risk. > > > > Verified directly by running under valgrind on x86_64 and s390x. > > > > With the patch, jit.sum on s390x-ibm-linux-gnu (RHEL 7) goes from: > > # of expected passes 1799 > > # of unexpected failures 61 > > # of unresolved testcases 2 > > to: > > # of expected passes 8514 > > > > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu. > > > > OK for trunk? > > > > gcc/ChangeLog: > > PR driver/69779 > > * gcc.c (driver::finalize): Fix cleanup of "specs". > Can't you just "free (specs->name) rather than messing with the cast? > > OK with that twiddle, assuming it works. Thanks. The problem is that struct spec_list's "name" field is declared const: const char *name; /* name of the spec. */ and likewise for the "name" field within struct spec_list_1: const char *const name; Some are statically allocated, others are dynamically allocated. If I convert them to: char *name; /* name of the spec. */ and: char *const name; respectively, then I run into lots of: warning: ISO C++ forbids converting a string constant to ‘char*’ [ -Wpedantic] Some of these warnings are (relatively) easily fixed by adding a const_cast to gcc.c's INIT_STATIC_SPEC. However, the other warnings are from this: static const struct spec_list_1 extra_specs_1[] = { EXTRA_SPECS }; EXTRA_SPECS is defined in the config-specific subdirectories in numerous ways, and touching those seems like too big a rathole to be going down, especially in stage 4. It seems simpler to keep the "const" on those string fields, and cast it away when cleaning up the dynamically-allocated ones (as in the candidate patch). OK without the twiddle? Dave