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]

Re: [PATCH 2/3] Implement new features needed for handling SystemTap probes


I've looked this one over.  See my by comments below.  I've tried to skip issues
already pointed out.

On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:

> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c


> +/* See the comment in breakpoint.h.  */
> +
> +void
> +modify_semaphore (struct bp_location *loc, int set)
> +{


Starting with a nit... This function name is a bit too generic for an
extern function.  Can we make it a bit more specific, like
breakpoint_modify_stap_semaphore, or modify_stap_semaphore, or
perhaps even clearer, (bp_)stap_semaphore_up/(bp_)stap_semaphore_down?
I'd even suggest changing the function's prototype to take the semaphore
address as argument instead, and move it elsewhere most stap things
are (stap-probe.c?).


On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:
> +{
> +  struct gdbarch *arch = loc->gdbarch;
> +  gdb_byte bytes[sizeof (LONGEST)];
> +  /* The ABI specifies "unsigned short".  */
> +  struct type *type = builtin_type (arch)->builtin_unsigned_short;
> +  CORE_ADDR address = loc->semaphore;
> +  ULONGEST value;
> +
> +  if (address == 0)
> +    return;
> +
> +  /* Swallow errors.  */
> +  if (target_read_memory (address, bytes, TYPE_LENGTH (type)) != 0)
> +    return;
> +
> +  value = extract_unsigned_integer (bytes, TYPE_LENGTH (type),
> +				    gdbarch_byte_order (arch));
> +  /* Note that we explicitly don't worry about overflow or
> +     underflow.  */
> +  if (set)
> +    ++value;
> +  else
> +    --value;
> +
> +  store_unsigned_integer (bytes, TYPE_LENGTH (type),
> +			  gdbarch_byte_order (arch), value);
> +
> +  target_write_memory (address, bytes, TYPE_LENGTH (type));
> +}

Is this racing with threads in the inferior, in non-stop mode?  If
so, we should find a way to fix, but a comment about it here
meanwhile would be quite worth it.


>  	  val = bl->owner->ops->insert_location (bl);
> +
> +	  modify_semaphore (bl, 1);

This is modifying the semaphore even if the insertion failed.  Please make sure
we can't reach this with the breakpoint already inserted, due to the recent
target side breakpoint conditions work, in which case you should
not increment the semaphore.  And I think we do.

>  	}
>        else
>  	{
> @@ -3153,6 +3194,8 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
>  	{
>  	  /* No overlay handling: just remove the breakpoint.  */
>  	  val = bl->owner->ops->remove_location (bl);
> +
> +	  modify_semaphore (bl, 0);

This is modifying the semaphore even if removal failed.  Not so clear what to do in this
case; things are bonkers already anyway.  But, in any case, at insertion time you have:

- insert breakpoint
- increment semaphore

so at removal, the obvious code that doesn't raise suspicions is the reverse sequence:

- decrement semaphore
- remove breakpoint

but you didn't do that.  You should, to avoid raising eyebrows :-).

And this makes me wonder about non-stop.  If the inferior is running, does is make a
different if you increment the semaphore before, or after you insert the breakpoint?


On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:
> +  /* Arguments.  We can only extract the argument format if there is a valid
> +     name for this probe.  */
> +  if (ret->name)

Am I the only one who's eyes sore with implicit pointer->bool conversions?  :-)

> +    {
> +      ret->args = memchr (ret->name, '\0',
> +			  (char *) el->data + el->size - ret->name);
> +
> +      if (ret->args != NULL)
> +	++ret->args;
> +      if (ret->args == NULL

Mixbag of styles in the same function is what hurts the most.  :-)


> +      if (! elf_tdata (obfd)->sdt_note_head)

No space after ! please.  Several instances of this.

On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:
> +      /* Parsing each probe's information.  */
> +      for (iter = elf_tdata (obfd)->sdt_note_head, i = 0;
> +	   iter;
> +	   iter = iter->next, i++)
> +	/* We first have to handle all the information about the
> +	   probe which is present in the section.  */
> +	handle_probe (objfile, iter, &ret[i], base);

GDB's coding convention now requires {}'s around the
multi-line comment + statement, even though there's only a
single-line statement.  More instances of this throughout.


> -/* Helper function that frees any unsed space in the expout array.
> -   It is generally used when the parser has just been parsed and
> -   created.  */
> +/* See definition in parser-defs.h.  */
>
> -static void
> +void
>  reallocate_expout (void)
>  {
>    /* Record the actual number of expression elements, and then
> @@ -811,7 +803,7 @@ copy_name (struct stoken token)
>     return the index of the subexpression which is the left-hand-side
>     of the struct operation at EXPOUT_LAST_STRUCT.  */
>
> -static int
> +int
>  prefixify_expression (struct expression *expr)
>  {

Seems like you move some comments to the headers, and others not.  Other
examples elsewhere.

On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -66,6 +66,14 @@
>  #include "features/rs6000/powerpc-isa205-vsx64l.c"
>  #include "features/rs6000/powerpc-e500l.c"
>
> +#include "stap-probe.h"
> +#include "ax.h"
> +#include "ax-gdb.h"
> +#include "cli/cli-utils.h"
> +#include "parser-defs.h"
> +#include "user-regs.h"
> +#include <ctype.h>

Please put these above the .c includes, along with the other .h includes.
More cases of this elsewhere.


> +    STAP_PROBE (teste, two);

Aha, a little PT sneaked in. :-)


> +proc stap_test {{arg ""}} {
> +    global testfile hex
> +
> +    if {$arg != ""} {
> +    	set arg "additional_flags=$arg"
> +	set addendum ", with semaphore, not optimized"
> +    } else {
> +	set addendum ", no semaphore, not optimized"
> +    }
> +

Instead of appending $addendum to all test messages (and missing
those that are emitted from within gdb.exp and friends), you should
now use with_test_prefix instead.

> --- a/gdb/testsuite/gdb.cp/nextoverthrow.exp
> +++ b/gdb/testsuite/gdb.cp/nextoverthrow.exp
> @@ -54,6 +54,17 @@ gdb_test_multiple "print _Unwind_DebugHook" "check for unwinder hook" {
>      }
>  }
>  if {!$ok} {
> +    gdb_test_multiple "info probe" "check for stap probe in unwinder" {
> +	-re ".*libgcc.*unwind.*\r\n$gdb_prompt $" {
> +	    pass "check for stap probe in unwinder"
> +	    set ok 1
> +	}
> +	-re "\r\n$gdb_prompt $" {
> +	}
> +    }
> +}
> +
> +if {!$ok} {
>      unsupported "nextoverthrow.exp could not find _Unwind_DebugHook"
>      return -1
>  }

I don't understand why we'd now skip the test when we don't have the unwinder
stap probe, but in any case, this should not be in this patch, GDB only learns to
use the unwind probe in the next patch.

> +
> +if $tracelevel then {
> +	strace $tracelevel
> +}

Please remove all traces of this.  All existing instances have already been
removed from the testsuite throughout.


On 03/09/2012 08:33 PM, Sergio Durigan Junior wrote:
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -1717,6 +1717,7 @@ start_tracing (char *notes)
>    for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, b); ix++)
>      {
>        struct tracepoint *t = (struct tracepoint *) b;
> +      struct bp_location *loc;
>
>        if (b->enable_state == bp_enabled)
>  	any_enabled = 1;
> @@ -1779,6 +1780,9 @@ start_tracing (char *notes)
>  	}
>
>        t->number_on_target = b->number;
> +
> +      for (loc = b->loc; loc; loc = loc->next)
> +	modify_semaphore (loc, 1);
>      }
>    VEC_free (breakpoint_p, tp_vec);
>
> @@ -1851,9 +1855,28 @@ void
>  stop_tracing (char *note)
>  {
>    int ret;
> +  VEC(breakpoint_p) *tp_vec = NULL;
> +  int ix;
> +  struct breakpoint *t;
>
>    target_trace_stop ();
>
> +  tp_vec = all_tracepoints ();
> +  for (ix = 0; VEC_iterate (breakpoint_p, tp_vec, ix, t); ix++)
> +    {
> +      struct bp_location *loc;
> +
> +      if ((t->type == bp_fast_tracepoint
> +	   ? !may_insert_fast_tracepoints
> +	   : !may_insert_tracepoints))
> +	continue;
> +
> +      for (loc = t->loc; loc; loc = loc->next)
> +	modify_semaphore (loc, 0);
> +    }
> +
> +  VEC_free (breakpoint_p, tp_vec);

This made me wonder about something else with this semaphore
handling: the target can itself stop tracing, without GDB requesting
it.  E.g., if the trace buffer is full.  If so, then you'll miss
decrementing the semaphore count...  Even worse with disconnected
tracing; GDB might not even be connected when the tracing stops,
and when you reconnect, you have no clue whether to decrement
the counts or not...

-- 
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]