From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by sourceware.org (Postfix) with ESMTPS id A1C7B385DC1C for ; Sat, 25 Apr 2020 15:53:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A1C7B385DC1C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tdevries@suse.de X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id DD4E0AC11; Sat, 25 Apr 2020 15:53:15 +0000 (UTC) Subject: Re: [PATCH] Fix remaining inline/tailcall unwinding breakage for x86_64 To: Luis Machado , gdb-patches@sourceware.org Cc: tromey@adacore.com, andrew.burgess@embecosm.com References: <20200425040934.17011-1-luis.machado@linaro.org> From: Tom de Vries Autocrypt: addr=tdevries@suse.de; keydata= xsBNBF0ltCcBCADDhsUnMMdEXiHFfqJdXeRvgqSEUxLCy/pHek88ALuFnPTICTwkf4g7uSR7 HvOFUoUyu8oP5mNb4VZHy3Xy8KRZGaQuaOHNhZAT1xaVo6kxjswUi3vYgGJhFMiLuIHdApoc u5f7UbV+egYVxmkvVLSqsVD4pUgHeSoAcIlm3blZ1sDKviJCwaHxDQkVmSsGXImaAU+ViJ5l CwkvyiiIifWD2SoOuFexZyZ7RUddLosgsO0npVUYbl6dEMq2a5ijGF6/rBs1m3nAoIgpXk6P TCKlSWVW6OCneTaKM5C387972qREtiArTakRQIpvDJuiR2soGfdeJ6igGA1FZjU+IsM5ABEB AAHNH1RvbSBkZSBWcmllcyA8dGRldnJpZXNAc3VzZS5kZT7CwKsEEwEIAD4WIQSsnSe5hKbL MK1mGmjuhV2rbOJEoAUCXSW0JwIbAwUJA8JnAAULCQgHAgYVCgkICwIEFgIDAQIeAQIXgAAh CRDuhV2rbOJEoBYhBKydJ7mEpsswrWYaaO6FXats4kSgc48H/Ra2lq5p3dHsrlQLqM7N68Fo eRDf3PMevXyMlrCYDGLVncQwMw3O/AkousktXKQ42DPJh65zoXB22yUt8m0g12xkLax98KFJ 5NyUloa6HflLl+wQL/uZjIdNUQaHQLw3HKwRMVi4l0/Jh/TygYG1Dtm8I4o708JS4y8GQxoQ UL0z1OM9hyM3gI2WVTTyprsBHy2EjMOu/2Xpod95pF8f90zBLajy6qXEnxlcsqreMaqmkzKn 3KTZpWRxNAS/IH3FbGQ+3RpWkNGSJpwfEMVCeyK5a1n7yt1podd1ajY5mA1jcaUmGppqx827 8TqyteNe1B/pbiUt2L/WhnTgW1NC1QDOwE0EXSW0JwEIAM99H34Bu4MKM7HDJVt864MXbx7B 1M93wVlpJ7Uq+XDFD0A0hIal028j+h6jA6bhzWto4RUfDl/9mn1StngNVFovvwtfzbamp6+W pKHZm9X5YvlIwCx131kTxCNDcF+/adRW4n8CU3pZWYmNVqhMUiPLxElA6QhXTtVBh1RkjCZQ Kmbd1szvcOfaD8s+tJABJzNZsmO2hVuFwkDrRN8Jgrh92a+yHQPd9+RybW2l7sJv26nkUH5Z 5s84P6894ebgimcprJdAkjJTgprl1nhgvptU5M9Uv85Pferoh2groQEAtRPlCGrZ2/2qVNe9 XJfSYbiyedvApWcJs5DOByTaKkcAEQEAAcLAkwQYAQgAJhYhBKydJ7mEpsswrWYaaO6FXats 4kSgBQJdJbQnAhsMBQkDwmcAACEJEO6FXats4kSgFiEErJ0nuYSmyzCtZhpo7oVdq2ziRKD3 twf7BAQBZ8TqR812zKAD7biOnWIJ0McV72PFBxmLIHp24UVe0ZogtYMxSWKLg3csh0yLVwc7 H3vldzJ9AoK3Qxp0Q6K/rDOeUy3HMqewQGcqrsRRh0NXDIQk5CgSrZslPe47qIbe3O7ik/MC q31FNIAQJPmKXX25B115MMzkSKlv4udfx7KdyxHrTSkwWZArLQiEZj5KG4cCKhIoMygPTA3U yGaIvI/BGOtHZ7bEBVUCFDFfOWJ26IOCoPnSVUvKPEOH9dv+sNy7jyBsP5QxeTqwxC/1ZtNS DUCSFQjqA6bEGwM22dP8OUY6SC94x1G81A9/xbtm9LQxKm0EiDH8KBMLfQ== Message-ID: Date: Sat, 25 Apr 2020 17:53:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0 MIME-Version: 1.0 In-Reply-To: <20200425040934.17011-1-luis.machado@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-28.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, KAM_NUMSUBJECT, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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: Sat, 25 Apr 2020 15:53:19 -0000 On 25-04-2020 06:09, Luis Machado wrote: > Commit 5939967b355ba6a940887d19847b7893a4506067 fixed inline > frame unwinding breakage for some targets (aarch64, riscv, s390...) > but regressed a few amd64 testcases related to tailcalls. > > Given the following example situation... > > Frame #-1 - sentinel frame > Frame # 0 - inline frame > Frame # 1 - normal frame > > ... suppose we're at level #1 and call into dwarf2_tailcall_sniffer_first. > > We'll attempt to fetch PC, which used to be done via the gdbarch_unwind_pc call > (before 5939967b355ba6a940887d19847b7893a4506067), but now it is being handled > by the get_frame_register function. > > gdbarch_unwind_pc will attempt to use frame #1's cache to retrieve information > about the PC. Here's where different architectures behave differently. > > x86_64 will find a dwarf rule to retrieve PC from memory, at a CFA + offset > location. So the PC value is readily available and there is no need to > create a lazy value. > > For aarch64 (and others), GCC doesn't emit an explicit location for PC, so we > eventually will find that PC is DWARF2_FRAME_REG_UNSPECIFIED. This is known > and is handled by GDB by assuming GCC really meant DWARF2_FRAME_REG_SAME_VALUE. > > This means we'll attempt to fetch the register value from frame #0, via a call > to frame_unwind_got_register, which will trigger the creation of a lazy value > that requires a valid frame id for frame #0. > > We don't have a valid id for frame #0 yet, so we assert. > > Given the above, the following patch attempts to handle the situation without > being too hacky. We verify if the next frame is an inline frame and if its > frame id has been computed already. If it hasn't been computed yet, then we > use the safer get_frame_register function, otherwise we use the regular > gdbarch_unwind_pc hook. > > I've verified this makes both aarch64-linux and x86_64 happy testsuite-wise. > Hi Luis, thanks for working on this. I've tested this patch on x86_64-linux and can confirm that this fixes all the regressions I saw. I've reviewed the patch and it looks ok to me. Please check this in, given that if fixes regressions. If there are any comments from others to be addressed, that can still be done post-commit. Thanks, - Tom > gdb/ChangeLog: > > 2020-04-25 Luis Machado > > * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Handle > problematic inline frame unwinding situation. > * frame.c (frame_id_computed_p): New function. > * frame.h (frame_id_computed_p): New prototype. > --- > gdb/dwarf2/frame-tailcall.c | 39 ++++++++++++++++++++++++++++++++++--- > gdb/frame.c | 8 ++++++++ > gdb/frame.h | 4 ++++ > 3 files changed, 48 insertions(+), 3 deletions(-) > > diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c > index 01bb134a5c..16dba2b201 100644 > --- a/gdb/dwarf2/frame-tailcall.c > +++ b/gdb/dwarf2/frame-tailcall.c > @@ -384,10 +384,43 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame, > > prev_gdbarch = frame_unwind_arch (this_frame); > > + /* The dwarf2 tailcall sniffer runs early, at the end of populating the > + dwarf2 frame cache for the current frame. If there exists inline > + frames inner (next) to the current frame, there is a good possibility > + of that inline frame not having a computed frame id yet. > + > + This is because computing such a frame id requires us to walk through > + the frame chain until we find the first normal frame after the inline > + frame and then compute the normal frame's id first. > + > + Some architectures' compilers generate enough register location > + information for a dwarf unwinder to fetch PC without relying on inner > + frames (x86_64 for example). In this case the PC is retrieved > + according to dwarf rules. > + > + But others generate less strict dwarf data for which assumptions are > + made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as > + DWARF2_FRAME_REG_SAME_VALUE). For such cases, GDB may attempt to > + create lazy values for registers, and those lazy values must be > + created with a valid frame id, but we potentially have no valid id. > + > + So, to avoid breakage, if we see a dangerous situation with inline > + frames without a computed id, use safer functions to retrieve the > + current frame's PC. Otherwise use the provided dwarf rules. */ > + frame_info *next_frame = get_next_frame (this_frame); > + > /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p. */ > - get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch), > - (gdb_byte *) &prev_pc); > - prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc); > + if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME > + && !frame_id_computed_p (next_frame)) > + { > + /* The next frame is an inline frame and its frame id has not been > + computed yet. */ > + get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch), > + (gdb_byte *) &prev_pc); > + prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc); > + } > + else > + prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame); > > /* call_site_find_chain can throw an exception. */ > chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc); > diff --git a/gdb/frame.c b/gdb/frame.c > index ac1016b083..ff27b9f00e 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -687,6 +687,14 @@ frame_id_build_wild (CORE_ADDR stack_addr) > return id; > } > > +bool > +frame_id_computed_p (struct frame_info *frame) > +{ > + gdb_assert (frame != nullptr); > + > + return frame->this_id.p != 0; > +} > + > int > frame_id_p (struct frame_id l) > { > diff --git a/gdb/frame.h b/gdb/frame.h > index cfc15022ed..e835d49f9c 100644 > --- a/gdb/frame.h > +++ b/gdb/frame.h > @@ -236,6 +236,10 @@ extern struct frame_id > as the special identifier address are set to indicate wild cards. */ > extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr); > > +/* Returns true if FRAME's id has been computed. > + Returns false otherwise. */ > +extern bool frame_id_computed_p (struct frame_info *frame); > + > /* Returns non-zero when L is a valid frame (a valid frame has a > non-zero .base). The outermost frame is valid even without an > ID. */ >