From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31652 invoked by alias); 29 Jan 2013 09:53:22 -0000 Received: (qmail 31636 invoked by uid 22791); 29 Jan 2013 09:53:20 -0000 X-SWARE-Spam-Status: No, hits=-6.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_SPAMHAUS_DROP,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS,TW_EG X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 29 Jan 2013 09:53:14 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r0T9rA7Q032360 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 29 Jan 2013 04:53:10 -0500 Received: from host2.jankratochvil.net (ovpn-116-88.ams2.redhat.com [10.36.116.88]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r0T9r48I009999 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Tue, 29 Jan 2013 04:53:07 -0500 Date: Tue, 29 Jan 2013 09:53:00 -0000 From: Jan Kratochvil To: "Metzger, Markus T" Cc: "markus.t.metzger@gmail.com" , "gdb-patches@sourceware.org" Subject: Re: record-btrace Message-ID: <20130129095303.GA12614@host2.jankratochvil.net> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2013-01/txt/msg00678.txt.bz2 Hi Markus, thanks for a nice plan overview. On Mon, 28 Jan 2013 17:13:59 +0100, Metzger, Markus T wrote: > 1. add a new command "record-btrace" that pushes a new "record btrace", there are already subcommands of "record". It would be unconvenient to have some subcommands as "record-ABC" and some as "record DEF" wrt tab-completion. On Thu, 20 Dec 2012 08:17:26 +0100, Jan Kratochvil wrote: # Currently one uses just "record" to start the full-recording. I was expecting # it could print an error and one would use in newer versions of GDB # "record btrace" vs. "record full" or something like that. Expecting in this mail the current record backend would be called "full", I do not have a better name now. "slow" does not seem to great. :-) > record_btrace_target_ops onto the target stack. By default, this will enable > branch tracing for all existing and all new threads. Arguments may later be > added to only enable branch tracing selectively for some threads. OK. > The existing brace target-ops and also the btrace RSP packets will need to > remain since I still need to configure branch tracing on the target. Without > them (i.e. the functionality hard-coded in record_btrace_target_ops), > I don't see how I could handle the remote case. Record_btrace_open will call > target_btrace_enable to do the actual work. It will fail if one of the > btrace enable calls fails. OK; therefore two btrace struct target_ops for linux-nat.c vs. remote.c and one btrace struct target_ops for record btrace (vs. full). > The target is global. The data (i.e. configuration, history), however, will > still be per-thread. I don't know how target record handles this. record.c has global variables which is sure wrong. > I was thinking of keeping the btrace internal structure. Yes; FYI struct thread_info could have REGISTRY_FIELDS for dynamic data with functions like register_thread_info_data_with_cleanup. But currently it does not have REGISTRY_FIELDS so the current struct thread_info->btrace is OK. > If we're going to extend this in the future, say, by adding an LBR-based > branch trace, this would be another record_btrace_lbr_target_ops that may > share some of the functions with the BTS-based record-btrace target. Probably, unaware now how much the LBR backend will differ. > You mentioned that we should not pop that target when the process terminates > to implement auto enabling for new processes. I'm not quite clear how that > will work. I always thought the target stack is newly setup for each > process. I wonder how we can leave the topmost entry always on? It is rather the opposite. Targets hook on to_mourn_inferior and call unpush_target on themselves. > 2. implement the standard "record" commands for this new record-btrace > target, i.e. "info record", "set/show record", "record goto", "record save", > "record restore", "record stop", "record delete". > > The "set/show record" sub-commands won't make much sense for record-btrace. > On the other hand, we would need some other sub-commands to configure branch > tracing. Would I dynamically add and remove those (sub-)commands depending > on which record target is currently active? I have not seen functions to > remove commands. Or would I need to put sub-command handling into a single > "set record" and "show record" command (i.e. do not use sub-commands)? For example current "set record insn-number-max" should be moved to "set record full insn-number-max" and current "set record insn-number-max" made an alias to it with deprecate_cmd. You are right dynamically adding/removing commands is not used in GDB. They are added as long as the module is compiled in during GDB configure and commands are never removed. It would be good to protect these commands like "set record full insn-number-max" and the btrace ones so that if other record target is currently pushed on stack they print a warning that the new value is ignored with the currently active record backend. > The "info record" command makes sense for all record variants, but it will > show different information for each record target. Would I add a new > target-ops function for it? Or would I add a new struct record_ops to > collect those functions? As there will be already pushed/unpushed target_ops for the backend I find easier to make new target_ops methods for such dispatching. This abstract part of record.c should be moved to a separate file. Or rather the "full" backend to be moved from record.c to record-full.c and record.c to remain only a thin wrapper for some of these record parts common across the btrace+full record backends. "target record" could be also renamed to "target record-full" (and "target record" kept as deprecated_cmd alias). > The "record save/restore" commands won't be available. Needs more target-ops > or record-ops functions. Yes, so they should also use targets_ops method and the btrace backend will print an error for them. > The "record goto" and "record delete" commands will need to work on btrace > data structures, instead. Needs more target-ops or record-ops functions. Yes, new target_ops methods seem OK to me. > I would essentially move those commands into a new file and implement some > record abstraction for them to work on. I see you probably describe what I have described above. > 3. add a new "record disas" "record disassemble" (the shortening is done automatically by GDB commands parser). > sub-command to print the disassembly of the last > instructions and iterate through the instruction history similar to the list > command. This will be disabled for target record and implemented for target > record-btrace. "disabled" = printing an error. > I would add another target_ops or record_ops function to do the actual work > that is called from the "record disas" command. > > In a later step, this could be implemented for target record, as well. Yes. > 4. add a new "record list" sub-command similar to "record disas" but listing > the source lines that have been executed. Extending the functionality also for the record-full target can be thought afterwards. > 5. extend "record stop" to take parameters that allow selective disabling of > branch tracing. Not sure if and how this will work with the existing target > record. Existing record-full does not have any selection so it would print an error on such attempt. > 6. Implement reverse execution for record_btrace_target_ops: > > To_resume/to_wait: iterate through instruction history > To_insert_breakpoint: maintain breakpoint list - don't forward to target > To_fetch_registers: only support the instruction pointer register > To_store_registers: not allowed > To_xfer_partial: not allowed > To_~_watchpoint: not allowed > > When I don't allow reading memory in target record, nothing works, anymore, > i.e. almost all gdb commands fail. > When I don't allow fetching registers in target record, there seems to be no > impact - I guess the regcache is up-to-date so we don't really need to ask > the target. > When I do a single reverse stepping command, however, it fails and gdb > believes that the target is executing - nothing works, anymore. > > The first experiment is not very encouraging. Do you think this can be done, > at all? > > I may need to allow reading memory for read-only dynamic sections so that at > least "disas" works. Does gdb maintain a dynamic section map? Could gdb be > taught to read this memory from a file, instead? I would not be too strict with accessing the inferior memory. I understand that the memory content may be different when GDB is in the btrace history but most of the memory including read/write variables a user may want to read will be the same. It could rather just print a CLI (the default command-line interface) warning if one accesses a read/write memory during command execution. Forbidding any memory access may be correct but it may be pain for the users. > I may also need to replace the unwinder even though I'm not adding unwind > support, yet - just to prevent it from failing. I agree, that would be too confusing. > Are unwinders global or can > a target add/remove unwinders? If unwinders can't be added/removed > dynamically, I would add an unwinder to target_ops that will, if present, > precede all other unwinders. Would that be OK? You can use frame_unwind_prepend_unwinder to place it as the first unwinder. You do not need to remove it, struct frame_unwind->frame_sniffer_ftype sniffer can just always return 0 when the record-btrace backend is currently not pushed on the target stack. Therefore next unwinder will be queried in such case. > I would like to simulate the above two (i.e. allow access to read-only > memory and add a do-nothing unwinder) to see what issues we might be running > into with a target record-btrace. Would you help me with that? Yes, still I hope it is more clear now. > Reverse- and subsequent forward- stepping command should target the current > thread only as long as the thread is inside the recorded execution history. > I hope that's what I get when I overwrite to_resume and to_wait and only > consider the current thread. BTW this behavior normally depends on "set scheduler-locking off/step/on". to_resume's 3rd parameter STEP will be 0 if one does "continue". FSF GDB has default "set scheduler-locking off" which I (and IIRC users) find confusing so for example Fedora GDB has default "set scheduler-locking step". I believe FSF GDB should also default to "set scheduler-locking step" but it causes some other issues which is why it is not so. I do not see how to implement scheduler-locking-aware implementation of the btrace backend. So I agree it is OK it would behave as if "set scheduler-locking on" was set. > When they reach the end of the trace, they turn into normal stepping > commands, i.e. I will forward to_resume and to_wait to the target beneath. > This should implicitly continue other threads depending on the execution > mode. When the target stops next time, the tracing positions for all stopped > threads will be reset - I won't be able to find the old position in the new > version of the trace. This behavior might be somewhat surprising if you > switched threads while traversing the history. I would rather print an error in such case - that one should switch to the thread with position in history and roll it back to the current position before the whole process can be resumed by the target beneath. But I really do not mind how this special case is handled, IMO it won't happen in any common case. Thanks, Jan