From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by sourceware.org (Postfix) with ESMTPS id EE4B63846078 for ; Thu, 9 Jul 2020 12:10:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EE4B63846078 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-wr1-f68.google.com with SMTP id q5so2088395wru.6 for ; Thu, 09 Jul 2020 05:10:03 -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=TPSv7AfltzqE+MJAFH7TI8TWbvCMmgAikmT/1cCObLM=; b=CvsKlly9UOiPntJh4vPL4BE0vXzrOwBK543AvLy+j4OlSsR0asqlglt7QkM+/N0V5a ye+hOaWJUuxFYoL/shrnGeKwEJcmYZbTJoHt7LZQ08SA7tYNx1M7WSTsnJNtjEntjbPU bnzW5qp+ta0ohB6i9I6efrBwlpSZLTC8XedHiM9wfVAc3uh4XNL/E1idnhmC3Z5bLx6r 29wzYZLlXH+9IiExc0XDKGW/XQafFmcW75Y/xfCUXDnYqIJjyZ2jRDp3DEQtbr/5UpoY t62jmSVKYuqHfdhhC8w1rKJDIeLVsdGBh1K4gHJ5LvJgNvjkY1NR7HG4v+bE7vD825CE HqxA== X-Gm-Message-State: AOAM533YvGc8gb/btTYhAx05u3zjBwdz8WSGRo9BQ4iXscqAD8cSIpYg msIr7jqg6K47dCyw08GOeY+wWxKQVCI= X-Google-Smtp-Source: ABdhPJzvNUuI1CQVOUg+eH4QQDCiX3UkE/BeMpOCkSvaYmmqXUl0DjAnhiAPydJeXvSCDsOoXTRnBA== X-Received: by 2002:adf:dd8d:: with SMTP id x13mr61887482wrl.362.1594296602514; Thu, 09 Jul 2020 05:10:02 -0700 (PDT) Received: from ?IPv6:2001:8a0:f91a:c400:56ee:75ff:fe8d:232b? ([2001:8a0:f91a:c400:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id v5sm5367904wre.87.2020.07.09.05.10.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Jul 2020 05:10:01 -0700 (PDT) Subject: Re: [PATCH 3/3] Make scoped_restore_current_thread's cdtors exception free (RFC) 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> Message-ID: Date: Thu, 9 Jul 2020 13:09:58 +0100 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-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 09 Jul 2020 12:10:05 -0000 On 7/9/20 12:56 PM, Pedro Alves wrote: > On 7/9/20 4:49 AM, Simon Marchi wrote: >>> void >>> select_frame (struct frame_info *fi) >>> { >>> selected_frame = fi; >>> + selected_frame_level = frame_relative_level (fi); >>> + if (selected_frame_level == 0) >>> + { >>> + /* Treat the current frame especially -- we want to always >>> + save/restore it without warning, even if the frame ID changes >>> + (see restore_selected_frame). Also get_frame_id may access >>> + the target's registers/memory, and thus skipping get_frame_id >>> + optimizes the common case. */ >>> + selected_frame_level = -1; >>> + selected_frame_id = null_frame_id; >>> + } >>> + else >>> + selected_frame_id = get_frame_id (fi); >>> + >> >> I don't really understand this part, why don't we want to set selected_frame_level >> and selected_frame_id when the level is 0. I'm more interested by why it wouldn't >> be correct or how it would break things, rather than the optimization aspect. >> > > At first, I was recording frame 0 normally, without that special case. > But running the testsuite revealed regressions in a couple testcases: > > gdb.python/py-unwind-maint.exp > gdb.server/bkpt-other-inferior.exp > > Both are related to the get_frame_id call. Before the patch, get_frame_id > isn't called on the current frame until you try to backtrace from it. > Adding the get_frame_id call makes the gdb.python/py-unwind-maint.exp testcase > print the Python unwinder callbacks in a different order, unexpected > by the testcase. I didn't look too deeply into this one, but I suspect > it would just be a matter of adjusting the testcase's expectations. > > The gdb.server/bkpt-other-inferior.exp one though is what got me > thinking. The testcase makes sure that setting a breakpoint in a > function that doesn't exist in the remote inferior does not cause > remote protocol traffic. After the patch, without the special casing, > the testcase would fail because the get_frame_id call, coming from > > check_frame_language_change # called after every command > -> get_selected_frame > -> restore_selected_frame > -> select_frame(get_current_frame()) > -> get_frame_id > > would cause registers and memory to be read from the remote target (when > restoring the selected frame). Those accesses aren't wrong, but they > aren't the kind that the bug the testcase is looking for. Those were > about spurious/incorrect remote protocol accesses when parsing the > function's prologue. > > Neither of these cases were strictly incorrect, though they got me > thinking, and I came to the conclusion that warning when we fail to > re-find the current frame is pointless, and that avoids having > unbreak the testcases mentioned, or even redo them differently in > the gdb.server/bkpt-other-inferior.exp case. > > I've updated the comment to make it clearer with an example. > > I've also polished the patch some more. I now renamed > the current restore_selected_frame to lookup_selected_frame, > to give space to the new save_selected_frame/restore_selected_frame > pair. select_frame_lazy is now restore_selected_frame. > save_selected_frame/restore_selected_frame are now noexcept, and > their intro comments explain why. > > I declared lookup_selected_frame in frame.h already, thinking that > it's easier if I move lookup_selected_frame from thread.c to frame.c > after this is in, instead of before. > > I rewrote most of the comments. For example, I think the > selected_frame_id/selected_frame_level/selected_frame comments are now > much clearer. > > And I made scoped_restore_selected_frame save/restore the language > too. I was only doing that in scoped_restore_current_thread before. > > Let me know what you think of this version. I've pushed this, along with all the PR26199 patches to: users/palves/pr26199-busy-loop-target-events (The version pushed has a couple comment typos fixed compared to the one posted.)