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 EF1F0384DDB5 for ; Mon, 12 Dec 2022 13:26:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF1F0384DDB5 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 2BCDQDw6014812 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 12 Dec 2022 08:26:18 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 2BCDQDw6014812 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1670851578; bh=LQwDYdaL6ffedm8PD5XDwwfiCc1aA25civpfqmFZzcI=; h=Date:Subject:To:References:From:In-Reply-To:From; b=Y1MmHdXg2NquAScCEqdLNcN1Cpew/UB7bX40UU/wBt6e3yUCt3rdYaOPFj8Oe3PQo mCFC59hsGjNlLjIIE6Q8Wy9w8r/dlEu4uBjQ55z/ewebjGpzs7BcZTMHczJe3I0+WJ XQg0jNRrst0twvhgTvtN7+h5RkmFm9HJN5+4x674= Received: from [10.0.0.11] (unknown [217.28.27.60]) (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 B270F1E0CB; Mon, 12 Dec 2022 08:26:13 -0500 (EST) Message-ID: Date: Mon, 12 Dec 2022 08:26:13 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH 4/6] gdb: revert frame_unwind::this_id and callees to use `frame_info *` Content-Language: en-US To: Bruno Larsen , gdb-patches@sourceware.org References: <20221202180052.212745-1-simon.marchi@polymtl.ca> <20221202180052.212745-5-simon.marchi@polymtl.ca> <3a0c4952-e44e-bed8-518a-52156a16747b@redhat.com> From: Simon Marchi In-Reply-To: <3a0c4952-e44e-bed8-518a-52156a16747b@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 12 Dec 2022 13:26:13 +0000 X-Spam-Status: No, score=-3032.2 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/12/22 08:17, Bruno Larsen wrote: > On 02/12/2022 19:00, Simon Marchi via Gdb-patches wrote: >> A following patch will change frame_info_ptr to grab the frame id and >> reinflate automatically. This makes it so frame_info_ptr will no longer >> be able to wrap frame_info objects whose ids are being computed. This >> means that the frame_unwind::this_id method can't take a frame_info_ptr >> anymore, since that's the entry point of computing frame ids. And then, >> everything that "this_id" implementations can possibly call won't be >> able to accept a frame_info_ptr anymore. >> >> So, this patch started out as changing the prototype of >> frame_unwind::this_id and adjusting callees recursively until the code >> compiled again. >> >> The question is: doesn't this defeat the purpose of frame_info_ptr? I >> think it depends on what we decide the purpose of frame_info_ptr is. >> >> 1. catch accesses to stale frame_info ptr (and assert if so) >> 2. reinflate frame_info objects automatically. > I agree with Tom's point, both points fix the bugs that we tackled when starting this, and I personally like the second option better. >> >> Right now, it does #1. My goal is to make it to #2. I think that is >> more in line with the original "reinflatable frames" idea, and in >> general more useful. And since those frames whose id is being computed >> are by definition not reinflatable (you need to id to reinflate), it's >> not really useful to have frame_info_ptr on code paths that deal with >> frames whose id is currently being computed. Reinflation only really >> makes sense for the "users" of the frame API, those who receive a frame >> with a computed id. And by experience, this is where the "using stale >> frame_info" bugs happen, a user holding a frame_info across a target >> resumption. If a frame cache reinit was to happen in the middle of a >> frame id computation, I think things would be seriously wrong. After >> the reinit, we'd be computing an id for a frame_info that doesn't exist >> anymore, I don't think it make sense. >> >> There are many functions that can be used in both contexts (frames with >> ids computed, and frames with ids being computed). value_of_register, >> for instance. These functions must be changed back to take a raw >> frame_info pointer. But to make things convenient to callers holding a >> frame_info_ptr, and to avoid having to change too many call sites, I >> introduced wrappers like this: >> >> static inline value * >> value_of_register (int regnum, frame_info_ptr frame) >> { >> return value_of_register (regnum, frame.get ()); >> } >> >> Since frame_info_ptr::get will, at the end of this series, automatically >> reinflate the wrapped frame_info object, if necessary, the caller still >> gets the "protection" offered by frame_info_ptr, at least up to that >> point. Now, if a frame cache reinit was to happen inside that >> get_prev_frame call, we'd be screwed. But I think the risk of it >> happening is pretty low, for this kind of functions. And remember, >> since get_prev_frame is also used in the context of frame id >> computation, it's hard to imagine that it could happen. > > s/get_prev_frame/value_of_register ? or am I missing something? > > Regardless, your logic seems solid. I feel like a wrapper like this is better than a second class, unless we start seeing the issues you theorized. > >> >> Now, we could have two separate class frame_info wrapper classes, one >> that is today's frame_info_ptr, whose goal is just to catch uses of >> stale frame_info objects, and one that is the one at the end of this >> series, whose goal is to do automatic reinflation. The former one could >> continue being used in the paths involved in frame id computation. >> However, I'm not convinced it is worth it, due to (1) the low risk of >> this problem happening in these paths and (2) the availability of tools >> like ASan or Valgrind that will tell you precisely when a use-after-free >> happens. >> >> There are no user-visible changes expected with this patch. I >> built-tested and regression-tested on Linux x86-64. I don't think any >> platform-specific nat file needs to be changed, as they don't deal with >> frames. > > For the patch itself, I took a look at it and saw nothing wrong, but I'm not confident I understand the code well enough to say it is ok. This is all I got: > > Tested-By: Bruno Larsen Hi Bruno, This patch will likely be dropped from the next version (see the thread with Tom). Simon