From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 11BFD3851AA5 for ; Mon, 20 Jun 2022 13:12:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 11BFD3851AA5 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-663-EqV-a0gdPfOx-A1Bv-vJxQ-1; Mon, 20 Jun 2022 09:12:35 -0400 X-MC-Unique: EqV-a0gdPfOx-A1Bv-vJxQ-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 581181C06ED7; Mon, 20 Jun 2022 13:12:35 +0000 (UTC) Received: from [10.97.116.35] (ovpn-116-35.gru2.redhat.com [10.97.116.35]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9A23F1121314; Mon, 20 Jun 2022 13:12:34 +0000 (UTC) Message-ID: <3ff0c39a-ed56-2f58-d81a-9af7b9252bae@redhat.com> Date: Mon, 20 Jun 2022 10:12:31 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH v2] gdb/stack.c: avoid stale pointers when printing frame arguments To: Tom Tromey , Bruno Larsen via Gdb-patches References: <20220504191036.143360-1-blarsen@redhat.com> <87letyx78l.fsf@tromey.com> From: Bruno Larsen In-Reply-To: <87letyx78l.fsf@tromey.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Mon, 20 Jun 2022 13:12:40 -0000 On 6/15/22 12:43, Tom Tromey wrote: >>>>>> "Bruno" == Bruno Larsen via Gdb-patches writes: > > Bruno> @@ -738,8 +738,17 @@ call_function_by_hand (struct value *function, > Bruno> type *default_return_type, > Bruno> gdb::array_view args) > Bruno> { > Bruno> - return call_function_by_hand_dummy (function, default_return_type, > Bruno> - args, NULL, NULL); > Bruno> + scoped_restore_selected_frame restore_frame; > Bruno> + struct value *ret = call_function_by_hand_dummy (function, > Bruno> + default_return_type, > Bruno> + args, NULL, NULL); > Bruno> + /* Ensure that the frame cache does not contain any frames that were > Bruno> + specific to the handmade function call. If something is leftover > Bruno> + while GDB has kept a reference to a frame (for instance, when a > Bruno> + stacktrace is being printed and a pretty printed called an inferior > Bruno> + function), GDB could crash. */ > Bruno> + reinit_frame_cache (); > > I don't understand the need for this call. > It seems like it must be redundant, because the frame cache should be > reset by causing the inferior to run during the inferior call. Oh yes, it must be from an earlier iteration (or me just not understanding how frames work). All the changes to the function will be reverted locally, as the scoped_restore was here because of the re-initialization of the frame cache. > > Bruno> + struct frame_id id = get_frame_id (get_selected_frame (NULL)); > Bruno> if (execution_direction == EXEC_REVERSE) > Bruno> gdb_printf (_("Run back to call of ")); > Bruno> else > Bruno> @@ -1866,6 +1867,10 @@ finish_command (const char *arg, int from_tty) > Bruno> } > > Bruno> print_stack_frame (get_selected_frame (NULL), 1, LOCATION, 0); > Bruno> + frame = frame_find_by_id (id); > Bruno> + if (frame == nullptr) > Bruno> + error (_("frames disappeared mid-printing.")); > Bruno> + frame = get_prev_frame (frame); > > This code probably needs a comment since it's non-obvious why 'finish' > would need to do the frame-id thing. I'll add a comment too, but as an explanation to you: Everywhere that prints a frame will need this because if a frame is printed, it may call a function by hand, which makes this pointer stale. > > I think error text normally does not end in ".". > I see a lot of these though, I wonder if we ought to remove them all. A very rough 'grep error.*\.\" ' showed 32 lines with this. Since I'm already making a new version, I can change this on my patch, no problem > > Bruno> + error (_("frames disappeared mid-printing.")); > > Here too. > > Bruno> + /* print_frame can invalidate frame, so cache the frame_id to rebuild > Bruno> + the whole stack later, if needed. */ > Bruno> + struct frame_id frame_id = get_frame_id (frame); > > This repeated code makes me wonder if perhaps the print_frame_info API > ought to be changed instead. print_frame_info could return an updated pointer to the frame being printed, or receive a frame_info** and update it in-place. However, We'll still need to check for a null pointer after calling, since we won't want print_frame_info to error out if a frame no longer exists. We might not care when printing. Does any of these options sound like a better option? Or would we always want to error out if the frame disappears from under us? Cheers! Bruno Larsen > > Tom >