From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11845 invoked by alias); 3 Mar 2017 17:24:36 -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 6889 invoked by uid 89); 3 Mar 2017 17:24:32 -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,KAM_LAZY_DOMAIN_SECURITY,KHOP_DYNAMIC,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=focuses, Pedro, pedro, restoring X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 03 Mar 2017 17:24:29 +0000 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v23HO2dv040546 for ; Fri, 3 Mar 2017 12:24:28 -0500 Received: from e06smtp13.uk.ibm.com (e06smtp13.uk.ibm.com [195.75.94.109]) by mx0a-001b2d01.pphosted.com with ESMTP id 28xs8eaq37-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 03 Mar 2017 12:24:28 -0500 Received: from localhost by e06smtp13.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 3 Mar 2017 17:24:26 -0000 Received: from d06dlp02.portsmouth.uk.ibm.com (9.149.20.14) by e06smtp13.uk.ibm.com (192.168.101.143) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 3 Mar 2017 17:24:23 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (d06relay11.portsmouth.uk.ibm.com [9.149.109.196]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id A23C52190019; Fri, 3 Mar 2017 17:23:23 +0000 (GMT) Received: from d06av21.portsmouth.uk.ibm.com (d06av21.portsmouth.uk.ibm.com [9.149.105.232]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v23HONgx8454408; Fri, 3 Mar 2017 17:24:23 GMT Received: from d06av21.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 4310652041; Fri, 3 Mar 2017 16:23:12 +0000 (GMT) Received: from ThinkPad (unknown [9.152.212.148]) by d06av21.portsmouth.uk.ibm.com (Postfix) with ESMTP id 2CAB052011; Fri, 3 Mar 2017 16:23:12 +0000 (GMT) Date: Fri, 03 Mar 2017 17:24:00 -0000 From: Philipp Rudo To: Simon Marchi Cc: Subject: Re: [PATCH RFC] Decouple user selection from internal selection In-Reply-To: <20170223222850.5511-1-simon.marchi@ericsson.com> References: <20170223222850.5511-1-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 17030317-0012-0000-0000-000004DA9076 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17030317-0013-0000-0000-000017769718 Message-Id: <20170303182421.4ba0d248@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-03_12:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703030158 X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg00025.txt.bz2 Hi Simon I have mixed feelings for this patch. On the one hand I think its good you try to untangle the different uses of the global variables. On the other hand your approach is just an other workaround for the main problem, using global variables. For me the long-term goal should be to get rid of the global variables. This would require to define a "debug_context" which can be a user selected as well as a GDB internal context. This "debug_context" (or parts of it) can then be passed down the stack. So every function gets the information it needs from its caller. This not only gives you flexibility, as you can have different contexts, but also prevents bugs where the global variables are not properly set/restored, like you have with MI. Those different "debug_contexts" will also be needed for the multi-target idea Andreas and I have. With this feature we want to allow the user to take a look at a given problem from the point of view of different targets. For example consider the potential target stack (from [1]): - goroutine (Goroutine) - multi-thread (multi-threaded child process.) <-- linux-thread-db - lk-user (User-space task) <-- focuses on one user-space process - lk-thread (Linux kernel threads) <-- uses the kernel's task list - core (Local core dump file) <-- "threads" are actually CPUs - exec (Local exec file) - None (None) In this scenario there are five different views on threads (from hardware threads to goroutines). The user might be interested in all of them and wants to choose a target and get its perspective on the problem. But switching between different targets, with different views on threads/inferiors, using global variables would be a pain and error prone. That's why we prefer the approach described above. Of course getting rid of the global variables would be a huge job. Here your approach could be an intermediate solution (I understand your "user_selection" to be a "debug_context"). It allows us to update GDB over time by passing it down the stack one after one and apply the context to the global variables for the parts which where not updated, yet.=20=20 Independent to what I said before you should split up this patch into a series to make review easier. For example the introduction of thread_info::put/get in gdbthread.h or scoped_restore_suppress_output in ui-out.c are independent of the user-selection and should go in separate patches. Philipp [1] https://sourceware.org/ml/gdb-patches/2017-02/msg00106.html On Thu, 23 Feb 2017 17:28:50 -0500 Simon Marchi wrote: > I am sending this as an RFC because it's far from complete and > definitive, but I'd like to gather some comments and opinions before > going further in this direction. >=20 > The goal of this patch is to decouple the notion of the user-selected > inferior/thread/frame from GDB's internally selected > inferior/thread/frame. >=20 > Currently, for example, the inferior_ptid variable has two jobs: >=20 > - it's the user-selected thread: it's changed by the "thread" > command. Other commands (continue, backtrace, etc) apply to this > thread. > - it's the internally-selected thread: it defines the thread GDB is > currently "working" on. For example, implementations of > to_xfer_partial will refer to it to know from which thread to > read/write memory. >=20 > Because of this dual usage, if we want to do some operations on a > thread other than the currently selected one, we have to save the > current inferior/thread/frame and restore them when we're done. > Failing to do so would result in an unexpected selection switch for > the user. >=20 > To improve this, Pedro suggested in [1]=C2=A0to decouple the two > concepts. This is essentially what this patch is trying to do. >=20 > A new "user_selection" object is introduced, which contains the > selected inferior/thread/frame from the point of view of the user. > Before every command, we "apply" this selection to the core of GDB to > make sure the internal selection matches the user selection. >=20 > There is a single user selection for the whole GDB (named "global > user-selection"), but as was mentioned in the linked thread, it opens > the door to having different selections for different UIs. This means > that each UI would have its own user-selection object, which would be > applied to the core prior to executing commands from this UI. >=20 > The global user-selection object only gets modified when we really > intend to change it. It can be because of the thread / > -thread-select / up / down / frame / inferior commands, a breakpoint > hit in all-stop, an inferior exit, etc. >=20 > The problem that initially prompted this effort is that the "--thread" > flag of MI commands changes the user-selected thread under the user's > feet. My initial attempt to fix it was to restore the selection after > the MI command execution. However, some cases are hard to get right. > For example: >=20 > (thread 1 is currently selected) > -interpreter-exec --thread 2 console "thread 3" >=20 > Restoring the selected thread to thread 1 after the MI command > execution wrongfully cancels the switch to thread 3. So it's hard to > determine when we should or shouldn't restore. With the current > patch, it works naturally: the --thread flag doesn't touch the > user-selected thread, only the internal one. The "thread 3" command > updates the user selection. >=20 > Another difficulty is to send the right notifications to MI when the > user selection changes. That means to not miss any, but not send too > many either. Getting it somewhat right lead to ugly hacks (see the > command_notifies_uscc_observer function) and even then it's not > perfect (see the kfails in user-selected-context-sync.exp test). > With the proposed method, it's easy to know when the user-selection > changes and send notifications. >=20 > With this patch, there are probably a few usage of > make_cleanup_restore_current_thread that are not needed anymore, if > they are only used to restore the user selection. I kept removing > them for a later time though. >=20 > In the current state, there are a few minor regressions in the > testsuite (especially some follow-fork stuff I'm not sure how to > handle), but the vast majority of the previously passing tests still > pass. >=20 > I've pushed the patch to users/simark/user-selection-rfc on > sourceware. Comments are welcome! >=20 > Thanks, >=20 > Simon >=20 > [1] https://sourceware.org/ml/gdb-patches/2016-08/msg00031.html