This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Pedro, The attachment is the compress for all the patches of precord. And I will post each patch to maillist too. Please help me review it. The following is what I updated according to your comment: > >> +record_list_release (record_t * rec) >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?^ > > drop the space between '*' and rec, here and elsewhere. It was fixed in 3. > > Is record_t supposed to be an opaque type? ?Throughout > gdb, we tend to only "_t" struct typedef types if they're > going to be value-like, passed by type, and opaque. > Otherwise, we just use something like: > > ?record_list_release (struct record_entry *rec) > record_t is changed to record_entry in 3. > > ?Add a blank line between comment and function. > I fixed it in all the patches of p record. >> + record_regcache = get_current_regcache (); > > > - ARGS is a `struct gdbarch', yet you access the current regcache. > I see you pass current_gdbarch to do_record_message. We're trying > to eliminate the current_gdbarch global. > >> + if (do_record_message (current_gdbarch)) record_regcache was removed. Now, regcache is the argument of a lot of functions of precord. > Please delete this comment: > >> + /* XXX: I try to use some simple commands such as "disconnect" and >> + "detach" to support this functions. But these commands all have >> + other affect to GDB such as call function "no_shared_libraries". >> + So I add special commands to GDB. */ It was deleted. > * 4-linux-record.txt > >> + Copyright (C) 2008 Free Software Foundation, Inc. > > 2009. Done. >> +#include <stdint.h> > Remove this. defs.h includes stdint.h. Done. > I was looking at the commands record, delrecord, stoprecord, > record-stop-at-limit, record-insn-number-max, record-insn-number, > and wondering what would you think if we made all record commands > under a "record" prefix? > > record > record stop > record delete > set record stop-at-limit > set record insn-number-max > info record insn-number Done. > * 5-infrun.txt > infun.c: > >> + #include "record.h" > >> + && current_target.to_stratum != record_stratum); > > Sigh, I've spent to much time trying to explain why this was > wrong. Let's at least leave this behind the macro you used to > have. OK. I make it to a macro. The following is my explain for your comment: > >> + ? ? ? ? ? q = yquery (_("Do you want to auto delete previous execution " >> + ? ? ? ? ? ? ? ? ? ? ? ? "log entries when record/replay buffer becomes " >> + ? ? ? ? ? ? ? ? ? ? ? ? "full (record-stop-at-limit)?")); > > > ?What do you mean here by "when"? ?Didn't the user just hit > the limit *now*, and that is why you're asking what to do? > Because if user said yes, it will auto delete each time. It will not ask user again. So I say when. > - When I read the first time, my question is: 'What is a "record message"'? GDB record the running message. >> +static void >> +record_sig_handler (int signo) >> +{ >> + ?if (record_debug) >> + ? ?fprintf_unfiltered (gdb_stdlog, "Process record: get a signal\n"); >> + >> + ?/* It will break the running inferior in replay mode. ?*/ >> + ?record_resume_step = 1; > > ?I still don't understand this comment. ?Please explain *why* you > need to set this here. > { /* In EXEC_REVERSE mode, this is the record_end of prev instruction. In EXEC_FORWARD mode, this is the record_end of current instruction. */ /* step */ if (record_resume_step) { if (record_debug > 1) fprintf_unfiltered (gdb_stdlog, "Process record: step.\n"); continue_flag = 0; } /* check breakpoint */ tmp_pc = regcache_read_pc (regcache); if (breakpoint_inserted_here_p (tmp_pc)) { if (record_debug) fprintf_unfiltered (gdb_stdlog, "Process record: break " "at 0x%s.\n", paddr_nz (tmp_pc)); if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)) && execution_direction == EXEC_FORWARD && !record_resume_step) regcache_write_pc (regcache, tmp_pc + gdbarch_decr_pc_after_break (get_regcache_arch (regcache))); continue_flag = 0; } } When p record get a sig, I need it stop like single step. So set record_resume_step to 1. > >> +struct cleanup * >> +record_gdb_operation_disable_set (void) >> +{ >> + ?struct cleanup *old_cleanups = NULL; >> + >> + ?if (!record_gdb_operation_disable) >> + ? ?{ >> + ? ? ?old_cleanups = >> + ? ? ? ?make_cleanup_restore_integer (&record_gdb_operation_disable); >> + ? ? ?record_gdb_operation_disable = 1; >> + ? ?} >> + >> + ?return old_cleanups; >> +} > > This returns a NULL cleanup if record_gdb_operation_disable is > already set, but make_cleanup also returns a legitimate > NULL, and it is not an error. ?It returns NULL when for the > first cleanup put in the chain. ?See make_my_cleanup2. > if (set_cleanups) do_cleanups (set_cleanups); void do_cleanups (struct cleanup *old_chain) { do_my_cleanups (&cleanup_chain, old_chain); } static void do_my_cleanups (struct cleanup **pmy_chain, struct cleanup *old_chain) { struct cleanup *ptr; while ((ptr = *pmy_chain) != old_chain) { *pmy_chain = ptr->next; /* Do this first incase recursion */ (*ptr->function) (ptr->arg); if (ptr->free_arg) (*ptr->free_arg) (ptr->arg); xfree (ptr); } } NULL will make all the chain clean. > >> +static void >> +record_wait_cleanups (void *ignore) >> +{ >> + ?if (execution_direction == EXEC_REVERSE) >> + ? ?{ >> + ? ? ?if (record_list->next) >> + ? ? record_list = record_list->next; >> + ? ?} >> + ?else >> + ? ?record_list = record_list->prev; >> +} > >> +static ptid_t >> +record_wait (struct target_ops *ops, >> + ? ? ? ? ? ? ?ptid_t ptid, struct target_waitstatus *status) > (...) >> +struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0); > > This cleanup looks suspiciously broken to me. ?It appears that > is will do weird things depending on when an exception is thrown. > But then again, record_wait's nesting and use of goto's makes > my eyes bleed. ?:P This cleanup will make record_list point to the record entry that execution before this record entry because set this record entry get fail. This part is not very easy to make clear. I make clear it use a lot of time. :P About goto, most of them are deleted. I just keep one: /* Check breakpoint when forward execute. */ if (execution_direction == EXEC_FORWARD) { xxx xxx goto replay_out; } } > + ? ? ? uint32_t addr, count; > + ? ? ? regcache_raw_read (record_regcache, tdep->arg2, (gdb_byte *) & addr); > > This (and many more similar instances) is assuming > host-endianness == target-endianess, that the registers are 32-bit, and > probably that signed<->unsigned bit representation is equal. ?Is > this what you want? ?When you get to 64-bit, most of this will break, > it seems. For now, record just support i386 arch. Please let us keep it to the future. Thanks, Hui
Attachment:
prec20090416.tar.gz
Description: GNU Zip compressed data
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |