From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [63.128.21.74]) by sourceware.org (Postfix) with ESMTP id C002D385E006 for ; Fri, 27 Mar 2020 04:14:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C002D385E006 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-484-zhRyNX-qO92w0RV87ZnwRw-1; Fri, 27 Mar 2020 00:14:52 -0400 X-MC-Unique: zhRyNX-qO92w0RV87ZnwRw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 79C8F800D5B for ; Fri, 27 Mar 2020 04:14:51 +0000 (UTC) Received: from f31-4.lan (ovpn-115-20.phx2.redhat.com [10.3.115.20]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4AD31CDBD4; Fri, 27 Mar 2020 04:14:48 +0000 (UTC) Date: Thu, 26 Mar 2020 21:14:47 -0700 From: Kevin Buettner To: gdb-patches@sourceware.org Subject: Re: [PATCH v2 3/5] Expand 'fork_inferior' to check whether 'traceme_fun' succeeded Message-ID: <20200326211447.22e2446f@f31-4.lan> In-Reply-To: <20200317154719.2078283-4-sergiodj@redhat.com> References: <20200226200542.746617-1-sergiodj@redhat.com> <20200317154719.2078283-1-sergiodj@redhat.com> <20200317154719.2078283-4-sergiodj@redhat.com> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP 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, 27 Mar 2020 04:15:04 -0000 Hi Sergio, See my comments below. On Tue, 17 Mar 2020 11:47:17 -0400 Sergio Durigan Junior wrote: > + if (child_errno != 0) > + { > + /* The child can't use TRACE_TRACEME. We have to check whether > + we know the reason for the failure, and then error out. */ > + std::string reason = trace_me_fail_reason (child_errno); I think you mean PTRACE_TRACEME in the comment, right? [...] > +/* Helper struct for fork_inferior_1, containing information on > + whether we should check if TRACEME_FUN was successfully called or > + not. */ > + > +struct traceme_info > +{ > + /* True if we should check whether the call to 'traceme_fun > + (TRACE_ME...)' succeeded or not. */ > + bool check_trace_me_fail_reason; While reading the patch, I noticed that some things have "traceme" or "TRACEME" in the their names while others instead use "trace_me" or "TRACE_ME". While it's already the case that GDB is inconsistent in this regard, I think it might be a good to not propogate that inconsistency further. It's likely that this came about due to the two differently named constants used with ptrace(), i.e. PTRACE_TRACEME and PT_TRACE_ME. Since these constants are used on different platforms, I don't think there's any way we can reconcile these. But for, for names that we pick, I think we should choose either "traceme" or "trace_me" and stick with it. I prefer "traceme" / "TRACEME", but won't object though if you choose the other option. > + > + union > + { > + /* The non-check version of TRACEME_FUN. It will be set if > + CHECK_TRACEME_FAIL_REASON is false. "CHECK_TRACEME_FAIL_REASON" in this comment doesn't match the declaration above in which check_trace_me_fail_reason (with the _ between "trace" and "me") is used instead. There are several instances; I'll only point out this one. [...] > +/* Pointer to function which can be called by > + 'check_child_trace_me_errno' when we need to determine the reason > + of a e.g. 'ptrace (PTRACE_ME, ...)' failure. ERR is the ERRNO > + value set by the failing ptrace call. > + > + By default, the function returns an empty string (see > + fork-inferior.c). > + > + This pointer can be overriden by targets that want to personalize > + the error message printed when trace fails (see linux-nat.c or > + gdbserver/linux-low.c, for example). */ > +extern std::string (*trace_me_fail_reason) (int err); On my first pass through this patch, I had concerns about the introduction of this global variable. (That's why I asked for a new set of patches.) I can't think of a better way to do it though. Kevin