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 AFFCB3833A03 for ; Wed, 7 Dec 2022 16:52:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AFFCB3833A03 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 2B7GqDZU016540 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Wed, 7 Dec 2022 11:52:18 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2B7GqDZU016540 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1670431939; bh=WDZHdD3rs1aEOwwsFLF1bLZ+NsEVcnNDWMpkbLeL7sI=; h=Date:Subject:To:References:From:In-Reply-To:From; b=DUMtI8ZF/HfeaCMXoFNHisfOFl9Xtq2I3AuRLD4G8+gkuhgxqDqXE0kq0HzEEVcKr hBOd8ahjHFyqhQ9qr76A89KVH/a6VXVizBzbv3ECPHvnOvAXWXl6qD2XqbtnLYEKK6 cwOPB6sRpEzBi3Dr/fl7nBVrdwWeehOh8DDsM/Kw= 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 7F7D71E112; Wed, 7 Dec 2022 11:52:13 -0500 (EST) Message-ID: Date: Wed, 7 Dec 2022 11:52:12 -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/6] gdb: revert frame_unwind::this_id and callees to use `frame_info *` Content-Language: fr To: Tom Tromey , Simon Marchi via Gdb-patches References: <20221202180052.212745-1-simon.marchi@polymtl.ca> <20221202180052.212745-5-simon.marchi@polymtl.ca> <87cz8xjzp5.fsf@tromey.com> From: Simon Marchi In-Reply-To: <87cz8xjzp5.fsf@tromey.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 7 Dec 2022 16:52:13 +0000 X-Spam-Status: No, score=-3032.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham 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 12/5/22 16:41, Tom Tromey wrote: >>>>>> "Simon" == Simon Marchi via Gdb-patches writes: > > Simon> So, this patch started out as changing the prototype of > Simon> frame_unwind::this_id and adjusting callees recursively until the code > Simon> compiled again. > > Simon> The question is: doesn't this defeat the purpose of frame_info_ptr? I > Simon> think it depends on what we decide the purpose of frame_info_ptr is. > > Simon> 1. catch accesses to stale frame_info ptr (and assert if so) > Simon> 2. reinflate frame_info objects automatically. > > Simon> Right now, it does #1. My goal is to make it to #2. I think that is > Simon> more in line with the original "reinflatable frames" idea, and in > Simon> general more useful. > > To be clear, my goal in doing this was to try to eliminate the bug where > a frame_info pointer is held across a frame cache reinit. From my > perspective, both of these approaches satisfy that goal, so the idea of > this change seems completely fine. Ok, thanks. Like I said, it's not *impossible* for such a bug to creep up again in a function that is converted back to use `frame_info *`, just not likely. > Simon> And since those frames whose id is being computed > Simon> are by definition not reinflatable (you need to id to reinflate), it's > Simon> not really useful to have frame_info_ptr on code paths that deal with > Simon> frames whose id is currently being computed. Reinflation only really > Simon> makes sense for the "users" of the frame API, those who receive a frame > Simon> with a computed id. And by experience, this is where the "using stale > Simon> frame_info" bugs happen, a user holding a frame_info across a target > Simon> resumption. > > Basically I looked into this and it seemed difficult, so I gave up. > And, a simpler approach didn't work. But, you managed it :-) So kudos! > > I think a good long run goal would be if frame_id and frame_info_ptr > were generally merged. That is, the "using" code should only ever need > to touch a frame_info_ptr. I don't know if this is really feasible, but > it would be another step toward making the API simpler to use and less > buggy. Interesting idea. I think that a lot of places that save a frame_id (and sometimes frame level) for later use could indeed be converted to use frame_info_ptr. These places don't save a `frame_info *`, because they know the frame_info object will get deleted along the way. But with a frame_info_ptr, it will just get reinflated transparently. After all, frame_info_ptr is exactly that, a frame_id and a frame level. Plus a `frame_info *` that "caches" the frame_info object. So, a tiny bit of overhead compared to just frame_id + frame level, but much less change of being mis-used. > > Simon> Now, we could have two separate class frame_info wrapper classes, one > Simon> that is today's frame_info_ptr, whose goal is just to catch uses of > Simon> stale frame_info objects, and one that is the one at the end of this > Simon> series, whose goal is to do automatic reinflation. > > I would say the default choice should be a single class, unless for some > reason it's too expensive. I initially thought about that, a single class that could be in different states - frame id know and frame id unknown, but dropped it becuase of the extra complexity. A frame_info_ptr created for a `frame_info *` before the id is known will not save a frame_id. What happens once the frame_id of the frame_info becomes known? If you copy construct a frame_info_ptr from that original frame_info_ptr that doesn't know the frame_id, should the new frame_info_ptr fetch the frame_id from the frame_info? I didn't want to solve these issues. But in reality, that transition really happens around the this_id call inside compute_frame_id, so we can have some special handling there. The frame_info_ptr coming it could have no know frame id, and it could have one when going out. The only downside I see with this approach is that in a given random function using frame_info_ptr, you can't easily know statically if the frame_info_ptr is guaranteed to be reinflatable or not. In other words, if you are on the "frame id computation" path. Maybe we can just hit an assert when trying to reinflate a non-reinflatable frame_info_ptr (one that doesn't know the frame id). If that would happen, it means we had a frame cache reinit while computing a frame id, which is the case I told should never happen. And so that might help us catch it, if it was to happen. I think you've convinced me to go back to use frame_info_ptr and have these two states :). Simon