From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by sourceware.org (Postfix) with ESMTPS id 9A1383850429 for ; Fri, 4 Dec 2020 01:50:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 9A1383850429 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-f41.google.com with SMTP id f190so5731600wme.1 for ; Thu, 03 Dec 2020 17:50:44 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=3Jf8lHqTnCGnq8i0dcR29z7tQksMVQ9zMXXIY+27yG0=; b=qNyWwI6hS/5Mxh35X8g7F9GpUpzYNivJsFgHVElEMln6BT3qYK4fvE5dwIj5irA69n LKGSL6oFwE84YsStpfeWnceL//JCY6Ximay1V8fHFmT13PP5J3xx9L99Ft8QTPAyLR/D UhR4V6RPcmqxZBEhLlLyKg151cbdkmD1hB13+ou8ZoX8Tg/Z6jfuIdHWfOoTD5EE26E5 9nvn53TQ5+74nr6ANEbkX2LCUVIaSlR6kfl9D3IiPK3yvb4IKoCVFmSDmhZe/sJGTLw/ /YKGeq49rQXaWnJjMonXbUvjn4a+wdJ7M2RpnXUtZIhGlxryhWqZYuXbzw2clhftn3BR 664g== X-Gm-Message-State: AOAM530JRYDYUi7VYPOvjSP4ltkqAgHz4U4YFzEMWEhs2r//77gv2r/L f+45+doS63rfArsrMqME/jo= X-Google-Smtp-Source: ABdhPJwHRff1m5BUrAcb5uNz7TJvql99zTho2YuuVtcOgA+g4kh6Jj/YLODHiYrCvgQ9Q8NSOgZY0Q== X-Received: by 2002:a7b:c208:: with SMTP id x8mr1502592wmi.179.1607046643672; Thu, 03 Dec 2020 17:50:43 -0800 (PST) Received: from ?IPv6:2001:8a0:f91f:e900:1d90:d745:3c32:c159? ([2001:8a0:f91f:e900:1d90:d745:3c32:c159]) by smtp.gmail.com with ESMTPSA id j6sm1479616wrq.38.2020.12.03.17.50.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Dec 2020 17:50:42 -0800 (PST) Subject: Re: [PATCH v2 11/14] gdb: move displaced stepping logic to gdbarch, allow starting concurrent displaced steps To: Simon Marchi , gdb-patches@sourceware.org References: <20201202154805.1484317-1-simon.marchi@polymtl.ca> <20201202154805.1484317-12-simon.marchi@polymtl.ca> Cc: Simon Marchi From: Pedro Alves Message-ID: Date: Fri, 4 Dec 2020 01:50:41 +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: <20201202154805.1484317-12-simon.marchi@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3.3 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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: Fri, 04 Dec 2020 01:50:47 -0000 Very good! Only a few minor comments below. Otherwise OK. On 12/2/20 3:48 PM, Simon Marchi via Gdb-patches wrote: > +displaced_step_finish_status > +displaced_step_buffer::finish (gdbarch *arch, thread_info *thread, > + gdb_signal sig) > +{ > + gdb_assert (thread->displaced_step_state.in_progress ()); > + gdb_assert (thread == m_current_thread); > + > + /* Move this to a local variable so it's released in case something goes > + wrong. */ > + displaced_step_copy_insn_closure_up copy_insn_closure > + = std::move (m_copy_insn_closure); > + gdb_assert (copy_insn_closure != nullptr); > + > + /* Reset m_current_thread immediately to mark the buffer as available, in case Uppercase m_current_thread. > + something goes wrong below. */ > + m_current_thread = nullptr; > + > + /* Now that a buffer gets freed, tell GDB it can ask us to prepare a displaced How about "tell infrun" instead of "tell GDB"? This is all in GDB. > + step again for this inferior. Do that here in case something goes wrong > + below. */ > + thread->inf->displaced_step_state.unavailable = false; > + > + ULONGEST len = gdbarch_max_insn_length (arch); > + > @@ -1927,14 +1833,41 @@ static step_over_what thread_still_needs_step_over (struct thread_info *tp); > static bool > start_step_over (void) > { > - struct thread_info *tp, *next; > + thread_info *next; > > /* Don't start a new step-over if we already have an in-line > step-over operation ongoing. */ > if (step_over_info_valid_p ()) > return false; > > - for (tp = global_thread_step_over_chain_head; tp != NULL; tp = next) > + /* Steal the global thread step over chain. As we try to initiate displaced > + steps, threads will be enqueued in the global chain if no buffers are > + available. If we iterated on the global chain directly, we might iterate > + indefinitely. */ > + thread_info *threads_to_step = global_thread_step_over_chain_head; > + global_thread_step_over_chain_head = NULL; > + > + infrun_debug_printf ("stealing global queue of threads to step, length = %d", > + thread_step_over_chain_length (threads_to_step)); > + > + bool started = false; > + > + /* On scope exit (whatever the reason, return or exception), if there are > + threads left in the THREADS_TO_STEP chain, put back these threads in the > + global list. */ > + SCOPE_EXIT { This format looks un-GNU-ish to me. I've been putting the { on the following line, indented by two spaces, like if it was a if/for/while block, like: SCOPE_EXIT { ... unless the whole thing fits in one line: SCOPE_EXIT { foo (); }; > + if (threads_to_step == nullptr) > + infrun_debug_printf ("step-over queue now empty"); > + else > + { > + infrun_debug_printf ("putting back %d threads to step in global queue", > + thread_step_over_chain_length (threads_to_step)); > + > + global_thread_step_over_chain_enqueue_chain (threads_to_step); > + } > + }; > + > + for (thread_info *tp = threads_to_step; tp != NULL; tp = next) > { > struct execution_control_state ecss; > struct execution_control_state *ecs = &ecss; > @@ -1943,12 +1876,23 @@ start_step_over (void) > > gdb_assert (!tp->stop_requested); > > - next = global_thread_step_over_chain_next (tp); > + next = thread_step_over_chain_next (threads_to_step, tp); > > - /* If this inferior already has a displaced step in process, > - don't start a new one. */ > - if (displaced_step_in_progress (tp->inf)) > - continue; > + if (tp->inf->displaced_step_state.unavailable) > + { > + /* The arch told us to not even try preparing another displaced step > + for this inferior. Just leave the thread in THREADS_TO_STEP, it > + will get moved to the global chain on scope exit. */ > + continue; > + } > + > + /* Remove thread from the THREADS_TO_STEP chain. If anything goes wrong > + while we try to prepare the displaced step, we don't add it back to > + the global step over chain. This is to avoid a thread staying in the > + step over chain indefinitely if something go wrong when resuming it. something goes wrong > + If the error is intermittent and it still needs a step over, it will > + get enqueued again when we try to resume it normally. */ > + thread_step_over_chain_remove (&threads_to_step, tp);