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 1363E385841F for ; Sat, 1 Oct 2022 20:58:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1363E385841F Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-387-mjoTr6RrNPWcLU9KE65zyQ-1; Sat, 01 Oct 2022 16:58:46 -0400 X-MC-Unique: mjoTr6RrNPWcLU9KE65zyQ-1 Received: by mail-wm1-f72.google.com with SMTP id l15-20020a05600c4f0f00b003b4bec80edbso3996632wmq.9 for ; Sat, 01 Oct 2022 13:58:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date; bh=wqicHf9V4JuDaNjejtXrcxzlKA+P9YXbcP3Z63pHTz4=; b=DLYP9Tj7hNWfMVLCAfX0bQk76FRlYelKVQWgB49cpc18y92Lp7AP9xXQruj0sMd4nU n9iwgZryL+e3PW9emw501UPxDGxO8Pcwp5zP1qaEKoYPXIQ/RmjZZ/wollRaZAfZpufT zpRZw6eT5k3LzfaIr2PhILQiPBxZ9h6pVHS2LlxQRunXPzTM/MgvO/RBUkHhS4QXREVj mN3FzsDF6Csiih5VAcMcbA0s1NOYFXXK7G4ZoHpqUxe5fNnVCRZRQLbjcdov2y9OvlL+ 5Vse6n6D7yaufVYzGAV+BT1V6Lp42v07YIhDDJbPlwQue4G5nf0Ij/ux509WFLaVVmdB ZZjg== X-Gm-Message-State: ACrzQf2qVs9tY3cS3uDXVivhnDQoQoWb7ANG4NnbBofbSP2G7WWmVqBf h+FeAzs2wcO2gBOD8f4U1G2GME7SSijtf4CUcx5eAauYqN1cz0fb0lNkjRLmaoZ4aquV2vBRhOp cFbvJ7PsctnFgzt4rxSj4WA== X-Received: by 2002:a05:600c:1e87:b0:3b5:1e2:3c3c with SMTP id be7-20020a05600c1e8700b003b501e23c3cmr2550438wmb.130.1664657925127; Sat, 01 Oct 2022 13:58:45 -0700 (PDT) X-Google-Smtp-Source: AMsMyM5qCQ2W/x1VuDZuHfTbfgRdbhu/23oTwf0MZ5iBaAhCzjXAl4h+Xq1EaP0M8GB2l/gWd5/3Bw== X-Received: by 2002:a05:600c:1e87:b0:3b5:1e2:3c3c with SMTP id be7-20020a05600c1e8700b003b501e23c3cmr2550434wmb.130.1664657924874; Sat, 01 Oct 2022 13:58:44 -0700 (PDT) Received: from localhost (52.72.115.87.dyn.plus.net. [87.115.72.52]) by smtp.gmail.com with ESMTPSA id m3-20020a056000008300b0022afcc11f65sm5200196wrx.47.2022.10.01.13.58.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 01 Oct 2022 13:58:44 -0700 (PDT) From: Andrew Burgess To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] gdb: fix target_ops reference count for some cases In-Reply-To: <1ff3e5de-f218-4657-0798-525158ba4cbe@simark.ca> References: <20220921131200.3983844-1-aburgess@redhat.com> <9a31d60a-01a8-e527-71d9-0f5908be2961@simark.ca> <87a66re30o.fsf@redhat.com> <1ff3e5de-f218-4657-0798-525158ba4cbe@simark.ca> Date: Sat, 01 Oct 2022 21:58:43 +0100 Message-ID: <87y1tzgt18.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no 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: Sat, 01 Oct 2022 20:58:49 -0000 Simon Marchi writes: >>> Just wondering, why do we need to restore explicitly the current >>> pspace, instead of using just scoped_restore_current_thread? >>> >>> scoped_restore_current_pspace_and_thread's doc says: >>> >>> /* Save/restore the current program space, thread, inferior and frame. >>> Use this when you need to call >>> switch_to_program_space_and_thread. */ >>> >>> ... but you are not using switch_to_program_space_and_thread here. >>> Maybe it's ok and I just don't understand. Same in >>> ~scoped_mock_context. >> >> I suspect the comment you quote is just out of date. >> >> switch_to_program_space_and_thread can end up calling >> switch_to_inferior_no_thread if there are no running threads in the >> program space being switched too. But, even if switch_to_thread does >> end up being called we: >> >> - set the program space, >> - set the inferior, >> - set the current thread, >> - reinit the frame cache, >> >> By comparison, switch_to_inferior_no_thread does: >> >> - sets the program space, >> - sets the inferior, >> - sets the current thread (to nullptr this time though), >> - reinits the frame cache, >> >> As you can see they do the same set of things, all of which I think >> should be reverted once we leave the scope, hence >> scoped_restore_current_pspace_and_thread seems like the way to go. > > Hmm okay, but won't scoped_restore_current_thread always restore the > pspace one way or another? scoped_restore_current_thread::restore will > either call switch_to_thread or switch_to_inferior_no_thread, which both > end up setting the pspace. That sounds completely correct. So I did an experiment. I added ~scoped_restore_current_pspace_and_thread, and with a little hacking, had it check: is the program_space in m_restore_pspace the same program_space as would be restored by m_restore_thread? Turns out that it _isn't_ always the case! Just to be clear on what this means: sometimes GDB is in a state where the currently selected program_space is NOT the program_space for the currently selected inferior! One example of when this occurs is create_longjmp_master_breakpoint in breakpoint.c, in here we do this: scoped_restore_current_program_space restore_pspace; for (struct program_space *pspace : program_spaces) { set_current_program_space (pspace); ... } Obviously we enter this code with some inferior selected, but, within the body of this loop we select each program_space in turn. All but one of these will be the wrong program_space for the current inferior. This feels (to me) like a problem waiting to happen. I wonder if we should have some better approach here. Some ideas would be: 1. Change to always require switch_to_program_space_and_thread and remove set_current_program_space. This would then require that we use scoped_restore_current_thread and remove scoped_restore_current_program_space. This would mean GDB always had a sane thread/inferior/program_space combo selected, 2. Allow GDB to have no inferior selected, then, in the above code, where (I guess) we are claiming that everything in the for loop only cares about the program_space, and not the inferior, we could temporarily switch to no inferior. This would suggest adding a set_current_program_space_no_inferior function, and again, we would use scoped_restore_current_thread and remove scoped_restore_current_program_space from GDB, 3. Embrace the mismatch and have scoped_restore_current_thread capture the current_program_space as well as the current thread and inferior. It would then restore the program space after restoring the thread/inferior, thus ensuring that we always restore the program_space correctly. This would allow for scoped_restore_current_pspace_and_thread to be removed and just use scoped_restore_current_thread in all cases. I rather like #2, but I worry that too much of GDB would just assume there's always a current inferior, so I'm tempted to think #1 is probably the best approach. My concern is mostly the performance impact of searching for a suitable inferior for a given program_space (this currently involves a linear search through all inferiors). With a low number of inferiors this shouldn't be an issue, but maybe if we started to deal with large numbers of inferiors we'd need to be smarter? Ideally I think we shouldn't just live with the mismatch, though as a short term stop-gap, maybe we could implement #3? Thoughts? Thanks, Andrew > I just don't understand why > scoped_restore_current_pspace_and_thread exists, it seems like > scoped_restore_current_thread would always work where > scoped_restore_current_pspace_and_thread is used. I must be missing > something, scoped_restore_current_pspace_and_thread must have been added > for a reason. > > Simon