From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11610 invoked by alias); 3 Oct 2014 17:14:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 11594 invoked by uid 89); 3 Oct 2014 17:14:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 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, 03 Oct 2014 17:14:54 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s93HEoeW028403 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 3 Oct 2014 13:14:51 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s93HEmJU013286; Fri, 3 Oct 2014 13:14:49 -0400 Message-ID: <542ED988.8050407@redhat.com> Date: Fri, 03 Oct 2014 17:14:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches , Stan Shebs Subject: Re: Questions on commit 6c95b8df7fef5273da71c34775918c554aae0ea8 References: <54259976.9000907@redhat.com> <21547.2428.934404.571592@ruffy2.mtv.corp.google.com> In-Reply-To: <21547.2428.934404.571592@ruffy2.mtv.corp.google.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-10/txt/msg00075.txt.bz2 On 09/30/2014 08:50 PM, Doug Evans wrote: > Pedro Alves writes: > > On 09/20/2014 07:39 PM, Doug Evans wrote: > > > [Ugh, bad To address on first try.] > > > > > > Hi. > > > > > > While looking into removing lwp_list from gdb (akin to what I did for > > > gdbserver) I found that several bits of target code are calling > > > init_thread_list > > > (grep init_thread_list *.c). > > > Maybe there's the odd case where target code would need to do this, > > > but normally when should target code *ever* do this? > > > > > To try to assist you with getting a handle on my confusion, consider > > > remote.c:extended_remote_create_inferior_1 from gdb 6.8: > > > > > > /* Clean up from the last time we were running. */ > > > init_thread_list (); > > > init_wait_for_inferior (); > > > > > > Now look at what's there today: > > > > > > if (!have_inferiors ()) > > > { > > > /* Clean up from the last time we ran, before we mark the target > > > running again. This will mark breakpoints uninserted, and > > > get_offsets may insert breakpoints. */ > > > init_thread_list (); > > > init_wait_for_inferior (); > > > } > > > > > > > > I think(!) there may be multiple ways to look at all of this as being > > > wrong, so pick your favorite, but here's one way: What does it matter > > > whether there are other inferiors as to whether > > > remote.c:extended_remote_create_inferior has to "clean up from the > > > last time we were running"? > > > > > > Obviously we can't call init_thread_list if there are other inferiors, > > > but why are we calling init_thread_list here at all? Why isn't this > > > state being managed by gdb itself (inf*.c or some such)? I can > > > imagine one can look at this as just being still a work in progress on > > > the way to proper multi-target support. It's stil a bit odd though to > > > have taken this step this way, so I'm hoping for some clarification. > > > > Really not sure what sort of answer you're looking for. > > The most succinct way of expressing what I'm looking for that I can > think of is that I want to understand this code, and I don't. > There are several instances of the !have_inferiors() check, I picked > this one because it seemed like as good a choice as any. init_thread_list does two things: - wipes all threads. - resets the thread id counter back to 0, so the next thread is gdb thread number 1. The main point of calling init_thread_list if there are no live inferiors, is to reset the thread id counter. That is so that with "run; kill; run", you end up with the main thread being gdb thread number 1, in both runs. Now, if there are other live inferiors, then we shouldn't wipe their threads. And as gdb thread ids are global currently, not private per-inferior/process, then we obviously shouldn't reset the thread id counter either. The reason this isn't managed by the core itself, is I believe that nobody has tried to do that yet. > > > Maybe it's a simple > > > oversight, but I think (emphasis on THINK, I could certainly be > > > missing something) we need to take a step back and ask why this code > > > is there at all. Putting this code in target routines gives us a lot > > > of flexibility, but the cost is more mental load (for lack of a better > > > phrase) and more touch points when trying to fix/improve core gdb, and > > > I'm getting the impression that the pendulum has swung too far towards > > > putting general housekeeping operations in target code. > > > > Huh? I think you're getting this backwards. You make it sound like > > we've been adding more of this stuff in target code ("putting"), > > while instead these are _ancient_ code that over the years we've > > been cleaning up. > > Well, yes we have been adding non-target-specific code to target files. > E.g., I don't see how the trustability of compilers to emit good > data for prologues varies by target, and yet we're doing different things > in amd64_skip_prologue vs arm_skip_prologue w.r.t. gcc vs clang. Well, on the compiler side, prologue emission is tied to the backend. Certainly it's easy to picture that different backends on the compiler side vary in quality. For example, I recall that on GCC, there was a long term conversion of prologue emission from text to rtl. A quick google search finds: https://gcc.gnu.org/ml/gcc/2011-06/msg00222.html and https://gcc.gnu.org/ml/gcc-patches/2011-07/msg00685.html for example. I don't know whether something like that affects the cases in question or not. I'm just saying that I could see there being a reason. > I could certainly be wrong here, but I don't think so. > Can gcc get this right for ARM enough that we accept the risk of the > odd bug whereas with AMD64 we've chosen to not accept this risk? > Here's a case where some cleanup is needed, but this is relatively > freshly added code. > [From one perspective *-tdep files are different than > target files like remote.c. But it's still a case of balancing > target-specific and non-target-specific code.] > > Plus, pending an understanding of the !have_inferiors() check above (and > elsewhere), it's not clear to me we have always been cleaning up. I honestly fail to see the relation and I don't think it's useful to try to generalize like that. Each case is a different case. If you find patterns that can be factored out, and find ways to clean things up, then please go for it. Thanks, Pedro Alves