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 > To: Metzger, Markus T Thanks for your review! [...] > > +#if !defined(EALREADY) > > GNU Coding Style formatting would be: > #if !defined (EALREADY) > but maybe it is easier: > #ifndef EALREADY Fixed. > > +/* Remap EALREADY for systems that do not define it, e.g. mingw. */ > > +# define EALREADY EBUSY #endif > > + > > +int > > +enable_btrace (struct thread_info *tinfo) > > Very every new function must have a comment before. > It is very common across the whole patchset. > > This function is documented in btrace.h so it is enough to write here: > > /* See definition in btrace.h. */ > > int > enable_btrace (struct thread_info *tinfo) OK. I'll add comments to every function. > I did not list them specifically but I see at least some of the new > functions > without comment are really not documented in their corresponding .h file (at > least the 'static' ones). > > Also use some common prefix for the global functions in btrace.c, most > probably just rename this function to btrace_enable and other functions too. I tried to stick to the existing style. If you look at gdb/tracepoint.h, for example, functions are called start_tracing() and stop_tracing() instead of trace_start() and trace_stop(). If you're OK, I would leave the names as they are. > > +{ > > + if (!tinfo) > > + return EINVAL; > > This cannot happen (similarly in other functions) in current code. It could > be > rather 'gdb_assert (tinfo != NULL);' but I think it can be even omitted. I removed those pointer checks. > Also 'struct thread_info *' is commonly called 'tp' in GDB. But it is not > required to change it. Fixed. > > + > > + if (tinfo->btrace.target) > > + return EALREADY; > > Isn't more suitable here to say some user message instead? > error (_("Branch tracing is already enable for %s."), > target_pid_to_str (tinfo->ptid)); > > The generic message is not too user friendly, in KVM (assuming not > supporting btrace that way): > (gdb) btrace enable all > warning: Couldn't enable branch tracing for 26535: No such file or > directory > > "No such file or directory" may not be understood well by a user. Agreed. > > + > > + tinfo->btrace.target = target_enable_btrace (tinfo->ptid); if > > + (!tinfo->btrace.target) > > + return (errno ? errno : ENOSYS); > > I do not see the whole picture now but I do not find these error codes too > right, it is like in C code. GDB uses more the > error()/throw_error()/TRY_CATCH exceptions. Wouldn't it simplify the code > a lot? The idea was to not have the low levels talk to the user directly. I was not aware of the extensive use of exception handling until I followed the discussion about C++ - which was after I wrote this code. I am trying to share the low level code with gdbserver, which seems to use a restricted form of exception handling or none at all for IN_PROCESS_AGENT. This reminds me that I have not tested IN_PROCESS_AGENT. I have done nothing to either include or exclude the new btrace packets and I don't know what the default is. Can you point me to some documentation on how to build and test it? If I called error () to indicate that I could not configure branch tracing, for example, this would cause the IN_PROCESS_AGENT to exit () and might not lead to the correct response for standard gdbserver (I have not checked this). Do you have a proposal on what to do in this case? Is it OK to call error () in gdbserver code and trust the gdbserver infrastructure to handle it correctly? [...] > > + if (!errcode) > > + { > > + VEC_free (btrace_block_s, btinfo->btrace); > > + btinfo->btrace = NULL; > > VEC_free already does 'btinfo->btrace = NULL;' (I agree it is confusing). Fixed. [...] > > +void > > +disconnect_btrace (void) > > +{ > > + ptid_t ptid = inferior_ptid; > > Minor style issue - instead: > struct cleanup *old_chain = save_inferior_ptid (); > > > + > > + iterate_over_threads (do_disconnect_btrace, NULL); > > + > > + switch_to_thread (ptid); > > Minor style issue - instead: > do_cleanups (old_chain); > > This makes it safe against possible future throws of errors. Fixed. [...] > > + /* The first block ends at the current pc. */ > > + if (!VEC_empty (btrace_block_s, btinfo->btrace)) > > + { > > + struct frame_info *frame = get_current_frame (); > > Empty line after declarations. Fixed. > > + if (frame) > > + { > > + struct btrace_block *head = > > + VEC_index (btrace_block_s, btinfo->btrace, 0); > > Empty line after declarations. Fixed. [...] > > + if (btinfo->iterator >= VEC_length (btrace_block_s, btinfo->btrace)) > > + { > > + btinfo->iterator = VEC_length (btrace_block_s, btinfo->btrace); > > + return NULL; > > So == VEC_length is not permitted and you still set it to VEC_length? > Shouldn't it be set to VEC_length -1 in such case? See more by comment for > the btrace_thread_info.iterator field. Changed the condition to >. Also for the other cases. [...] > > +struct btrace_block > > +{ > > + CORE_ADDR begin; > > + CORE_ADDR end; > > Describe END is the last byte (and not one-after-the-last-one). BTW I would > find easier to make it rather one-after-the-last-byte. This is not the last byte of the block but the address of the last instruction in the block. I'll add a comment. [...] > Could you describe here what does mean if it is -1 and what does mean if it > is > VEC_length (btrace)? The code is doing some magic with it. I'll add a comment to the iterator field in struct btrace_thread_info in gdb/btrace.h [...] > > INHERIT (to_traceframe_info, t); > > INHERIT (to_use_agent, t); > > INHERIT (to_can_use_agent, t); > > + INHERIT (to_supports_btrace, t); > > This whole INHERIT / de_fault / '#define target_.*' way is the deprecated > one. > The currently recommended way is to use stub functions like > target_verify_memory (and many others). I am sorry it is probably not > documented anywhere. I can add those. I used to declare respective macros. Do you want me to drop the INHERIT changes from the patch? [...] 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] |