Re: [PATCH V2 6/9] Support for DTrace USDT probes in x86_64 targets.

[Pedro Alves <palves@redhat.com>]

> +amd64_dtrace_probe_is_enabled (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  gdb_byte buf[5];
> +  read_memory (addr - 3, buf, 5);
> +
> +  /* The instruction sequence used in x86_64 machines for a disabled
> +     is-enabled probe is:
> +
> +              xor %rax, %rax  =>  48 33 C0
> +     ADDR:    nop             =>  90
> +              nop             =>  90
> +

Please use read_code when reading code, so that these reads go through
the code cache.

> +  /* We use the following instruction sequence for disabling an
> +     is-enabled probe:
> +
> +     xor %rax, %rax; nop; nop  =>  48 33 C0 90 90
> +
> +     Note that ADDR is offset 3 bytes from the beginning of the
> +     sequence.  */
> +
> +  gdb_byte buf[5];
> +
> +  buf[0] = 0x48;
> +  buf[1] = 0x33;
> +  buf[2] = 0xc0;
> +  buf[3] = 0x90;
> +  buf[4] = 0x90;
> +  
> +  write_memory (addr - 3, buf, 5);

These code sequences are duplicated in lots of places.

Writing like this instead is both shorter and less error prone:

   const gdb_byte buf[] = { 0x48, 0x33, 0xc0, 0x90, 0x90 };

   write_memory (addr - 3, buf, sizeof (buf));

... and IMO eliminates the need to have the sequence duplicated in the
comment above.

Going further, we can make these sequences be globals, like:

  /* The instruction sequences used in x86_64 machines for a
     disabled is-enabled probe.  */
   const gdb_byte amd64_dtrace_disabled_probe_sequence_1[] = {
     /*          xor %rax, %rax */  0x48, 0x33, 0xc0,
     /* ADDR:    nop            */  0x90,
     /*          nop            */  0x90
   };
   const gdb_byte amd64_dtrace_disabled_probe_sequence_2[] = {
     /*          xor %rax, %rax */  0x48, 0x33, 0xc0,
     /* ADDR:    ret            */  0xc3,
     /*          nop            */  0x90
   };

etc.  And then instead of:

> +amd64_dtrace_probe_is_enabled (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  gdb_byte buf[5];
> +  read_memory (addr - 3, buf, 5);
> +
> +  /* The instruction sequence used in x86_64 machines for a disabled
> +     is-enabled probe is:
> +
> +              xor %rax, %rax  =>  48 33 C0
> +     ADDR:    nop             =>  90
> +              nop             =>  90
> +
> +     or
> +
> +              xor %rax, %rax  =>  48 33 C0
> +     ADDR:    ret             =>  c3
> +              nop             =>  90
> +
> +     This function returns 1 if the instructions at ADDR do _not_
> +     follow any of these patterns.
> +
> +     Note that ADDR is offset 3 bytes from the beginning of these
> +     sequences.  */
> +
> +  return !((buf[0] == 0x48) && (buf[1] == 0x33) && (buf[2] == 0xc0) /* xor */
> +	   && ((buf[3] == 0x90) || (buf[3] == 0xc3))                /* nop | ret */
> +	   && (buf[4] == 0x90));                                    /* nop */
> +}

Simply write:

amd64_dtrace_probe_is_enabled (struct gdbarch *gdbarch, CORE_ADDR addr)
{
   gdb_byte buf[5];
   read_code (addr - 3, buf, 5);

   return (memcmp (buf, amd64_dtrace_disabled_probe_sequence_1) != 0
           && memcmp (buf, amd64_dtrace_disabled_probe_sequence_2) != 0)
}

... etc.

Let the compiler worry about optimizing those memcmps if necessary.


> +  static int arg_reg_map[6] =

write:

  static const int arg_reg_map[] =


> +    {
> +      AMD64_RDI_REGNUM,  /* Arg 1.  */
> +      AMD64_RSI_REGNUM,  /* Arg 2.  */
> +      AMD64_RDX_REGNUM,  /* Arg 3.  */
> +      AMD64_RCX_REGNUM,  /* Arg 4.  */
> +      AMD64_R8_REGNUM,   /* Arg 5.  */
> +      AMD64_R9_REGNUM    /* Arg 6.  */
> +    };
+
+  /* DTrace probe arguments can be found on the ABI-defined places for
+     regular arguments at the current PC.  The probe abstraction
+     currently supports up to 12 arguments for probes.  */
+
+  if (narg < 6)
+    {

I'd suggest putting the arg_reg_map array within this if block.

Thanks,
Pedro Alves