From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id C2B973858D3C for ; Tue, 8 Nov 2022 16:05:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C2B973858D3C Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=polymtl.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=polymtl.ca Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id 2A8G5BPI014821 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 8 Nov 2022 11:05:16 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2A8G5BPI014821 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1667923516; bh=JjSeScih47cMfMQt9t2k1/9CmgrOpSsQdU/69uSpbas=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Vz4v1G+Aa6YRDRvyPnhvUhhFCcedeBg0Nn5fE+yP3s638yPqGcT4ENqjr9Q9RtHE5 Mj8B83rNppD5HW8aLj0tqSK0sTqwsgpbBhmtRwNA4OaoDpyM4FdJA56DsYEywxJ/QA pSCHv7ij3qd76x6sbMKiOyS0lorKuQdAnh+dJ/EI= Received: from [172.16.0.64] (192-222-180-24.qc.cable.ebox.net [192.222.180.24]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id E80FC1E0D3; Tue, 8 Nov 2022 11:05:10 -0500 (EST) Message-ID: <71e837fa-888a-9929-9b5f-241a5cdad49e@polymtl.ca> Date: Tue, 8 Nov 2022 11:05:10 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH 4/7] gdb: remove manual frame_info reinflation code in backtrace_command_1 Content-Language: fr To: Bruno Larsen , gdb-patches@sourceware.org Cc: Simon Marchi References: <20221107155310.2590069-1-simon.marchi@polymtl.ca> <20221107155310.2590069-4-simon.marchi@polymtl.ca> <9beca9bd-ab05-e007-63ec-be615c3ea937@redhat.com> From: Simon Marchi In-Reply-To: <9beca9bd-ab05-e007-63ec-be615c3ea937@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 8 Nov 2022 16:05:11 +0000 X-Spam-Status: No, score=-3030.7 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_NUMSUBJECT,NICE_REPLY_A,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/8/22 05:14, Bruno Larsen wrote: > On 07/11/2022 16:53, Simon Marchi via Gdb-patches wrote: >> From: Simon Marchi >> >> With the following patch applied (gdb: use frame_id_p instead of >> comparing to null_frame_id in frame_info_ptr::reinflate), I would get: >> >>      $ ./gdb -q -nx --data-directory=data-directory testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame -ex "b breakpt" -ex r -ex "bt full" >>      Reading symbols from testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame... >>      Breakpoint 1 at 0x1131: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c, line 22. >>      Starting program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/bt-selected-frame/bt-selected-frame >>      [Thread debugging using libthread_db enabled] >>      Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". >> >>      Breakpoint 1, breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22 >>      22      } >>      #0  breakpt () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.base/bt-selected-frame.c:22 >>      No locals. >>      /home/smarchi/src/binutils-gdb/gdb/frame-info.c:42: internal-error: reinflate: Assertion `frame_id_p (m_cached_id)' failed. >> >> This is because the code in backtrace_command_1 to manually reinflate >> `fi` steps overs frame_info_ptr's toes. >> >> When calling >> >>      fi.prepare_reinflate (); >> >> `fi` gets properly filled with the cached frame id.  But when this >> happens: >> >>      fi = frame_find_by_id (frame_id); >> >> `fi` gets replaced by a brand new frame_info_ptr that doesn't have a >> cached frame id.  Then this is called without a cached frame id: >> >>      fi.reinflate (); >> >> That doesn't cause any problem currently, since >> >>   - the gdb_assert in the reinflate method doesn't actually do anything >>     (the following patch fixes that) >>   - `fi.m_ptr` will always be non-nullptr, since we just got it from >>     frame_find_by_id, so reinflate will not do anything, it won't try to >>     use m_cached_id >> >> Fix that by removing the code to manually re-fetch the frame.  That >> should be taken care of by frame_info_ptr::reinflate. >> >> Note that the old code checked if we successfully re-inflated the frame >> or not, and if not it did emit a warning.  The equivalent in >> frame_info_ptr::reinflate asserts that the frame has been successfully >> re-inflated.  It's not clear if / when this can happen, but if it can >> happen, we'll need to find a solution to this problem globally >> (everywhere a frame_info_ptr can be re-inflated), not just here.  So I >> propose to leave it like this, until it does become a problem. >> > I think this patch could be squashed with the one you mention at the top of the commit message. I think they are both related to the same thing (fixing the assert and dealing with fallout) and I don't see much point in separating how you did here. I think it would be fine to have them merged or have them separate, but at this point I'd prefer to keep it like this, since it's done. Simon