From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4896 invoked by alias); 23 Oct 2014 17:36:31 -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 4859 invoked by uid 89); 23 Oct 2014 17:36:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_SOFTFAIL autolearn=no version=3.3.2 X-HELO: mail-la0-f49.google.com Received: from mail-la0-f49.google.com (HELO mail-la0-f49.google.com) (209.85.215.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 23 Oct 2014 17:36:29 +0000 Received: by mail-la0-f49.google.com with SMTP id q1so1246791lam.8 for ; Thu, 23 Oct 2014 10:36:25 -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 :content-transfer-encoding; bh=/tjyO1lpkepx+TUIGf1QjxmNIDqOLRreJn0oBRstl3I=; b=l4Z4m1czbvgfTgElR5ptf7C/0f2pLOh69V+m/e9HW2AJXKVr18CDhKNL2i07y4f8Me bVfC3/RdjdCZdJUqSZ83I2/XUy8b+Ke9+hWL9gAmAd9nfVPST9AzduQDHGZcRR1DZEV2 zbVCxgLF8kwSSYWYZ2TX4uS6WezKJgL4jK0irwxOrXMbWSUaGcRf4X80JNQhtNuKK7oM C5jn1C17kVRJr3V9ytKD2PDExxL/FiHRhP3AyeJwksLK3xX/1EwarGzurgM0aiMDH7zm j3QDDBMzMAbjicCh0sZk8wckXjJRICs7bfZG6Pr/rCkaxvp7M8cP5fqw5F8PIMhJcCf8 ANWQ== X-Gm-Message-State: ALoCoQlQB6nvgSQNm4LaOY7JM6dU4NYR9ceqPw7D9aEE0M3bbp3QlBAte8mLDQ56BkR+jebTMqph MIME-Version: 1.0 X-Received: by 10.112.85.106 with SMTP id g10mr6702804lbz.38.1414085784974; Thu, 23 Oct 2014 10:36:24 -0700 (PDT) Received: by 10.112.59.129 with HTTP; Thu, 23 Oct 2014 10:36:24 -0700 (PDT) In-Reply-To: <544828BB.9040900@redhat.com> References: <1413986485-4673-1-git-send-email-martin.galvan@tallertechnologies.com> <544822D6.8020606@redhat.com> <544828BB.9040900@redhat.com> Date: Thu, 23 Oct 2014 17:36:00 -0000 Message-ID: Subject: Re: [PATCH] Python API: Add gdb.is_in_prologue and gdb.is_in_epilogue. From: Martin Galvan To: Pedro Alves Cc: gdb-patches@sourceware.org, dje@google.com, Eli Zaretskii Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2014-10/txt/msg00608.txt.bz2 First of all, thanks a lot to all of you for the feedback. On Wed, Oct 22, 2014 at 4:23 PM, Doug Evans wrote: > We should have a test for is_in_epilogue anyway. > The canonical choice is to just make it amd64-linux specific. I briefly thought of that. The test I originally wrote for is_in_epilogue checked for the last PC of a known, simple function which ended with retq on amd64 and pop on thumb ARM. It worked fine on ARM, however for some reason the retq was returning False. I looked at amd64_in_function_epilogue_p and saw all it did was check if the opcode was the one for ret. Not being too familiar with amd64 I initially assumed the opcode for retq was different, but after doing an objdump I noticed it's the same (0xc3). I was surprised to see this, and debugged GDB itself only to find out that gdbarch->in_function_epilogue_p was the generic one instead of amd64. Is this a bug? > API functions like these are problematic. > Users don't expect API functions to be heuristic-based, > and that is all these can ever be in the general case. > The patch does try to provide the user some guarantees > ("however if the result is @code{False} you can be sure > @value{GDBN} is right."), > but it's not clear to me this will be true in the general case. > I may have missed something so I'm certainly willing to be > persuaded otherwise. Well, the comment at the top of in_prologue says "Returns 1 if PC *might* be in prologue, 0 if definately (sic) *not* in prologue". However, looking at the function itself it relies on the compiler having marked the prologue as its own "line", and if it can't use that info it falls back to using the architecture-dependant gdbarch_skip_prologue. So far every time I've tested it it's worked fine, and if it didn't it would've probably already been reported as a bug of in_prologue or gdbarch_skip_prologue. I guess if you want to be *really* careful I could change that line in the documentation for something like "if the result is False, you can be almost sure GDB is right", or "GDB is most likely right". I think in this case it's more important to emphasize false positives rather than false negatives. > I might be ok with this patch if the functions were named something like > "maybe_is_in_prologue" and "maybe_is_in_epilogue". > That way they scream to the user (and future code readers) "Heads Up!" Agreed, although from what Pedro said I think it would be even more accurate if we renamed the epilogue function to something like "stack_frame_destroyed". On Wed, Oct 22, 2014 at 6:34 PM, Pedro Alves wrote: > target's whose gdbarch_in_function_epilogue_p is returning true for > instructions in the epilogue before the stack is destroyed, will > have software watchpoints broken in the tail call case Yao described, > like ARM had. > > And so continuing with the in_function_epilogue_p name results in a > misnamed Python hook too; I'd much rather not carry that confusion to > the exported API. That function could well return true if for some > reason we're stopped at code in the middle of the function, not > an in epilogue, that for some reason destroys the stack. > > Or, consider the case of at some point we wanting to expose a > function that actually returns whether the PC is in the > epilogue, no matter whether the stack/frame has been destroyed > or not. Agreed. > How about a test that single-steps out of a function, and checks > in_function_epilogue at each single-step until the function is > finished? We can use istarget "foo" to tweak whether to expect > whether in_function_epilogue returns true in one of the steps > or not. I actually wrote a test that checked is_in_epilogue with the last PC in a very simple function. I used it to test on ARM and it worked fine, however for amd64 the story was different (see my answer to Doug), and that's why I didn't include it. If we were to single-step and do architecture-dependant checks for every single instruction, however, I think the test would be quite complicated. >> + ** Two new functions: gdb.is_in_prologue and gdb.is_in_epilogue, >> + which are wrappers for in_prologue and gdbarch_in_function_epilogu= e_p >> + respectively. > > This is talking about wrapping some GDB internals. I think it'd be > better to say something in user-speak. The implementations of > these new Python functions could change, even. Agreed. How about: ** Two new functions: gdb.maybe_in_prologue and gdb.stack_destroyed. The former returns True if a given PC may be inside a function's prolog= ue and thus the local variables may show wrong values at that point, and the latter returns True if a given PC is at a point where GDB detec= ted the stack frame as having been destroyed, usually by the instructions in a function's epilogue. >> +The optional @var{functionStart} argument is the start address of the >> +function you want to check if @var{pc} belongs to. If your binary >> +doesn't have debugging info, @value{GDBN} may need to use this value >> +to guess if @var{pc} belongs to the prologue. If omitted it defaults >> +to 0. > > Some targets have code at address 0. Seems like we may be exposing a > bad interface for these targets here? I used 0 because in_prologue expects it to be non-zero. If it's 0 and we have no debugging info, it'll always return true: /* We don't even have minsym information, so fall back to using func_start, if given. */ if (! func_start) return 1; /* We *might* be in a prologue. */ Again, I did it because of the way in_prologue works, but as Eli said this would probably be better handled with a Python exception or a message of some kind. > Any reason these new tests aren't in a separate file? Not particularly. I thought they should go in python.exp since my functions belong to the gdb module itself, but I could place them in a separate file if you want. I'm thinking of naming the file py-prologue-epilogue.exp or maybe py-frame-is-invalid.exp. Any better naming suggestions are welcome! >> +# Get the current frame object. >> +gdb_py_test_silent_cmd "python selectedFrame =3D gdb.selected_frame()" \ >> + "Get the current frame object." 0 > > Lowercase, and no "." please. Here and throughout. Usual style is to > drop the "the"s to be more concise: > > gdb_py_test_silent_cmd "python selectedFrame =3D gdb.selected_frame()" \ > "get current frame object" 0 > > BTW, I happened to read this back, and noticed: "current" and "selected" > have distinct meanings in the frame machinery. Best not mix them up: > > gdb_py_test_silent_cmd "python selectedFrame =3D gdb.selected_frame()" \ > "get selected frame object" 0 Will do. One more thing: I'd like to know if everyone's ok with this: On Wed, Oct 22, 2014 at 3:06 PM, Martin Galvan wrote: > On Wed, Oct 22, 2014 at 2:47 PM, Eli Zaretskii wrote: >> I think we are miscommunicating. What I had in mind is raise an >> exception or display an error message when GDB has no other means to >> determine where the function's start is (e.g., no debug info), and no >> functionStart argument was passed. That is what I meant by >> "require". IOW, it's up to the user to decide when to provide it, but >> GDB will refuse to use some arbitrary number, such as zero, if it >> cannot determine the starting address. > > Oh, I see. That's a great idea. > > I think the simplest way to do this is not to call GDB's in_prologue > directly from gdbpy_is_in_prologue, but reproduce some of its behavior > and adding a check on the return value of find_pc_partial_function > which will throw an exception if functionStart wasn't provided. This > may result in a bit of duplicated code, but in_prologue isn't that > long a function if you take the comments out so I don't think that > would be a problem. What do you think? --=20 Mart=C3=ADn Galv=C3=A1n Software Engineer Taller Technologies Argentina San Lorenzo 47, 3rd Floor, Office 5 C=C3=B3rdoba, Argentina Phone: 54 351 4217888 / +54 351 4218211