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.133.124]) by sourceware.org (Postfix) with ESMTPS id 6F0D738308DC for ; Fri, 16 Dec 2022 10:03:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6F0D738308DC Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1671185039; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QBbYN2aoBFDuZiU13CuHmoCExjz1BRiS25ZnXNU5law=; b=WKZsLxCEi+lxg1PmZ+yrsy3N9Rxs0PXkrDu4wMGH0lbjbwJieZceRGlpiNYupIp3PjyorY +f2jVs1joQGOPg+27MOsKUdBWyuGqGelebKTv67eJ3u8syAlp0Da+yodHa7RPMKtmTdbhF 933dgue3wh9WURHC8BfOQjIkFenzqJQ= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-50-gLeKD5B2ObGE5mkhUkbzww-1; Fri, 16 Dec 2022 05:03:57 -0500 X-MC-Unique: gLeKD5B2ObGE5mkhUkbzww-1 Received: by mail-wm1-f71.google.com with SMTP id f1-20020a1cc901000000b003cf703a4f08so478999wmb.2 for ; Fri, 16 Dec 2022 02:03:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=QBbYN2aoBFDuZiU13CuHmoCExjz1BRiS25ZnXNU5law=; b=l+PhSbdYSAJjHhYMROGvz6YbpNi1pkW7QExt8h2QovbG7zt+RIqc4Ogj4xu8xzBFq5 dyEKshPN8gPEeWJhYxofxUGApYCg1iFH7etqxgmsSC3NgwOfxILcTC/ovGHjkqmfa1QZ XCr7kGpIRXU/OqFcTjoEB9RSTV2jItTsCwbP1MEwZxM1H6wsmiVtb0Bvgsjr6nBsIWhQ 13XWgEJ1qsJ4WMg2wdPfVt+wK+YdNM+G3c6I+kbORnzt0X94qvtz+bgbGNfDJUcHgfns SZ/sXtzL3AwNnRLZzsGm/GxpEt4UfZwVQfDbe+fC+ykTHgu5zb+SRK4FuE/Ly+YpWAAI 5jAw== X-Gm-Message-State: ANoB5pnwxTH9F/XPfDqDLbdbD1vkPA0KVJHkm7ICIdAIrh255eZ8xskO I6FXoih1fYw25lNrPRMGyM8kuhJUkWRf4EstSXYOw8XZDdN1WVgi/S6uPwP+GlNjkbeJA+BK1j4 /lUEqFpxJCKwwkiCjiWM39Q== X-Received: by 2002:a05:600c:4f08:b0:3d1:d746:d95b with SMTP id l8-20020a05600c4f0800b003d1d746d95bmr24341224wmq.41.1671185036507; Fri, 16 Dec 2022 02:03:56 -0800 (PST) X-Google-Smtp-Source: AA0mqf4AYJBnll+BtwjWH/jH1yW1ufT10rmsf7+wVT9OJ+Q94/B7bLY+ZEGlBuUCgB5JHSTsRPOlhg== X-Received: by 2002:a05:600c:4f08:b0:3d1:d746:d95b with SMTP id l8-20020a05600c4f0800b003d1d746d95bmr24341204wmq.41.1671185036163; Fri, 16 Dec 2022 02:03:56 -0800 (PST) Received: from [192.168.0.45] (ip-62-245-66-121.bb.vodafone.cz. [62.245.66.121]) by smtp.gmail.com with ESMTPSA id k11-20020a05600c0b4b00b003c5571c27a1sm2334192wmr.32.2022.12.16.02.03.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 16 Dec 2022 02:03:55 -0800 (PST) Message-ID: <41599666-8c34-8bbd-5264-65a5ea5ef825@redhat.com> Date: Fri, 16 Dec 2022 11:03:54 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH v2] gdb: add 'maintenance print record-instruction' command To: Lancelot SIX Cc: gdb-patches@sourceware.org References: <20221212104417.136536-1-blarsen@redhat.com> <20221214004529.3hgz2yuhv6rx2r4c@ubuntu.lan> From: Bruno Larsen In-Reply-To: <20221214004529.3hgz2yuhv6rx2r4c@ubuntu.lan> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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 14/12/2022 01:46, Lancelot SIX wrote: > Hi Bruno, > > I have a couple more comments and suggestions below. > > On Mon, Dec 12, 2022 at 11:44:17AM +0100, Bruno Larsen via Gdb-patches wrote: >> While chasing some reverse debugging bugs, I found myself wondering what >> was recorded by GDB to undo and redo a certain instruction. This commit >> implements a simple way of printing that information. >> --- >> gdb/NEWS | 6 ++++ >> gdb/doc/gdb.texinfo | 8 +++++ >> gdb/record-full.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 95 insertions(+) >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index c4ccfcc9e32..d6ce6bf86a0 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -103,6 +103,12 @@ >> >> * New commands >> >> +maintenance print record-instruction [ N ] >> + Print the recorded information for a given instruction. If N is not given >> + prints how GDB would undo the last instruction executed. If N is negative, >> + prints how GDB would undo the N-th previous instruction, and if N is >> + positive, it prints how GDB will redo the N-th following instruction. >> + >> maintenance set ignore-prologue-end-flag on|off >> maintenance show ignore-prologue-end-flag >> This setting, which is off by default, controls whether GDB ignores the >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 5b566669975..807af351e79 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -40531,6 +40531,14 @@ that symbol is described. The type chain produced by this command is >> a recursive definition of the data type as stored in @value{GDBN}'s >> data structures, including its flags and contained types. >> >> +@kindex maint print record-instruction >> +@item maint print record-instruction >> +@itemx maint print record-instruction @var{N} >> +@cindex print how GDB recorded a given instruction. If N is not positive >> +number, it prints the values stored by the inferior before the N-th previous >> +instruction was exectued. If N is positive, print the values after the N-th >> +following instruction is executed. If N is not given, 0 is assumed. >> + >> @kindex maint selftest >> @cindex self tests >> @item maint selftest @r{[}-verbose@r{]} @r{[}@var{filter}@r{]} >> diff --git a/gdb/record-full.c b/gdb/record-full.c >> index 48b92281fe6..25e1fd22c6f 100644 >> --- a/gdb/record-full.c >> +++ b/gdb/record-full.c >> @@ -2764,6 +2764,79 @@ set_record_full_insn_max_num (const char *args, int from_tty, >> } >> } >> >> +/* Implement the 'maintenance print record-instruction' command. */ >> + >> +static void >> +maintenance_print_record_instruction (const char *args, int from_tty) >> +{ >> + struct record_full_entry* to_print = record_full_list; >> + >> + if (args != nullptr) >> + { >> + int offset = value_as_long (parse_and_eval (args)); >> + if (offset > 0) >> + { >> + /* Move forward OFFSET instructions. We know we found the >> + end of an instruction when to_print->type is 0. */ > I think the literal 0 in the comment is a left-over from V1, right? > >> + while (to_print->next != nullptr && offset > 0) >> + { >> + to_print = to_print->next; >> + if (to_print->type == record_full_end) >> + offset--; >> + } >> + if (offset != 0) >> + error (_("Not enough recorded history")); >> + } >> + else >> + { >> + while (to_print->prev != nullptr && offset < 0) >> + { >> + to_print = to_print->prev; >> + if (to_print->type == record_full_end) >> + offset++; >> + } >> + if (offset != 0) >> + error (_("Not enough recorded history")); >> + } >> + } >> + gdb_assert (to_print != nullptr); >> + >> + /* Go back to the start of the instruction. */ >> + while (to_print->prev != nullptr && to_print->prev->type != record_full_end) >> + to_print = to_print->prev; >> + >> + while (to_print->type != record_full_end) >> + { >> + switch (to_print->type) >> + { >> + case record_full_reg: >> + { >> + gdb_byte* b = record_full_get_loc (to_print); > ^^ > > The space comes before the * here. > >> + gdb_printf ("Register %%%s changed:", > The '%' prefix for register names is specific to the att assembler > syntax. Do we want this here? I think in most places GDB uses the > plain register name without such prefix. > >> + gdbarch_register_name (target_gdbarch (), >> + to_print->u.reg.num)); >> + for (int i = 0; i < to_print->u.reg.len; i++) >> + gdb_printf (" %02x",b[i]); > ^ > Space after the ",". > >> + gdb_printf ("\n"); > Did you consider printing the register value instead of the bytes > composing the value? I think something like this could do: > > auto mark = value_mark (); > type *regtype = gdbarch_register_type (target_gdbarch (), > to_print->u.reg.num); > value *val > = value_from_contents (regtype, record_full_get_loc (to_print)); > > gdb_printf ("Register %s changed: %s\n", > gdbarch_register_name (target_gdbarch (), > to_print->u.reg.num), > hex_string (value_as_long (val))); > value_free_to_mark (mark); > > This would however not work for vector registers (anything which does > not fit in a long long). You might want to use something like > `value_print` to handle all cases, unless its output is too fancy: > > struct value_print_options opts; > get_user_print_options (&opts); > auto mark = value_mark (); > type *regtype = gdbarch_register_type (target_gdbarch (), > to_print->u.reg.num); > value *val > = value_from_contents (regtype, record_full_get_loc (to_print)); > > gdb_printf ("Register %s changed: %s\n", > gdbarch_register_name (target_gdbarch (), > to_print->u.reg.num)); > value_print (val, gdb_stdout, &opts); > gdb_printf ("\n"); > value_free_to_mark (mark); > > Not sure if there is a cleaner way to achieve this, but it should do the > trick. Hi Lancelot, Thanks for the review, I've fixed all the style nits. I wasn't aware of how to do this, thanks for explaining! I was a bit reticent of using the value_as_long option specifically because I wanted to handle all types of registers, but if I can make value_print work it will definitely be better. However, when I tried this I got the following output: (gdb) maint print record-instruction 8 bytes of memory at address 0x00007fffffffde48 changed from: e0 de ff ff ff 7f 00 00 (gdb) Register rip changed: (void (*)()) 0x40113e I see a few issues here: * The casting at the front is pretty confusing. I want the printing to be raw so the user can see what is going on * I'm not necessary fond of the at the end, mostly because I am worried that it might be misplaced in other instances * it is being printed after the gdb prompt I'm saying this mostly because it will probably take me a while to make a new version, but I really appreciate your feedback! -- Cheers, Bruno > >> + break; >> + } >> + case record_full_mem: >> + { >> + gdb_byte* b = record_full_get_loc (to_print); > Same here, the space shoud be before the "*". > >> + gdb_printf ("%d bytes of memory at address %s changed from:", >> + to_print->u.mem.len, >> + print_core_address (target_gdbarch (), >> + to_print->u.mem.addr)); >> + for (int i = 0; i < to_print->u.mem.len; i++) >> + gdb_printf (" %02x",b[i]); > Space after the ",". > > Best, > Lancelot. > >> + gdb_printf ("\n"); >> + break; >> + } >> + } >> + to_print = to_print->next; >> + } >> +} >> + >> void _initialize_record_full (); >> void >> _initialize_record_full () >> @@ -2868,4 +2941,12 @@ When ON, query if PREC cannot record memory change of next instruction."), >> c = add_alias_cmd ("memory-query", record_full_memory_query_cmds.show, >> no_class, 1,&show_record_cmdlist); >> deprecate_cmd (c, "show record full memory-query"); >> + >> + add_cmd ("record-instruction", class_maintenance, >> + maintenance_print_record_instruction, >> + _("\ >> +Print a recorded instruction.\nIf no argument is provided, print the last \ >> +instruction recorded.\nIf a negative argument is given, prints how the nth \ >> +previous instruction will be undone.\nIf a positive argument is given, prints \ >> +how the nth following instruction will be redone."), &maintenanceprintlist); >> } >> -- >> 2.38.1 >>