From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19487 invoked by alias); 17 Oct 2014 16:59:56 -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 19460 invoked by uid 89); 17 Oct 2014 16:59:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients 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, 17 Oct 2014 16:59:54 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9HGxG5h032028 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 17 Oct 2014 12:59:52 -0400 Received: from stumpy.slc.redhat.com (ovpn-113-71.phx2.redhat.com [10.3.113.71]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9HGsJQ6014829; Fri, 17 Oct 2014 12:54:20 -0400 Message-ID: <544149BB.9020201@redhat.com> Date: Fri, 17 Oct 2014 17:04:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: David Malcolm CC: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org, Ilya Enkovich Subject: Re: [PATCH 4/5] State cleanups -- also note for MPX work References: <1413222308-25753-1-git-send-email-dmalcolm@redhat.com> <1413222308-25753-5-git-send-email-dmalcolm@redhat.com> <54404176.6030309@redhat.com> <1413506508.9513.201.camel@surprise> In-Reply-To: <1413506508.9513.201.camel@surprise> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-10/txt/msg01768.txt.bz2 On 10/16/14 18:41, David Malcolm wrote: >> >> Anyway, just thought it was worth explicitly pointing out that these two >> hunks of work, while they're tackling totally different issues may >> conflict because of an implementation of the MPX bits. > > Presumably my state cleanup patch isn't going to break the MPX bits > though? I haven't been following them in detail. You won't break MPX, but they might break you due to state being saved in file-scoped statics. I haven't really looked at the state they've got and whether or not it needs to be cleared in a JIT world. > > I should spell out that I haven't exhaustively hunted down every > file-scoped static in the source tree; this work merely fixes enough > such state to ensure that the testsuite passes: Noted. We probably need to be more systematic in the future though. What you've done is probably good enough for now though. Particularly since this just affects JIT and we can iterate on the file statics. > > I've got some ideas for automatically tracking down additional > file-scoped static state by analysis of source or of the .o files. Am I > right in assuming I can regard that as followup bugfixing? Yes, this would be fine for follow-up fixing. > > >>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h >>> index 20b5c4e..794403d 100644 >>> --- a/gcc/cgraph.h >>> +++ b/gcc/cgraph.h >>> @@ -2042,6 +2043,8 @@ bool resolution_used_from_other_file_p (enum ld_plugin_symbol_resolution); >>> extern bool gimple_check_call_matching_types (gimple, tree, bool); >>> >>> /* In cgraphunit.c */ >>> +void cgraphunit_c_finalize (void); >> Combine this with the declaration of init_lowered_empty_function so that >> the references to cgraphunit's functions are together. > > I believe they are, since it's immediately followed by... Nevermind, I'm an idiot. Mentally had a "+" in front of the "In cgraphunit" line and thought you'd added another block of stuff for cgraphunit rather than just adding stuff to the existing block. > FWIW I've been experimenting with making the debugging structs be > classes, with the hooks becoming virtual functions. Then all of this > debugging state could be turned into member data of the singleton > instance of the appropriate debugging class, created when the > gcc::context is built in toplev initialization. > > That would be a much cleaner approach (assuming vfuncs would be > acceptable for the debug hooks) - though I don't yet have a working > patch for this. > > So for now I went with the less elegant but equally effective approach. Understood. > >>> diff --git a/gcc/passes.c b/gcc/passes.c >>> index 5001c3d..772993d 100644 >>> --- a/gcc/passes.c >>> +++ b/gcc/passes.c >>> @@ -1944,6 +1944,7 @@ execute_ipa_summary_passes (ipa_opt_pass_d *ipa_pass) >>> if (pass->tv_id) >>> timevar_push (pass->tv_id); >>> >>> + current_pass = pass; >>> ipa_pass->generate_summary (); >> Presumably it doesn't make sense to push this down into >> ipa_pass->generate_summary? > > The issue here is that ipa-pure-const.c has some state that ought to be > member data of the pass_ipa_pure_const instance. The patch makes that > change, the issue is that there's no way to get at the relevant pass > instance just from the IPA pass hooks. They're not vfuncs, they're just > tables of function pointers. > > (I skipped this part of the conversion of passes to C++ classes since > there wasn't a good way to handle the equivalent of checking to see if a > function ptr is NULL when turning it into a vfunc). > > The easy fix is to simply ensure that "current_pass" is set before > jumping through the IPA hook, so that pass_ipa_pure_const's > implementation can find its instance and get at the data. Ok. Thanks for the explanation. jeff