From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 4AF663858D35 for ; Fri, 24 Mar 2023 14:09:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4AF663858D35 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [172.16.42.115] (modemcable092.73-163-184.mc.videotron.ca [184.163.73.92]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id EC47A1E0D3; Fri, 24 Mar 2023 10:09:57 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1679666998; bh=0Q9vja07g3+9w6mqIVmj7Wvk5LM7D+MEQpOqYBQu7fY=; h=Date:Subject:To:References:From:In-Reply-To:From; b=R0IfIzBDKyCDfZt8UoB9xjBBV5ISm8eY/i5u0WwrKPrtSdGK47hh5dm0TCv22tGlM l5Jpa8xz2lqVNmogbiaW62Zl9PcXM+HWsmzCv36bLgKhXPxr1PdIIGqm3soaVq+MKq TolEeWIxxJWktgPJvsZL+FzjPJcW/Qdp3zw/WcWw= Message-ID: <2404c795-edf2-01fa-dbfb-f52933648494@simark.ca> Date: Fri, 24 Mar 2023 10:09:57 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 Subject: Re: [PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary instructions. Content-Language: fr To: Felix Willgerodt , gdb-patches@sourceware.org References: <20230321154626.448816-1-felix.willgerodt@intel.com> <20230321154626.448816-5-felix.willgerodt@intel.com> From: Simon Marchi In-Reply-To: <20230321154626.448816-5-felix.willgerodt@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote: > Print the auxiliary data when stepping. Don't allow to goto an auxiliary > instruction. > > This patch is in preparation for the new ptwrite feature, which is based on > auxiliary instructions. Hi Felix, Just a few minor comments. No need to repost just for that. > @@ -2388,12 +2392,27 @@ record_btrace_single_step_forward (struct thread_info *tp) > of the execution history. */ > steps = btrace_insn_next (replay, 1); > if (steps == 0) > + { > + *replay = start; > + return btrace_step_no_history (); > + } These lines just (wrongfully) replaces the indentation with all spaces, when in reality these lines don't need to be touched. > + > + const struct btrace_insn *insn = btrace_insn_get (replay); > + if (insn == nullptr) > + continue; > + > + /* If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary > + data and skip the instruction. */ > + if (insn->iclass == BTRACE_INSN_AUX) > { > - *replay = start; > - return btrace_step_no_history (); > + gdb_printf ("[%s]\n", > + btinfo->aux_data.at (insn->aux_data_index).c_str ()); I was a bit thrown off by the use of std::vector::at :P (I think it was used in the previous patch too). Did you use it specifically for the behavior when the access is out of bounds? If not, using operator[] would probably be better (and more usual). > @@ -2859,25 +2895,30 @@ record_btrace_target::goto_record_end () > /* The goto_record method of target record-btrace. */ > > void > -record_btrace_target::goto_record (ULONGEST insn) > +record_btrace_target::goto_record (ULONGEST insn_number) > { > struct thread_info *tp; > struct btrace_insn_iterator it; > unsigned int number; > int found; > > - number = insn; > + number = insn_number; > > /* Check for wrap-arounds. */ > - if (number != insn) > + if (number != insn_number) > error (_("Instruction number out of range.")); > > tp = require_btrace_thread (); > > found = btrace_find_insn_by_number (&it, &tp->btrace, number); > > - /* Check if the instruction could not be found or is a gap. */ > - if (found == 0 || btrace_insn_get (&it) == NULL) > + /* Check if the instruction could not be found or is a gap or an > + auxilliary instruction. */ > + if (found == 0) > + error (_("No such instruction.")); > + > + const struct btrace_insn *insn = btrace_insn_get (&it); > + if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX)) > error (_("No such instruction.")); Just wondering, would it be useful for the user to print a more specific error message ("Can't go to an auxiliary instruction") if they try to goto an aux instruction? Simon