From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id QHwWGWJum1+pGAAAWB0awg (envelope-from ) for ; Thu, 29 Oct 2020 21:37:38 -0400 Received: by simark.ca (Postfix, from userid 112) id 646431EFC1; Thu, 29 Oct 2020 21:37:38 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=MAILING_LIST_MULTI, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id EB8CC1E590 for ; Thu, 29 Oct 2020 21:37:37 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 32561385780E; Fri, 30 Oct 2020 01:37:37 +0000 (GMT) Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) by sourceware.org (Postfix) with ESMTPS id 5C1C1385780E for ; Fri, 30 Oct 2020 01:37:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5C1C1385780E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wm1-f51.google.com with SMTP id w23so1489331wmi.4 for ; Thu, 29 Oct 2020 18:37:34 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:from:to:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=gPf68U1306jti6ztkNijC5IpO3cVktjCAIvZFbLwKqo=; b=cwIzDxHLUJZXxVmuFE7NQ2AZaaagvWvgsQrf8fn5KFm9vOEPcjkIKS/JmZIK4uu5t3 37ZDvP46BsVDYgDZuOBWj4n7HKk0OUZBnl9EvS44wm1t1YvFtqT4fXNuz690V8W3Elyo dIgcbgNL42d+FwwenfZX+P1yYdcbfxzIvTQx7LFzbzy30/vmHzPQzAr46FUdMsnEzD25 whAwTbfRHMYHWB7sdz2zKq0/l6iUMwEuYLQ22Ax6kvBKjZiiCj4thptxCeW/pj+gJ1pX Tu/ifN6mIsF5wO1ko3BkneC68Xj/Fri0L40vm4cnA9+sbnhykwiTHZcvkhGffb4FGZuv M3gA== X-Gm-Message-State: AOAM532OxlozqUgpgv6UBonsrTKMv9IuNtrRa2C0IthqmT8c0WyADeyB cQUikb57IJjpitLFeg0ZwQCvWfJTmCAKZg== X-Google-Smtp-Source: ABdhPJzjjnBraHcqt8ntgGL6azx3z98LBMEthVwYwLPjJFvigV4TrOip2s8LOiO9J8GdsgsivHmLXg== X-Received: by 2002:a7b:c387:: with SMTP id s7mr2116820wmj.52.1604021851887; Thu, 29 Oct 2020 18:37:31 -0700 (PDT) Received: from ?IPv6:2001:8a0:f925:3b00:b96b:919a:aa14:2400? ([2001:8a0:f925:3b00:b96b:919a:aa14:2400]) by smtp.gmail.com with ESMTPSA id e2sm9020896wrr.85.2020.10.29.18.37.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 29 Oct 2020 18:37:30 -0700 (PDT) Subject: [pushed] Move lookup_selected_frame to frame.c From: Pedro Alves To: Simon Marchi , gdb-patches@sourceware.org References: <20200708233125.1030-1-pedro@palves.net> <20200708233125.1030-4-pedro@palves.net> <58a4020f-fbbb-611f-eb23-c1b3fa25d4f2@simark.ca> <39ad8351-3f99-1162-4d2f-b5dbee5756f8@simark.ca> <47b34393-3833-bb85-84dc-9a8bde3e1a77@palves.net> <3293dbc2-922a-8589-dbc7-75ebd5a26175@simark.ca> Message-ID: Date: Fri, 30 Oct 2020 01:37:29 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 10/30/20 1:13 AM, Pedro Alves wrote: > On 7/10/20 3:55 AM, Simon Marchi wrote: >>>> I don't know if it would be worth it, but I'd like if we could assert (abort >>>> GDB) if an exception does try to exit the destructor. The `restore` method >>>> is non-trivial and calls into other non-trivial functions, so it would be >>>> possible for a change far far away to cause that to happen. >>> >>> It will already abort. Destructors are noexcept by default, so if an exception >>> escapes a destructor, std::terminate() is called, and that calls abort by default. >> >> Oh, didn't know that! I thought it was just "undefined behavior". >> >>>> What do you think of keeping the try/catch, but using `gdb_assert_not_reached` >>>> in it? >>> >>> Not sure. If we do that, we do get a nicer error message. However if the user >>> says "n" to "Quit this debugging session" we still abort. >>> >>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >>> A problem internal to GDB has been detected, >>> further debugging may prove unreliable. >>> Quit this debugging session? (y or n) n >>> >>> This is a bug, please report it. For instructions, see: >>> . >>> >>> /home/pedro/brno/pedro/gdb/binutils-gdb-2/build/../src/gdb/thread.c:1441: internal-error: scoped_restore_current_thread::~scoped_restore_current_thread(): unexpected exception thrown from destructor: hello >>> A problem internal to GDB has been detected, >>> further debugging may prove unreliable. >>> Create a core file of GDB? (y or n) n >>> terminate called after throwing an instance of 'gdb_exception_quit' >>> Aborted (core dumped) >>> >>> Maybe it would be interesting to add a variant of internal_error that did >>> not throw a quit, so the user could swallow the exception... Maybe consider >>> wrapping that as a generic facility to add to all non-trivial RAII destructors >>> we have? Like a function that takes a function_view as parameter, so >>> we would write: >>> >>> foo::~foo () >>> { >>> safe_dtor (__FILE__, __LINE__, [&] () >>> { >>> restore (); >>> }); >>> } >>> >>> Even better, add a SAFE_DTOR macro using similar magic SCOPE_EXIT >>> macro uses to be able to write: >>> >>> foo::~foo () >>> { >>> SAFE_DTOR { restore (); }; >>> } >> >> That's fancier than what I hoped for :). My goal was just to make sure >> we catch it if we ever make a change that causes an exception to escape. >> Although I wouldn't be against what you proposed. >> >>> Here's the current version of the patch. >> >> That looks fine to me. > > FYI, I've finally merged this to master. > I've merged this obvious follow up patch as well. >From d70bdd3cc4cfdfd30bed44a271ff80f98b96eebf Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 9 Jul 2020 20:12:15 +0100 Subject: [PATCH] Move lookup_selected_frame to frame.c This function is now external, and isn't really threads related. Move it to frame.c. gdb/ChangeLog: * thread.c (lookup_selected_frame): Move ... * frame.c (lookup_selected_frame): ... here. Change-Id: Ia96b79c15767337c68efd3358bcc715ce8e26c15 --- gdb/ChangeLog | 5 +++++ gdb/frame.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ gdb/thread.c | 63 -------------------------------------------------------- 3 files changed, 71 insertions(+), 63 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 978576ac7c..d2df843b24 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2020-10-30 Pedro Alves + + * thread.c (lookup_selected_frame): Move ... + * frame.c (lookup_selected_frame): ... here. + 2020-10-30 Pedro Alves * blockframe.c (block_innermost_frame): Use get_selected_frame. diff --git a/gdb/frame.c b/gdb/frame.c index bb835e2dba..d0a4ce4d63 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -1740,6 +1740,72 @@ restore_selected_frame (frame_id frame_id, int frame_level) selected_frame = nullptr; } +/* See frame.h. */ + +void +lookup_selected_frame (struct frame_id a_frame_id, int frame_level) +{ + struct frame_info *frame = NULL; + int count; + + /* This either means there was no selected frame, or the selected + frame was the current frame. In either case, select the current + frame. */ + if (frame_level == -1) + { + select_frame (get_current_frame ()); + return; + } + + /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we + shouldn't see it here. */ + gdb_assert (frame_level > 0); + + /* Restore by level first, check if the frame id is the same as + expected. If that fails, try restoring by frame id. If that + fails, nothing to do, just warn the user. */ + + count = frame_level; + frame = find_relative_frame (get_current_frame (), &count); + if (count == 0 + && frame != NULL + /* The frame ids must match - either both valid or both + outer_frame_id. The latter case is not failsafe, but since + it's highly unlikely the search by level finds the wrong + frame, it's 99.9(9)% of the time (for all practical purposes) + safe. */ + && frame_id_eq (get_frame_id (frame), a_frame_id)) + { + /* Cool, all is fine. */ + select_frame (frame); + return; + } + + frame = frame_find_by_id (a_frame_id); + if (frame != NULL) + { + /* Cool, refound it. */ + select_frame (frame); + return; + } + + /* Nothing else to do, the frame layout really changed. Select the + innermost stack frame. */ + select_frame (get_current_frame ()); + + /* Warn the user. */ + if (frame_level > 0 && !current_uiout->is_mi_like_p ()) + { + warning (_("Couldn't restore frame #%d in " + "current thread. Bottom (innermost) frame selected:"), + frame_level); + /* For MI, we should probably have a notification about current + frame change. But this error is not very likely, so don't + bother for now. */ + print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); + } +} + bool has_stack_frames () { diff --git a/gdb/thread.c b/gdb/thread.c index 193f9d4c44..32d14e8662 100644 --- a/gdb/thread.c +++ b/gdb/thread.c @@ -1327,69 +1327,6 @@ switch_to_thread (process_stratum_target *proc_target, ptid_t ptid) /* See frame.h. */ -void -lookup_selected_frame (struct frame_id a_frame_id, int frame_level) -{ - struct frame_info *frame = NULL; - int count; - - /* This either means there was no selected frame, or the selected - frame was the current frame. In either case, select the current - frame. */ - if (frame_level == -1) - { - select_frame (get_current_frame ()); - return; - } - - /* select_frame never saves 0 in SELECTED_FRAME_LEVEL, so we - shouldn't see it here. */ - gdb_assert (frame_level > 0); - - /* Restore by level first, check if the frame id is the same as - expected. If that fails, try restoring by frame id. If that - fails, nothing to do, just warn the user. */ - - count = frame_level; - frame = find_relative_frame (get_current_frame (), &count); - if (count == 0 - && frame != NULL - /* The frame ids must match - either both valid or both outer_frame_id. - The latter case is not failsafe, but since it's highly unlikely - the search by level finds the wrong frame, it's 99.9(9)% of - the time (for all practical purposes) safe. */ - && frame_id_eq (get_frame_id (frame), a_frame_id)) - { - /* Cool, all is fine. */ - select_frame (frame); - return; - } - - frame = frame_find_by_id (a_frame_id); - if (frame != NULL) - { - /* Cool, refound it. */ - select_frame (frame); - return; - } - - /* Nothing else to do, the frame layout really changed. Select the - innermost stack frame. */ - select_frame (get_current_frame ()); - - /* Warn the user. */ - if (frame_level > 0 && !current_uiout->is_mi_like_p ()) - { - warning (_("Couldn't restore frame #%d in " - "current thread. Bottom (innermost) frame selected:"), - frame_level); - /* For MI, we should probably have a notification about - current frame change. But this error is not very - likely, so don't bother for now. */ - print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC, 1); - } -} - void scoped_restore_current_thread::restore () { base-commit: 79952e69634bd02029e79676a38915de60052e89 -- 2.14.5