From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id YMP1Nku2vV+OegAAWB0awg (envelope-from ) for ; Tue, 24 Nov 2020 20:41:31 -0500 Received: by simark.ca (Postfix, from userid 112) id DEC231F0AB; Tue, 24 Nov 2020 20:41:31 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI,RDNS_NONE autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 94A201E58E for ; Tue, 24 Nov 2020 20:41:31 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5C75139540BC; Wed, 25 Nov 2020 01:41:31 +0000 (GMT) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by sourceware.org (Postfix) with ESMTPS id 48A4D39540BC for ; Wed, 25 Nov 2020 01:41:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 48A4D39540BC 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-f67.google.com with SMTP id d142so638155wmd.4 for ; Tue, 24 Nov 2020 17:41:29 -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:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=WlCRS+/EUf4bc4UtZNGcLCCVdFGvYp2tv1hoNC5pw4c=; b=eN1IPTacb+xsVX8gM8qRXRGao3erJZJxr2I/vfXzvQwXHCvTGZRYKMavKr9r+LxCxd nBer7TirzDaByisP1IJUwwE7bbg6LUBl/tFoQnwqo2nDg/Zod6gZbEYzyaIV3hzqGEVW 7EIHs291grmqwwNK/7DEZcijPsuCIFqXece1JA/396IMDPVLVw+KF41TYA7l5wbZK0mA SgGOUZnW55cdpcmiDBqF0mBrHPa4IEPJmjoQaHYrzffi8BXt7mzOBGpem8k2MHFfKu9K 9xOZj8uEpfPmIkAFjYmeIt1s4kUKPlx3kM9WBUulsu1+4fGFf0f+3Vk9BRvOXt0sDcfX ppwQ== X-Gm-Message-State: AOAM533dnCYK+gmtJs0S1LERPGAqwO5Rmx2BQ/JeMCFF5VUU2glE2jgA rMYLPm+9QoUn+u7FXqRjv/4ybG8JjJhDTw== X-Google-Smtp-Source: ABdhPJwTcnG+3xfBjW90V3DF2YvIsaRm9qt7YHwYkDXXDHCQoSFfDqkAW1LdPlny2ySSk4TkRgaifw== X-Received: by 2002:a1c:3c4:: with SMTP id 187mr1071480wmd.143.1606268487075; Tue, 24 Nov 2020 17:41:27 -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 y18sm1386710wrr.67.2020.11.24.17.41.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 24 Nov 2020 17:41:26 -0800 (PST) Subject: Re: [PATCH 11/12] gdb: make displaced stepping implementation capable of managing multiple buffers To: Simon Marchi , gdb-patches@sourceware.org References: <20201110214614.2842615-1-simon.marchi@efficios.com> <20201110214614.2842615-12-simon.marchi@efficios.com> From: Pedro Alves Message-ID: <7cfcdcb4-f70f-a774-3378-e324735d1fe8@palves.net> Date: Wed, 25 Nov 2020 01:41:24 +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: <20201110214614.2842615-12-simon.marchi@efficios.com> 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" A few minor comments below. Otherwise LGTM. On 11/10/20 9:46 PM, Simon Marchi via Gdb-patches wrote: > @@ -44,86 +44,114 @@ show_debug_displaced (struct ui_file *file, int from_tty, > } > > displaced_step_prepare_status > -displaced_step_buffer::prepare (thread_info *thread, CORE_ADDR &displaced_pc) > +displaced_step_buffers::prepare (thread_info *thread, CORE_ADDR &displaced_pc) > { > gdb_assert (!thread->displaced_step_state.in_progress ()); > > - /* Is a thread currently using the buffer? */ > - if (m_current_thread != nullptr) > - { > - /* If so, it better not be this thread. */ > - gdb_assert (thread != m_current_thread); > - return DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE; > - } > + /* Sanity check: the thread should not be using a buffer at this point. */ > + for (displaced_step_buffer &buf : m_buffers) > + gdb_assert (buf.current_thread != thread); > > regcache *regcache = get_thread_regcache (thread); > const address_space *aspace = regcache->aspace (); > gdbarch *arch = regcache->arch (); > ULONGEST len = gdbarch_max_insn_length (arch); > > - if (breakpoint_in_range_p (aspace, m_addr, len)) > - { > - /* There's a breakpoint set in the scratch pad location range > - (which is usually around the entry point). We'd either > - install it before resuming, which would overwrite/corrupt the > - scratch pad, or if it was already inserted, this displaced > - step would overwrite it. The latter is OK in the sense that > - we already assume that no thread is going to execute the code > - in the scratch pad range (after initial startup) anyway, but > - the former is unacceptable. Simply punt and fallback to > - stepping over this breakpoint in-line. */ > - displaced_debug_printf ("breakpoint set in scratch pad. " > - "Stepping over breakpoint in-line instead."); > + /* Search for an unused buffer. */ > + displaced_step_buffer *buffer = nullptr; > + displaced_step_prepare_status fail_status > + = DISPLACED_STEP_PREPARE_STATUS_ERROR; > > - return DISPLACED_STEP_PREPARE_STATUS_ERROR; > + for (displaced_step_buffer &candidate : m_buffers) > + { > + bool bp_in_range = breakpoint_in_range_p (aspace, candidate.addr, len); > + bool is_free = candidate.current_thread == nullptr; > + > + if (!bp_in_range) > + { > + if (is_free) tabs vs spaces. > + { > + buffer = &candidate; > + break; > + } > + else > + { > + /* This buffer would be suitable, but it's used right now. */ > + fail_status = DISPLACED_STEP_PREPARE_STATUS_UNAVAILABLE; > + } > + } tabs vs spaces. > + else > + { > + /* There's a breakpoint set in the scratch pad location range > + (which is usually around the entry point). We'd either > + install it before resuming, which would overwrite/corrupt the > + scratch pad, or if it was already inserted, this displaced > + step would overwrite it. The latter is OK in the sense that > + we already assume that no thread is going to execute the code > + in the scratch pad range (after initial startup) anyway, but > + the former is unacceptable. Simply punt and fallback to > + stepping over this breakpoint in-line. */ > + displaced_debug_printf ("breakpoint set in displaced stepping " > + "buffer at %s, can't use.", > + paddress (arch, candidate.addr)); > + } > -/* Manage access to a single displaced stepping buffer. */ > +/* Control access to multiple displaced stepping buffers at fixed addresses. */ > > -struct displaced_step_buffer > +struct displaced_step_buffers > { > - displaced_step_buffer (CORE_ADDR buffer_addr) > - : m_addr (buffer_addr) > - {} > + displaced_step_buffers (gdb::array_view buffer_addrs) > + { > + gdb_assert (buffer_addrs.size () > 0); > + > + for (CORE_ADDR buffer_addr : buffer_addrs) > + m_buffers.emplace_back (buffer_addr); Call: m_buffers.reserve (buffer_addrs.size ()); before. But even better, the caller is allocating a temporary std::vector and passing that: + std::vector buffers; + for (int i = 0; i < gdbarch_data->num_disp_step_buffers; i++) + buffers.push_back (disp_step_buf_addr + i * buf_len); So how about making the ctor above be instead: displaced_step_buffers (std::vector &&buffer_addrs); and make the caller move the vector into displaced_step_buffers? + per_inferior->disp_step_bufs.emplace (std::move (buffers)); > + } > > displaced_step_prepare_status prepare (thread_info *thread, > CORE_ADDR &displaced_pc); > @@ -163,14 +168,33 @@ struct displaced_step_buffer > > private: > > - CORE_ADDR m_original_pc = 0; > - const CORE_ADDR m_addr; > + /* State of a single buffer. */ > + > + struct displaced_step_buffer > + { > + displaced_step_buffer (CORE_ADDR addr) "explicit". > + : addr (addr) > + {} > + > + const CORE_ADDR addr; > + > + /* The original PC of the instruction currently begin stepped. */ "begin" -> "being" ? > + CORE_ADDR original_pc = 0; > + > + /* If set, the thread currently using the buffer. If unset, the buffer is not > + used. */ > + thread_info *current_thread = nullptr; > + > + /* Saved copy of the bytes in the displaced buffer, to be restored once the > + buffer is no longer used. */ > + gdb::byte_vector saved_copy; >