RE: [PATCH 04/16] thread, btrace: add generic branch trace support

["Metzger, Markus T" <markus.t.metzger@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6323 bytes --]

> -----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 #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 417 bytes --]

--------------------------------------------------------------------------------------
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