From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id E878938A2417 for ; Fri, 13 Mar 2020 20:19:45 +0000 (GMT) Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 02DJosfC042631 for ; Fri, 13 Mar 2020 16:19:45 -0400 Received: from ppma04wdc.us.ibm.com (1a.90.2fa9.ip4.static.sl-reverse.com [169.47.144.26]) by mx0b-001b2d01.pphosted.com with ESMTP id 2yrd3tsfft-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 13 Mar 2020 16:19:45 -0400 Received: from pps.filterd (ppma04wdc.us.ibm.com [127.0.0.1]) by ppma04wdc.us.ibm.com (8.16.0.27/8.16.0.27) with SMTP id 02DKAnrv008217 for ; Fri, 13 Mar 2020 20:19:45 GMT Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by ppma04wdc.us.ibm.com with ESMTP id 2yqt6q8t1g-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 13 Mar 2020 20:19:45 +0000 Received: from b01ledav004.gho.pok.ibm.com (b01ledav004.gho.pok.ibm.com [9.57.199.109]) by b01cxnp23032.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 02DKJhFi35258676 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 13 Mar 2020 20:19:43 GMT Received: from b01ledav004.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5B35D112063; Fri, 13 Mar 2020 20:19:43 +0000 (GMT) Received: from b01ledav004.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 323A1112064; Fri, 13 Mar 2020 20:19:43 +0000 (GMT) Received: from pedro.localdomain (unknown [9.18.235.193]) by b01ledav004.gho.pok.ibm.com (Postfix) with ESMTP; Fri, 13 Mar 2020 20:19:43 +0000 (GMT) Received: by pedro.localdomain (Postfix, from userid 1000) id 558713C50DF; Fri, 13 Mar 2020 17:19:39 -0300 (-03) From: Pedro Franco de Carvalho To: Ulrich Weigand Cc: gdb-patches@sourceware.org, ulrich.weigand@de.ibm.com, rcardoso@linux.ibm.com Subject: Re: [PATCH v2 3/3] [PowerPC] Fix debug register issues in ppc-linux-nat In-Reply-To: <20200217174720.3CB09D802EA@oc3748833570.ibm.com> Date: Fri, 13 Mar 2020 17:19:39 -0300 Message-ID: <87sgicgg84.fsf@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.572 definitions=2020-03-13_08:2020-03-12, 2020-03-13 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 adultscore=0 lowpriorityscore=0 malwarescore=0 clxscore=1015 mlxscore=0 bulkscore=0 impostorscore=0 mlxlogscore=999 priorityscore=1501 phishscore=0 suspectscore=1 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003130090 X-Spam-Status: No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Mar 2020 20:19:47 -0000 "Ulrich Weigand" writes: > The one thing I'm wondering about here is that in order to detect > the interface, the code will rely on inferior_ptid. This introduces > a new use of the inferior_ptid deeply buried within a function that > is called from a large number of places -- are we sure all those > callers actually always have a (correct) inferior_ptid in place? > > Now, in practice, inferior_ptid is only needed for the *first* > call, and that first call will likely happen only from a small > subset of all the possible callers here. > > But it would seem cleaner to make this explicit by having an > explicit "initialize" or "detect" call, which gets called in > those places we expect to be "first", and which gets passed > a ptid_t to use (where the callers will still pass inferior_ptid, > but then at least the dependency will be explicit. I'm taking a while to post an updated patch because I've been thinking about this issue in some detail. I agree that it would be better to make the uses of inferior_ptid explicit with a "detect" method that takes the pid. I don't know if all the target methods guarantee that inferior_ptid will be properly set. However, after going through them, I don't believe I have introduced any additional effective uses of inferior_ptid, compared to the old have_ptrace_hwdebug_interface function (which is also a deeply buried use of inferior_ptid), with the exception of low_stopped_data_address and low_stopped_by_watchpoint, but I've checked that linux_nat_target explicitly sets inferior_ptid to the stopped lwp before calling these, so I assume they are allowed to use it. Another exception is that the interface class can call ptrace twice when detecting the interface, since it now also checks for the DEBUGREG interface, while have_ptrace_hwdebug_interface only checked for the HWDEBUG interface. All the new and modified low_ methods only use the interface if it has already been detected previously, otherwise, they don't have any work to do. So in practice they never use inferior_ptid. But here too it would be cleaner if the interface class had an explicit "is_detected" method and these low_ methods don't use "detect". Another solution I considered at length that seemed more natural is to detect the interface in post_startup_inferior and post_attach. ARM does something similar to figure out their debug register capabilities. The post_startup_inferior method seems to clearly work. I'm not sure about post_attach. I saw some weird cases where attaching to a threaded program causes post_attach to be called with inferior_ptid set to one of the extra threads, even though post_attach takes only the pid as an argument and all implementations (e.g. in linux_nat_target) use the this process pid for their ptrace calls. This happens because in all-stop mode all the threads are forced stopped and setup_inferior is called as a consequence of that. I suspect that using this pid would still work since the main thread which is attached to is explicitly waited for in linux_nat_target::attach. I'm not sure if other weird effects can happen in non-stop mode that would cause a watchpoint target method to be called before post_attach is called, e.g. a thread stopping after an attach command is issued, but before post_attach is executed, and the user trying to set a watchpoint in this instant, when the interface won't have been detected yet. I'm having some trouble reasoning about all these possibilities in infrun. Note that linux_nat_target sets ptrace flags in post_attach, but also sets a bool in every additional lwp object to set these same flags for every other thread the next time they stop. Considering these issues, would you advise me to: 1. Just use inferior ptid in the target methods, like it was done before, except more explicitly, with a "detect method". 2. Assume that post_startup_inferior and post_startup will always have been called before any watchpoint-related target method has a chance to be called, and detect the interface only in these two places. I don't know if this assumption holds true. - If so, should every target method that needs the interface assert that it is detected when they are called? This would probably be the easiest thing to do, as I don't know if there are any issues in case something else is done (such as returning an error code) and the interface is not detected in one call but is in a subsequent call. However, considering the issues above, these asserts could be triggered in some weird scenarios. > [ *Maybe* (and I'm not sure here) it would even make sense to move the > ppc_linux_dreg_interface into that per-process struct, to clearly > associate it with the pid that was used to query the kernel? ] This would naturally solve the problem of getting an ESRCH when querying the kernel if the process is unlucky enough to get a sigkill right before the ptrace call. In this case, only the signalled process would get no interface and be prevented from using hardware watchpoints. With the current version of the patch, if this happens, the whole GDB session will be prevented from using hardware watchpoints, since the interface is associated with the native target and there's only one global native target. Before the patch, this would only prevent the HWDEBUG interface from being used, since GDB would still try the DEBUGREG interface in can_use_hw_breakpoint, but this seems accidental and I think is more confusing that testing if DEBUGREG is available in the interface class. However, doing this would introduce additional dependencies on inferior_ptid in all the target methods, since they would need the pid to access the interface. Most of the target methods don't actually use the inferior_ptid, because by the time they are called, the interface will already have been detected, but associating the interface object with the per-process struct would necessarily require using the pid in inferior_ptid. Thanks! -- Pedro Franco de Carvalho