From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 892 invoked by alias); 31 Jul 2014 17:51:58 -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 880 invoked by uid 89); 31 Jul 2014 17:51:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-vc0-f174.google.com Received: from mail-vc0-f174.google.com (HELO mail-vc0-f174.google.com) (209.85.220.174) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 31 Jul 2014 17:51:56 +0000 Received: by mail-vc0-f174.google.com with SMTP id la4so4872408vcb.19 for ; Thu, 31 Jul 2014 10:51:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=o3Dtmjdo71LvcRCaaj169EEiQmOILqtZKwGMRsMFsWA=; b=YNT9NLP+NdFacb4xpN2J/Q+19nnRQu1gEBmjwNN979FoSy3rDKZt3TSojad42bs+lI WBsSPSH9gChglLkuWI7WG4jvmkTPU2Llw1/HfvJLpkiQ6R9lsyjhJrMrbf4oB38uX6Hu wFZxQuBXNufO2rIfNFOe52w2cRgefdkmX+gjHH8BA2UATbRQ1At1+SdAGz0yCeJxwF/E OxW8vsGiLQ9LXxVMpTkl1UeXIq6YKHjgECM+FMJa6yuyeRBvieXriZ01kb7kXGj0MkgY YQVUbI5xxXEBNFYhQYP+POBUEtUUcOBLhj8nSR1sNGINS/XApJSy+zKTO88LXm+/+I1G n46Q== X-Gm-Message-State: ALoCoQmQ0fJtQLZsxhkNvlva1AdcsApgKXHm9MdfP0va06NTsWM3R1wMqJ9Fnx810/iFKm+UT580 MIME-Version: 1.0 X-Received: by 10.220.187.134 with SMTP id cw6mr14984248vcb.3.1406829114058; Thu, 31 Jul 2014 10:51:54 -0700 (PDT) Received: by 10.52.28.233 with HTTP; Thu, 31 Jul 2014 10:51:53 -0700 (PDT) In-Reply-To: References: <83oay128ca.fsf@gnu.org> <87ioo7uuqm.fsf@fleche.redhat.com> Date: Thu, 31 Jul 2014 18:53:00 -0000 Message-ID: Subject: Re: [PATCH] Add Frame.read_register to Python API From: Doug Evans To: Alexander Smundak Cc: Tom Tromey , gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00827.txt.bz2 On Thu, Jul 24, 2014 at 9:41 AM, Doug Evans wrote: > On Mon, Jul 14, 2014 at 9:13 AM, Alexander Smundak wrote: >> Ping. >> It has been a month since I have responded to the reviewer's comments. >> Perhaps someone can take a look at this patch? >> >> On Mon, Jul 7, 2014 at 1:59 PM, Alexander Smundak wrote: >>> Ping. >>> >>> On Mon, Jun 30, 2014 at 9:22 AM, Alexander Smundak wrote: >>>> Ping >>>> >>>> On Mon, Jun 23, 2014 at 3:46 PM, Alexander Smundak wrote: >>>>> Ping >>>>> >>>>> On Wed, Jun 18, 2014 at 10:18 AM, Alexander Smundak wrote: >>>>>> Ping. >>>>>> >>>>>> On Wed, Jun 11, 2014 at 4:52 PM, Alexander Smundak wrote: >>>>>>>> Alexander> def __init__(self, fobj): >>>>>>>> Alexander> super(InlinedFrameDecorator, self).__init__(fobj) >>>>>>>> Alexander> + self.fobj = fobj >>>>>>>> >>>>>>>> Alexander> def function(self): >>>>>>>> Alexander> - frame = fobj.inferior_frame() >>>>>>>> Alexander> + frame = self.fobj.inferior_frame() >>>>>>>> Alexander> name = str(frame.name()) >>>>>>>> >>>>>>>> I think this is a nice fix but it seems unrelated to the patch at hand. >>>>>>>> >>>>>>>> Alexander> @defun Frame.find_sal () >>>>>>>> Alexander> -Return the frame's symtab and line object. >>>>>>>> Alexander> +Return the frame's @code{gdb.Symtab_and_line} object. >>>>>>>> >>>>>>>> Likewise. >>>>>>> >>>>>>> Should I mail these two as a single patch or as two separate patches? >>>>>>> >>>>>>>> Alexander> + FRAPY_REQUIRE_VALID (self, frame); >>>>>>>> Alexander> + if (!PyArg_ParseTuple (args, "i", ®num)) >>>>>>>> Alexander> + { >>>>>>>> Alexander> + const char *regnum_str; >>>>>>>> Alexander> + PyErr_Clear(); /* Clear PyArg_ParseTuple failure above. */ >>>>>>>> Alexander> + if (PyArg_ParseTuple (args, "s", ®num_str)) >>>>>>>> Alexander> + { >>>>>>>> Alexander> + regnum = user_reg_map_name_to_regnum (get_frame_arch (frame), >>>>>>>> Alexander> + regnum_str, >>>>>>>> Alexander> + strlen (regnum_str)); >>>>>>>> Alexander> + } >>>>>>>> Alexander> + } >>>>>>>> >>>>>>>> I tend to think this would be clearer if the arguments were only parsed >>>>>>>> once and then explicit type checks were applied to the resulting object. >>>>>>> >>>>>>> Did that, and then started doubting whether it is really necessary to read >>>>>>> a register by its (very arch-specific) number. The new version supports >>>>>>> reading the register by the name. Another change is that it now throws >>>>>>> an exception if the name is wrong. >>>>>>> >>>>>>>> Alexander> +# On x86-64, PC is register 16. >>>>>>>> Alexander> +gdb_test "python print ('result = %s' % ((f0.architecture().name() != 'i386:x86-64') or f0.read_register('pc') == f0.read_register(16)))" \ >>>>>>>> Alexander> + "True" \ >>>>>>>> Alexander> + "test Frame.read_register(regnum)" >>>>>>>> >>>>>>>> A test that is arch-specific needs to be conditionalized somehow. >>>>>>> IMHO it's borderline arch-specific -- it is runnable on any platform, >>>>>>> although it will not be testing much on any but x86-64. There hasn't >>>>>>> been any arch-specific tests for Python so far, so I am not sure what to do. >>>>>>> >>>>>>> Here's the new version (style violations have been addressed, too): >>>>>>> >>>>>>> The ability to read registers is needed to use Frame Filter API to >>>>>>> display the frames created by JIT compilers. >>>>>>> >>>>>>> gdb/Changelog >>>>>>> 2014-06-11 Sasha Smundak >>>>>>> >>>>>>> * python/py-frame.c (frapy_read_register): New function. >>>>>>> >>>>>>> 2014-06-11 Sasha Smundak >>>>>>> >>>>>>> * python.texi (Frames in Python): Add read_register description. >>>>>>> >>>>>>> 2014-06-11 Sasha Smundak >>>>>>> >>>>>>> * gdb.python/py-frame.exp: Test Frame.read_register. > > Hi. > > For myself, I don't like to step in once another reviewer has engaged the patch. > [Not that I won't. Only that I don't want to usurp someone else's > review - it's hard to make sure one has sufficiently covered > everything the other reviewer raised as issues.] > > Eli, I realize the doc parts are ok. Thanks! > > Tom: Anything else that needs to be done? Ping. If I'm given the "OK" to take over review of this patch, that's cool. I'd just rather not do so without an explicit handoff.