From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by sourceware.org (Postfix) with ESMTPS id 72ED53860017 for ; Thu, 18 Jun 2020 17:42:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 72ED53860017 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x441.google.com with SMTP id t18so6966211wru.6 for ; Thu, 18 Jun 2020 10:42:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gR6GYeEXUoGG+pdjMntktADIQAWqLNmXA3vpT6TcNS4=; b=f2B1SOB0KvS2YtXK3Iwso5WQ8LiOV/CaHoEpWwNZHAqly0zNv3wbNTUQPnvtY11ldk LId7paZUWk7k7VKJgul5r6Uwu5tCnlkjyRyy0X/n4GUXGvQMYFOVI5Cvf6PmsPhZh+jg T6XX4o4TKag2uubJU8dJUxc9YzSNIhsX+ir/b2JdowOJ+ygCqZD5H/4XSxpQjFisAiu5 wwxBPmTtk13FFLGNdOqFC/KfYoLRq7x74dgl1mOpXmGCO6dI3AHPy8Wn5jv1Ld7xSxb5 BPv3rWjaD8y7YJNB3+WpESD3HOuy+oEKY64JOVEMzAkwfeOZIMJ7CaNeIawMxv2aUQDc gVsQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gR6GYeEXUoGG+pdjMntktADIQAWqLNmXA3vpT6TcNS4=; b=GADHyns0vqWKaJ+vMQyPc6yfT+nm0xZnTcCNWrQVnwFSdHf+3KSduYy9gGdGIueI6Q eKi/xkwRfz0dXBGNs6YXMFyXx8Q6BCZ6cWwpVsVENkLUvZiTRbY6peJGqQLdi/bSiLj1 o32ykNPlJw1IOyja3ic7uEhkQ1rrgrcE7JICdYdTIX7Ey8LunX5m2zYDVO3h1PFNi+0w qkL+xRqMdNQVc91rJYe8SQfivl2y83WQgbfAGmO+cTJCVo6wXm2TE/8FzI18yRTEZnJ8 sU/TYqDYygEtPDHO4uVPnwlA6FcKb4GjkeDze6XmaUSloW/S16bMoiDnqXquUN9f4UVv R1Iw== X-Gm-Message-State: AOAM530h6dturtMZbie/jyDs36kJmpXNCO8d1G2J11gawId4SiViw6my 9ovet3CThP5QUslFC0hf4OZdQWg6Ozc= X-Google-Smtp-Source: ABdhPJzMVTfb+n/dOUXreOTa42yzaED55fJQUfc4msTBkVTVHtnEqvdJUX/0fbwbqpT02JokLKzELQ== X-Received: by 2002:adf:fec9:: with SMTP id q9mr6269169wrs.172.1592502175298; Thu, 18 Jun 2020 10:42:55 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id x66sm4353089wmb.40.2020.06.18.10.42.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Jun 2020 10:42:54 -0700 (PDT) Date: Thu, 18 Jun 2020 18:42:54 +0100 From: Andrew Burgess To: Luis Machado Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames Message-ID: <20200618174254.GB2737@embecosm.com> References: <0a457a2e-90c6-f6b0-1b60-1f12d5d1d3e3@linaro.org> <20200618082553.GV2737@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 18:41:40 up 10 days, 7:48, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, 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: Thu, 18 Jun 2020 17:42:59 -0000 * Luis Machado [2020-06-18 07:29:53 -0300]: > On 6/18/20 5:25 AM, Andrew Burgess wrote: > > * Luis Machado [2020-06-17 18:14:26 -0300]: > > > > > On 6/17/20 2:38 PM, Andrew Burgess wrote: > > > > I observed that when making use of a Python frame unwinder, if the > > > > frame sniffing code accessed any registers within an inline frame then > > > > GDB would crash with this error: > > > > > > > > gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed. > > > > > > > > The problem is that, when in the Python unwinder I write this: > > > > > > > > pending_frame.read_register ("register-name") > > > > > > > > This is translated internally into a call to `value_of_register', > > > > which in turn becomes a call to `value_of_register_lazy'. > > > > > > > > Usually this isn't a problem, `value_of_register_lazy' requires the > > > > next frame (more inner) to have a valid frame_id, which will be the > > > > case (if we're sniffing frame #1, then frame #0 will have had its > > > > frame-id figured out). > > > > > > > > Unfortunately if frame #0 is inline within frame #1, then the frame-id > > > > for frame #0 _is_ same as the frame-id for frame #1 - the frame-id for > > > > frame #0 requires us to first compute the frame-id for frame #1. As a > > > > result it is not same to call `value_of_register_lazy' before the > > > > frame-id of the current frame is known. > > > > > > > > The solution I propose here is that `value_of_register' stops making a > > > > lazy register value (the step that requires the frame-id, and thus > > > > causes the problems), and instead calls `get_frame_register_value'. > > > > This avoids the need to calculate the frame-id and so avoids the > > > > problem. > > > > > > I believe this situation is similar to the one I was investigating a while > > > ago with the tailcall/inline frame breakage, and it is a bit messy. > > > > > > The values returned from get_frame_register* functions (what you call now) > > > and frame_unwind_register* functions (I believe this is what > > > value_of_register_lazy ends up calling) may not be the same in all > > > situations. > > > > I find that hard to believe given that: > > > > struct value * > > get_frame_register_value (struct frame_info *frame, int regnum) > > { > > return frame_unwind_register_value (frame->next, regnum); > > } > > > > Previously, a call to value_of_register would result in: > > > > * value_of_register - creates a lazy register value with a call to > > value_of_register_lazy, and then immediately resolves it with a call > > to value_fetch_lazy. > > > > * value_fetch_lazy would call value_fetch_lazy_register, which would > > then call frame_unwind_register_value to unwind the register value. > > > > Now we have: > > > > * value_of_register calls get_frame_register_value. > > > > * get_frame_register_value calls frame_unwind_register_value. > > Looking at the code again, I agree that they do converge. The difference is > mostly that get_frame_register* starts right off the bat by passing the next > frame id, whereas value_of_register gets this from value_of_register_lazy. > So you're dealing with unwinding from the next frame. > > Take, for example, default_unwind_pc () (it calls frame_unwind_register* for > the current frame), which is what I was dealing with at the time. It will > try to unwind PC from the current frame first. Only then GDB will ask the > next frame for PC. So there is a subtle difference compared to > get_frame_register*, which calls frame_unwind_register for the next frame. > > The regressions commit 5939967b355ba6a940887d19847b7893a4506067 created > (defaulted to get_frame_register) for x86_64 were fixed by > 991a3e2e9944a4b3a27bd989ac03c18285bd545d (added the conditional block). > > It may be the case that value_of_register has always fetched things from the > next frame, without considering the current frame's idea of what the > register value is. We may not be seeing failures because we are not > exercising this situation. Just in case you'd not spotted I followed up to the thread for your original patch 5939967b355ba6a940887d19847b7893a4506067. Thanks, Andrew > > In any case, I just thought I'd mention this, as it is a bit of a trap when > you're dealing with the unwinding code. > > > > > The two code paths have converged. > > > > Additionally I don't see anywhere in value_fetch_lazy, or > > value_fetch_lazy_register that GDB could muck around with the value > > returned such that it might give us back a different value. > > > > BUT, there is one difference. Calling frame_unwind_register_value > > might itself return a lazy value (for example a lazy memory value for > > a register spilled to the stack), value_fetch_lazy ensures that these > > are resolved. > > > > This makes sense, after calling value_fetch_lazy I really should have > > a non-lazy value. However, I believe it should be fine if > > value_of_register returns a lazy value. That it didn't before does > > worry me _slightly_, but until I see a good reason why it _must_ > > return a non-lazy value I'm inclined to take the position that > > returning a lazy value is fine, and anyone who calls that function > > should only be using the value API to get the value contents, and > > anyone who does that will automatically resolve the lazy value at that > > point. > > > > > > > > The difference comes from the fact that some targets have enough CFI > > > information to compute a register's value according to DWARF rules (which > > > frame_unwind_register* functions use), without requiring information from > > > the previous frame. > > > > > > Some other targets just assume "SAME_VALUE" and end up fetching the register > > > from the previous frame, which requires a valid frame_id. In this last case, > > > calling get_frame_register* will yield the same result, since it will fetch > > > the data from the previous frame anyway. > > > > > > I'd pay special attention to x86_64, which is one I noticed has enough CFI > > > information in some cases, whereas AArch64 doesn't. > > > > > > In summary, the functions are not equivalent and don't take the same paths > > > all the time. The documentation is not too clear unfortunately, so there's > > > room for falling into traps. :-) > > > > I'll try to track down the thread your talking about. It sounds > > ... concerning ... that two APIs to read registers from the same frame > > should return different values. You kind of touched on _how_ that > > might happen, but I don't feel I really understand _why_, so I'd > > certainly be interested to learn more. > > > > > > Thanks, > > Andrew > > > > > > > > An alternative solution would be to only call get_frame_register_value if > > > the frame id is not yet computed, which is certain to drive us into an > > > assertion in that case. > > > > > > > > > > > This bug has crept in because we allowed calls to the function > > > > `value_of_register_lazy' at a time when the frame-id of the frame > > > > passed to the function didn't have its frame-id calculated. This is > > > > only ever a problem for inline frames. To prevent bugs like this > > > > getting into GDB in the future I've added an assert to the function > > > > `value_of_register_lazy' to require the current frame have its id > > > > calculated. > > > > > > > > If we are calling from outside of the frame sniffer then this will > > > > _always_ be true, so is not a problem. If we are calling this > > > > function from within a frame sniffer then it will always trigger. > > > > > > > > gdb/ChangeLog: > > > > > > > > * findvar.c (value_of_register): Use get_frame_register_value > > > > rather than value_of_register_lazy. > > > > (value_of_register_lazy): Add new assert, and update comments. > > > > > > > > gdb/testsuite/ChangeLog: > > > > > > > > * gdb.python/py-unwind-inline.c: New file. > > > > * gdb.python/py-unwind-inline.exp: New file. > > > > * gdb.python/py-unwind-inline.py: New file. > > > > --- > > > > gdb/ChangeLog | 6 ++ > > > > gdb/findvar.c | 28 ++++++-- > > > > gdb/testsuite/ChangeLog | 6 ++ > > > > gdb/testsuite/gdb.python/py-unwind-inline.c | 37 ++++++++++ > > > > gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++ > > > > gdb/testsuite/gdb.python/py-unwind-inline.py | 71 +++++++++++++++++++ > > > > 6 files changed, 192 insertions(+), 5 deletions(-) > > > > create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c > > > > create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp > > > > create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py > > > > > > > > diff --git a/gdb/findvar.c b/gdb/findvar.c > > > > index c7cd31ce1a6..e5445b34930 100644 > > > > --- a/gdb/findvar.c > > > > +++ b/gdb/findvar.c > > > > @@ -263,16 +263,13 @@ struct value * > > > > value_of_register (int regnum, struct frame_info *frame) > > > > { > > > > struct gdbarch *gdbarch = get_frame_arch (frame); > > > > - struct value *reg_val; > > > > /* User registers lie completely outside of the range of normal > > > > registers. Catch them early so that the target never sees them. */ > > > > if (regnum >= gdbarch_num_cooked_regs (gdbarch)) > > > > return value_of_user_reg (regnum, frame); > > > > - reg_val = value_of_register_lazy (frame, regnum); > > > > - value_fetch_lazy (reg_val); > > > > - return reg_val; > > > > + return get_frame_register_value (frame, regnum); > > > > } > > > > /* Return a `value' with the contents of (virtual or cooked) register > > > > @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum) > > > > next_frame = get_next_frame_sentinel_okay (frame); > > > > - /* We should have a valid next frame. */ > > > > + /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id > > > > + for the next frame will require that FRAME has a valid frame-id. > > > > + Usually this is not a problem, however, if this function is ever > > > > + called from a frame sniffer, the small window of time when not all > > > > + frames have yet had their frame-id calculated, this function will > > > > + trigger an assert within get_frame_id that a frame at a level > 0 > > > > + doesn't have a frame id. > > > > + > > > > + Instead of waiting for an inline frame to reveal an invalid use of > > > > + this function, we instead demand that FRAME must have had its frame-id > > > > + calculated before we use this function. > > > > + > > > > + The one time it is safe to call this function when FRAME does not yet > > > > + have a frame id is when FRAME is at level 0, in this case the > > > > + assertion in get_frame_id doesn't fire, and instead get_frame_id will > > > > + correctly compute the frame id for us. */ > > > > + gdb_assert (frame_relative_level (frame) == 0 > > > > + || frame_id_computed_p (frame)); > > > > + > > > > + /* We should have a valid next frame too. Given that FRAME is more > > > > + outer than NEXT_FRAME, and we know that FRAME has its ID calculated, > > > > + then this must always be true. */ > > > > gdb_assert (frame_id_p (get_frame_id (next_frame))); > > > > reg_val = allocate_value_lazy (register_type (gdbarch, regnum)); > > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c > > > > new file mode 100644 > > > > index 00000000000..0cfe06dd273 > > > > --- /dev/null > > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c > > > > @@ -0,0 +1,37 @@ > > > > +/* Copyright 2019-2020 Free Software Foundation, Inc. > > > > + > > > > + This program is free software; you can redistribute it and/or modify > > > > + it under the terms of the GNU General Public License as published by > > > > + the Free Software Foundation; either version 3 of the License, or > > > > + (at your option) any later version. > > > > + > > > > + This program is distributed in the hope that it will be useful, > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > + GNU General Public License for more details. > > > > + > > > > + You should have received a copy of the GNU General Public License > > > > + along with this program. If not, see . */ > > > > + > > > > +volatile int global_var; > > > > + > > > > +int __attribute__ ((noinline)) > > > > +bar () > > > > +{ > > > > + return global_var; > > > > +} > > > > + > > > > +static inline int __attribute__ ((always_inline)) > > > > +foo () > > > > +{ > > > > + return bar (); > > > > +} > > > > + > > > > +int > > > > +main () > > > > +{ > > > > + int ans; > > > > + global_var = 0; > > > > + ans = foo (); > > > > + return ans; > > > > +} > > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp > > > > new file mode 100644 > > > > index 00000000000..f7c65f6afc8 > > > > --- /dev/null > > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp > > > > @@ -0,0 +1,49 @@ > > > > +# Copyright (C) 2020 Free Software Foundation, Inc. > > > > + > > > > +# This program is free software; you can redistribute it and/or modify > > > > +# it under the terms of the GNU General Public License as published by > > > > +# the Free Software Foundation; either version 3 of the License, or > > > > +# (at your option) any later version. > > > > +# > > > > +# This program is distributed in the hope that it will be useful, > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > +# GNU General Public License for more details. > > > > +# > > > > +# You should have received a copy of the GNU General Public License > > > > +# along with this program. If not, see . > > > > + > > > > +# This script tests GDB's handling of using a Python unwinder in the > > > > +# presence of inlined frames. > > > > + > > > > +load_lib gdb-python.exp > > > > + > > > > +standard_testfile > > > > + > > > > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ > > > > + debug] } { > > > > + return -1 > > > > +} > > > > + > > > > +# Skip all tests if Python scripting is not enabled. > > > > +if { [skip_python_tests] } { continue } > > > > + > > > > +# The following tests require execution. > > > > +if ![runto_main] then { > > > > + fail "can't run to main" > > > > + return 0 > > > > +} > > > > + > > > > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py] > > > > + > > > > +gdb_breakpoint "foo" > > > > + > > > > +gdb_test "source ${pyfile}" "Python script imported" \ > > > > + "import python scripts" > > > > + > > > > +gdb_continue_to_breakpoint "foo" > > > > + > > > > +gdb_test_sequence "backtrace" "backtrace with dummy unwinder" { > > > > + "\\r\\n#0 foo \\(\\)" > > > > + "\\r\\n#1 main \\(\\)" > > > > +} > > > > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py > > > > new file mode 100644 > > > > index 00000000000..7206a0b2830 > > > > --- /dev/null > > > > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py > > > > @@ -0,0 +1,71 @@ > > > > +# Copyright (C) 2020 Free Software Foundation, Inc. > > > > + > > > > +# This program is free software; you can redistribute it and/or modify > > > > +# it under the terms of the GNU General Public License as published by > > > > +# the Free Software Foundation; either version 3 of the License, or > > > > +# (at your option) any later version. > > > > +# > > > > +# This program is distributed in the hope that it will be useful, > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > +# GNU General Public License for more details. > > > > +# > > > > +# You should have received a copy of the GNU General Public License > > > > +# along with this program. If not, see . > > > > + > > > > +# A dummy stack unwinder used for testing the Python unwinders when we > > > > +# have inline frames. This unwinder will never claim any frames, > > > > +# instead, all it does it try to read all registers possible target > > > > +# registers as part of the frame sniffing process.. > > > > + > > > > +import gdb > > > > +from gdb.unwinder import Unwinder > > > > + > > > > +apb_global = None > > > > + > > > > +class dummy_unwinder (Unwinder): > > > > + """ A dummy unwinder that looks at a bunch of registers as part of > > > > + the unwinding process.""" > > > > + > > > > + class frame_id (object): > > > > + """ Basic frame id.""" > > > > + > > > > + def __init__ (self, sp, pc): > > > > + """ Create the frame id.""" > > > > + self.sp = sp > > > > + self.pc = pc > > > > + > > > > + def __init__ (self): > > > > + """Create the unwinder.""" > > > > + Unwinder.__init__ (self, "dummy stack unwinder") > > > > + self.void_ptr_t = gdb.lookup_type("void").pointer() > > > > + self.regs = None > > > > + > > > > + def get_regs (self, pending_frame): > > > > + """Return a list of register names that should be read. Only > > > > + gathers the list once, then caches the result.""" > > > > + if (self.regs != None): > > > > + return self.regs > > > > + > > > > + # Collect the names of all registers to read. > > > > + self.regs = list (pending_frame.architecture () > > > > + .register_names ()) > > > > + > > > > + return self.regs > > > > + > > > > + def __call__ (self, pending_frame): > > > > + """Actually performs the unwind, or at least sniffs this frame > > > > + to see if the unwinder should claim it, which is never does.""" > > > > + try: > > > > + for r in (self.get_regs (pending_frame)): > > > > + v = pending_frame.read_register (r).cast (self.void_ptr_t) > > > > + except: > > > > + print ("Dummy unwinder, exception") > > > > + raise > > > > + > > > > + return None > > > > + > > > > +# Register the ComRV stack unwinder. > > > > +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True) > > > > + > > > > +print ("Python script imported") > > > >