From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90588 invoked by alias); 1 Mar 2018 11:09:27 -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 90578 invoked by uid 89); 1 Mar 2018 11:09:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=road, personal X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Mar 2018 11:09:25 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C7E4040FB646; Thu, 1 Mar 2018 11:09:23 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1613C2026E04; Thu, 1 Mar 2018 11:09:22 +0000 (UTC) Subject: Re: [PATCH] btrace, gdbserver: check btrace target pointers To: Yao Qi , "Metzger, Markus T" References: <1519727985-17914-1-git-send-email-markus.t.metzger@intel.com> <86sh9lgjxt.fsf@gmail.com> Cc: "gdb-patches@sourceware.org" From: Pedro Alves Message-ID: Date: Thu, 01 Mar 2018 11:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-03/txt/msg00006.txt.bz2 On 03/01/2018 09:14 AM, Yao Qi wrote: > On Wed, Feb 28, 2018 at 5:10 PM, Metzger, Markus T > wrote: >> >> I moved two of those checks into the target_* method since I thought >> it would be a good idea to be able to handle exceptions and since I had >> to add the TRY/CATCH/CATCH_END, anyway, I thought it made sense >> to handle the nullptr case also via exceptions. >> >> If we're not doing it that way, the caller would need to check the pointer >> (either directly or via a *_p () function like we do with gdbarch) and still >> be prepared to handle exceptions from the actual call. >> >> My motivation was to simplify the caller but if you think the other way >> is clearer I can move the check back into the callers. Is that your preference? >> > > I don't have a strong opinion on this. It is more about personal flavour of > writing code. You are the major btrace contributor, so it is better to keep > the code in a way you prefer. Patch is good to me, please push. To me too. A comment based on my experience hacking on the multi-target branch: I like imagining how the code would look like if it were C++-ified already. Here, if gdbserver's target_ops function pointers are converted to C++ methods, then it'll no longer be possible to check for null pointer. [1] So, whatever we do, I think checking for null pointers in function pointer tables is the option that just kicks the can down the road. The only options here then will be error out or check a *_p() function upfront. The latter is another way to say, go back to having a supports_btrace method, I guess. [*] - I've run into some cases like these in my gdb target_ops C++-ification in my multi-target branch. Like e.g., I had to add target_ops::can_create_inferior to replace target_ops->to_create_inferior null pointer checks. Thanks, Pedro Alves