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] |
> -----Original Message----- > From: Jan Kratochvil [mailto:jan.kratochvil@redhat.com] > Sent: Wednesday, May 30, 2012 10:42 PM Thanks for your review! [...] > > +/* A new thread observer enabling branch tracing for the new thread. > > +*/ static struct observer *btrace_thread_observer; > > + > > +/* Branch tracing command list. */ > > +static struct cmd_list_element *btcmdlist; static struct > > +cmd_list_element *btencmdlist; static struct cmd_list_element > > +*btdiscmdlist; > > The variables should be each commented. Fixed. > > #if !defined(EALREADY) > > /* Remap EALREADY for systems that do not define it, e.g. mingw. */ > > @@ -252,3 +268,591 @@ next_btrace (struct thread_info *tinfo) > > > > return VEC_index (btrace_block_s, btinfo->btrace, > > btinfo->iterator); } > > + > > Function comment. Fixed. > > +static int > > +thread_callback (struct thread_info *tinfo, void *arg) > > Despite it is static there are some wishes for GDB all the functions should > have the filename prefix, therefore btrace_thread_callback here. I pity the maintainer of gdb/dwarf2-frame-tailcall.c;-) You mentioned this for another patch, already. If this is gdb style, I can certainly name my functions accordingly. I just don't see this a lot in other gdb files, and it results in odd and quite lengthy names. > > +{ > > + observer_new_thread_ftype *tfun = arg; > > Empty line after declarations. Fixed all occurences in this source file. [...] > > + if (!pid) > > + pid = ptid_get_pid (tinfo->ptid); > > + > > + error (_("Couldn't enable branch tracing for %d: %s"), > > + pid, safe_strerror (errcode)); > > Just use %s and as argument: target_pid_to_str (tinfo) Fixed. > But enable_btrace should error on its own as I have recommended. Will fix once the situation with gdbserver and shared code is clear. [...] > > + error (_("Couldn't disable branch tracing for %d: %s"), > > + pid, safe_strerror (errcode)); > > + } > > Likewise above. Fixed. [...] > > + if (number_is_in_list (range, tinfo->num)) > > + { > > + /* Switching threads makes it easier for targets like kgdb, where > > we > need > > + to switch cpus, as well. */ > > + switch_to_thread (tinfo->ptid); > > In such case it should be switched in the most bottom function. I can move it to the enable and disable functions. This will result in a few extra switches back to the selected thread when processing a list of threads. I hope that's OK. > > + safe_do_enable_btrace (tinfo); > > Here is TINFO still passed as a parameter so it is not required to call > switch_to_thread. I originally did not intend to switch threads, at all. All the btrace functions take the thread to operate on as explicit argument. When I later added btrace support to kgdb, I realized that I had to switch cpu's in order to perform certain operations. I should really do this inside kgdb, but it was so much easier just to switch threads in gdb. If you're not OK with this, I can remove all the thread switching and look for a solution in kgdb. > > + } > > + > > + return 0; > > +} > > + > > +static void > > +cmd_btrace_enable (char *args, int from_tty) { > > + if (args) > > So far I think GDB considers ARGS may be either NULL or "" as no- > parameters. Fixed. > > > + { > > + ptid_t ptid = inferior_ptid; > > Again use save_inferior_ptid like I suggested before. Fixed all occurrences in this file. [...] > > + } > > + else > > + { > > + struct thread_info *tinfo = find_thread_ptid (inferior_ptid); > > Empty line after declarations. Fixed. > > + if (!tinfo) > > + error (_("Couldn't enable branch tracing: no inferior thread.")); > > + else > > 'else' is not needed, 'error' doe snot return. Fixed all occurrences in this file. [...] > > +#define BTR_LIST_ADDRESS (1 << 0) > > +#define BTR_LIST_FUNCTION (1 << 1) > > +#define BTR_LIST_LINE (1 << 2) > > +#define BTR_LIST_TOTAL (1 << 3) > > It should be rather enum. Each item needs its comment. Fixed. > > + > > +static void > > +do_btrace_list_item (unsigned int block, struct btrace_block *trace, > > + unsigned int flags) { > > + struct gdbarch *gdbarch = get_current_arch (); > > gdbarch depends on TRACE, not on current frame. Each frame can have > different gdbarch. But putting gdbarch into each TRACE is probably too > expensive. > > I would guess target_gdbarch is more acceptable to use for printing > CORE_ADDR. Replaced this with target_arch. > > + struct symtab *symtab = NULL; > > + struct symbol *symbol = NULL; > > + > > + if (!trace) > > + return; > > Cannot happen, remove or gdb_assert. Removed. [...] > > + { > > + struct minimal_symbol *msymbol = > > + lookup_minimal_symbol_by_pc (trace->begin); > > Empty line. Fixed. [...] > > +static void > > +do_btrace_list (VEC (btrace_block_s) *btrace, char *range, > > + unsigned int flags) > > +{ > > + struct gdbarch *gdbarch = get_current_arch (); > > Unused variable. Fixed. [...] > > + else if (!btrace) > > + error (_("No trace")); > > dot: > error (_("No trace.")); Rearranged the if so the error case is at the final else. Since the preceding then statements do not return, I still need to put the error () under else. [...] > > + error (_("No trace.")); > > + return; > > Excessive return. Fixed all occurrences in this file. > > + } > > + > > + if (trace->end < trace->begin) > > + error (_("Bad trace: 0x%" BFD_VMA_FMT "x - 0x%" BFD_VMA_FMT > "x."), > > + trace->begin, trace->end); > > It is CORE_ADDR, not bfd_vma. Therefore use paddress. Fixed. Changed this to a warning. > > + > > + gdb_disassembly (gdbarch, current_uiout, 0, flags, -1, > > + trace->begin, trace->end + 1); } > > + > > +static void > > +cmd_btrace (char *args, int from_tty) { > > + struct thread_info *thread = find_thread_ptid (inferior_ptid); > > + struct btrace_block *trace = NULL; > > + int flags = 0; > > Use enum. Flags is a bit-vector. The enum just provides names for the bits. > > + > > + if (!thread) > > + { > > + error (_("No thread.")); > > + return; > > Excessive return. Fixed. [...] > > + VEC (btrace_block_s) *btrace = NULL; > > + struct cleanup *cleanup; > > + struct get_number_or_range_state state; > > + unsigned int i; > > + > > + cleanup = make_cleanup (xfree, btrace); > > You must use VEC_cleanup here, not xfree. Fixed. > > + init_number_or_range (&state, args); > > + while (!state.finished) > > + { > > + int index = get_number_or_range (&state); > > Empty line after declarations. Fixed. > > + if (!index) > > + { > > + error (_("Args must be numbers or '$' variables.")); > > + return; > > Excessive return. Fixed. > > + } > > + > > + trace = read_btrace (thread, index - 1); > > + if (!trace) > > + continue; > > + > > + VEC_safe_push (btrace_block_s, btrace, trace); > > + } > > + > > + i = VEC_length (btrace_block_s, btrace); > > + while (i--) > > + do_btrace (VEC_index (btrace_block_s, btrace, i), flags); > > + > > + do_cleanups (cleanup); > > + return; > > The function is a bit spaghetting, maybe you could split it for the > different > cases. Just optional. I split the function into argument processing and trace printing. > > + } > > + > > + if (args && *args) > > + { > > + error (_("Junk after argument: %s"), args); > > + return; > > Excessive return. Fixed. [...] > > + add_cmd ("branchtrace", class_btrace, NULL, > > + _("Recording a branch trace."), &cmdlist); > > Incorrect indentation, it would be (using tab): > add_cmd ("branchtrace", class_btrace, NULL, > _("Recording a branch trace."), &cmdlist); but you can: > add_cmd ("branchtrace", class_btrace, NULL, _("Recording a branch > trace."), > &cmdlist); Fixed. > > + add_prefix_cmd ("btrace", class_btrace, cmd_btrace, _("\ > > +Disassemble the selected branch trace block.\n\n\ With a /m modifier, > > +source lines are included (if available).\n\ With a /r modifier, raw > > +instructions in hex are included.\n\n\ Without arguments, selects the > > +chronologically preceding block.\n\ With \"+[<n>]\" argument, selects > > +the n-th chronologically preceding block.\n\ With \"-[<n>]\" > > +argument, selects the n-th chronologically succeeding block.\n\ With > > +one positive integer argument, selects the respective block.\n\ With > > +a range argument \"<l>-<h>\", selects the h-th block and disassembles > > +\ blocks in the range in reverse (i.e. original control flow) order.\n"), > > + &btcmdlist, "btrace ", 1, &cmdlist); > > In many cases - you should use tab character instead of 8 spaces. Fixed. Do you happen to know how to tell emacs to use tab for indentation? [...] > > "List of classes of commands:" > > "" > > "aliases -- Aliases of other commands" > > + "branchtrace -- Recording a branch trace" > > I am not sure if it is worth a new class but it does not fit much in > 'tracepoints'. It didn't fit anywhere, really, so I ended up adding a new class. Regards, Markus.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
-------------------------------------------------------------------------------------- Intel GmbH Dornacher Strasse 1 85622 Feldkirchen/Muenchen, Deutschland Sitz der Gesellschaft: Feldkirchen bei Muenchen Geschaeftsfuehrer: Douglas Lusk, Peter Gleissner, Hannes Schwaderer Registergericht: Muenchen HRB 47456 Ust.-IdNr./VAT Registration No.: DE129385895 Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |