From patchwork Wed Nov 18 20:58:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 9733 Received: (qmail 30647 invoked by alias); 18 Nov 2015 20:58:45 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 30627 invoked by uid 89); 18 Nov 2015 20:58:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Wed, 18 Nov 2015 20:58:42 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id EF115118278; Wed, 18 Nov 2015 15:58:40 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id GTVHUxyrlIgt; Wed, 18 Nov 2015 15:58:40 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id CE9EC2964A; Wed, 18 Nov 2015 15:58:40 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 62DA1405BE; Wed, 18 Nov 2015 15:58:40 -0500 (EST) Date: Wed, 18 Nov 2015 15:58:40 -0500 From: Joel Brobecker To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: RFA: [ARM] "svc" insn check at irrelevant address in ARM unwind info sniffer Message-ID: <20151118205840.GA3603@adacore.com> References: <1447092513-20690-1-git-send-email-brobecker@adacore.com> <86y4e5wxy9.fsf@gmail.com> <20151115172537.GA10374@adacore.com> <861tbqw8p5.fsf@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <861tbqw8p5.fsf@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) > > I will start by saying that I see you point. If we're in a frame whose > > first instruction is a call, then we could be seeing the same issue. > > I would also argue that innermost-ness is important, here, assuming > > that we agree that the ARM info is only correct at the point of call. > > So, strictly speaking, we should not even be attempting to use it > > for innermost frame. > > Yes, that is true. > > Should I work on a version of the patch that merges the two ideas? > > Or do you stand with just checking "get_frame_pc (this_frame) > > > func_start"? I confess I know ARM unwind info just enough to get by, > > so I trust your judgement on this. > > Let us start from the simple thing, just checking > "get_frame_pc (this_frame) > func_start". If we need to check > innermost-ness for other problems in the future, we can add it then. I have had a little bit of time to think this over while in the plane, and I am not convinced that checking for "get_frame_pc (this_frame) > func_start" is really treating the symptom rather than the ailment. Here is how I understand things: The code is trying to figure out if our frame corresponds to a system call or not. For that, it searches the instruction at "get_frame_pc (this_frame) - 2" (or -4 for non-thumb). To me, the reason for the -2 or the -4, is that it implicitly makes the assumption that "get_frame_pc (this_frame)" is a *return address*. So, it has to check the instruction immediately before that return address. That assumption is not valid for the inner-most address. Imagine we have a function whose code has been compiled into: ...stuff... svc #imm insn #2 <<<-- breakpoint here ... and we're stopped at the breakpiont. In this case, get_frame_pc will return the address in "insn #2", and therefore see an "svc" instruction just before it, and conclude that we're stuck on a system call, which is not true. If we really want to go simple, perhaps what we can do is just the following (untested): @@ -2787,6 +2787,11 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self, /* See if we have an ARM exception table entry covering this address. */ addr_in_block = get_frame_address_in_block (this_frame); + + /* Add a comment explaining why here... */ + if (get_frame_pc (this_frame) != addr_in_block) + return 0; + entry = arm_find_exidx_entry (addr_in_block, &exidx_region); if (!entry) return 0; ... but the downside of it is that it shunts the code that decides to use the ARM table when there is no minimal symbol for our address. Before we make this decision, however, we check whether we actually have *symbol* information for the current frame. If not, prologue parsing would not work anyway, so we might as well use the exception table and hope for the best. */ if (find_pc_partial_function (addr_in_block, NULL, &func_start, NULL)) So, in my opinion, the patch I initially propose is still best. When you look at the patch with whitespace changes removed (attached), it is actually fairly tiny. Thanks! diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c index 73f26b9..f755076 100644 --- a/gdb/arm-tdep.c +++ b/gdb/arm-tdep.c @@ -2818,7 +2818,17 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self, /* We also assume exception information is valid if we're currently blocked in a system call. The system library is supposed to - ensure this, so that e.g. pthread cancellation works. */ + ensure this, so that e.g. pthread cancellation works. + + But before verifying the instruction at the point of call, make + sure this_frame is actually making a call (or, said differently, + that it is not the innermost frame). For that, we compare + this_frame's PC vs this_frame's addr_in_block. If equal, it means + there is no call (otherwise, the PC would be the return address, + which is the instruction after the call). */ + + if (get_frame_pc (this_frame) != addr_in_block) + { if (arm_frame_is_thumb (this_frame)) { LONGEST insn; @@ -2837,6 +2847,7 @@ arm_exidx_unwind_sniffer (const struct frame_unwind *self, && (insn & 0x0f000000) == 0x0f000000 /* svc */) exc_valid = 1; } + } /* Bail out if we don't know that exception information is valid. */ if (!exc_valid)