[PATCH] gdb/x86: handle stap probe arguments in xmm registers

[PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Andrew Burgess]

On x86 machines with xmm register, and with recent versions of
systemtap (and gcc?), it can occur that stap probe arguments will be
placed into xmm registers.

I notice this happening on a current Fedora Rawhide install with the
following package versions installed:

  $ rpm -q glibc systemtap gcc
  glibc-2.35.9000-10.fc37.x86_64
  systemtap-4.7~pre16468670g9f253544-1.fc37.x86_64
  gcc-12.0.1-0.12.fc37.x86_64

If I check the probe data in libc, I see this:

  $ readelf -n /lib64/libc.so.6
  ...
  stapsdt              0x0000004d       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: pthread_start
    Location: 0x0000000000090ac3, Base: 0x00000000001c65c4, Semaphore: 0x0000000000000000
    Arguments: 8@%xmm1 8@1600(%rbx) 8@1608(%rbx)
  stapsdt              0x00000050       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: pthread_create
    Location: 0x00000000000912f1, Base: 0x00000000001c65c4, Semaphore: 0x0000000000000000
    Arguments: 8@%xmm1 8@%r13 8@8(%rsp) 8@16(%rsp)
  ...

Notice that for both of these probes, the first argument is a uint64_t
stored in the xmm1 register.

Unfortunately, if I try to use this probe within GDB, then I can't
view the first argument.  Here's an example session:

  $ gdb $(which gdb)
  (gdb) start
  ...
  (gdb) info probes stap  libc pthread_create
  ...
  (gdb) break *0x00007ffff729e2f1       # Use address of probe.
  (gdb) continue
  ...
  (gdb) p $_probe_arg0
  Invalid cast.

What's going wrong?  If I re-run my session, but this time use 'set
debug stap-expression 1', this is what I see:

  (gdb) set debug stap-expression 1
  (gdb) p $_probe_arg0
  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: xmm1
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: r13
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: UNOP_IND
    Operation: UNOP_CAST
     Operation: BINOP_ADD
      Operation: OP_LONG
       Type: long
       Constant: 0x0000000000000008
      Operation: OP_REGISTER
       String: rsp
     Type: uint64_t *
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: UNOP_IND
    Operation: UNOP_CAST
     Operation: BINOP_ADD
      Operation: OP_LONG
       Type: long
       Constant: 0x0000000000000010
      Operation: OP_REGISTER
       String: rsp
     Type: uint64_t *
   Type: uint64_t
  Invalid cast.
  (gdb)

The important bit is this:

  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: xmm1
   Type: uint64_t

Which is where we cast the xmm1 register to uint64_t.  And the final
piece of the puzzle is:

  (gdb) ptype $xmm1
  type = union vec128 {
      v8bf16 v8_bfloat16;
      v4f v4_float;
      v2d v2_double;
      v16i8 v16_int8;
      v8i16 v8_int16;
      v4i32 v4_int32;
      v2i64 v2_int64;
      uint128_t uint128;
  }

So, we are attempting to cast a union type to a scalar type, which is
not supporting in C/C++, and as a consequence GDB's expression
evaluator throws an error when we attempt to do this.

The first approach I considered for solving this problem was to try
and make use of gdbarch_stap_adjust_register.  We already have a
gdbarch method (gdbarch_stap_adjust_register) that allows us to tweak
the name of the register that we access.  Currently only x86
architectures use this to transform things like ax to eax in some
cases.

I wondered, what if we change gdbarch_stap_adjust_register to do more
than just change the register names?  What if this method instead
became gdbarch_stap_read_register.  This new method would return a
operation_up, and would take the register name, and the type we are
trying to read from the register, and return the operation that
actually reads the register.

The default implementation of this method would just use
user_reg_map_name_to_regnum, and then create a register_operation,
like we already do in stap_parse_register_operand.  But, for x86
architectures this method would fist possibly adjust the register
name, then do the default action to read the register.  Finally, for
x86 this method would spot when we were accessing an xmm register,
and, based on the type being pulled from the register, would extract
the correct field from the union.

The benefit of this approach is that it would work with the expression
types that GDB currently supports.  The draw back would be that this
approach would not be very generic.  We'd need code to handle each
sub-field size with an xmm register.  If other architectures started
using vector registers for probe arguments, those architectures would
have to create their own gdbarch_stap_read_register method.  And
finally, the type of the xmm registers comes from the type defined in
the target description, there's a risk that GDB might end up
hard-coding the names of type sub-fields, then if a target uses a
different target description, with different field names for xmm
registers, the stap probes would stop working.

And so, based on all the above draw backs, I rejected this first
approach.

My second plan involves adding a new expression type to GDB called
unop_extract_operation.  This new expression takes a value and a type,
during evaluation the value contents are fetched, and then a new value
is extracted from the value contents (based on type).  This is similar
to the following C expression:

  result_value = *((output_type *) &input_value);

Obviously we can't actually build this expression in this case, as the
input_value is in a register, but hopefully the above makes it clearer
what I'm trying to do.

The benefit of the new expression approach is that this code can be
shared across all architectures, and it doesn't care about sub-field
names within the union type.

The draw-backs that I see are potential future problems if arguments
are not stored within the least significant bytes of the register.
However if/when that becomes an issue we can adapt the
gdbarch_stap_read_register approach to allow architectures to control
how a value is extracted.

For testing, I've extended the existing gdb.base/stap-probe.exp test
to include a function that tries to force an argument into an xmm
register.  Obviously, that will only work on a x86 target, so I've
guarded the new function with an appropriate GCC define.  In the exp
script we use readelf to check if the probe exists, and is using the
xmm register.

If the probe doesn't exist then the associated tests are skipped.

If the probe exists, put isn't using the xmm register (which will
depend on systemtap/gcc versions), then again, the tests are skipped.

Otherwise, we can run the test.  I think the cost of running readelf
is pretty low, so I don't feel too bad making all the non-xmm targets
running this step.

I found that on a Fedora 35 install, with these packages installed, I
was able to run this test and have the probe argument be placed in an
xmm register:

  $ rpm -q systemtap gcc glibc
  systemtap-4.6-4.fc35.x86_64
  gcc-11.2.1-9.fc35.x86_64
  glibc-2.34-7.fc35.x86_64

Finally, as this patch adds a new operation type, then I need to
consider how to generate an agent expression for the new operation
type.

I have kicked the can down the road a bit on this.  In the function
stap_parse_register_operand, I only create a unop_extract_operation in
the case where the register type is non-scalar, this means that in
most cases I don't need to worry about generating an agent expression
at all.

In the xmm register case, when an unop_extract_operation will be
created, I have sketched out how the agent expression could be
handled, however, this code is currently not reached.  When we try to
generate the agent expression to place the xmm register on the stack,
GDB hits this error:

  (gdb) trace -probe-stap test:xmmreg
  Tracepoint 1 at 0x401166
  (gdb) actions
  Enter actions for tracepoint 1, one per line.
  End with a line saying just "end".
  >collect $_probe_arg0
  Value not scalar: cannot be an rvalue.

This is because GDB doesn't currently support placing non-scalar types
on the agent expression evaluation stack.  Solving this is clearly
related to the original problem, but feels a bit like a second
problem.  I'd like to get feedback on whether my approach to solving
the original problem is acceptable or not before I start looking at
how to handle xmm registers within agent expressions.
---
 gdb/ax-gdb.c                          |  19 ++++
 gdb/eval.c                            |  19 ++++
 gdb/expop.h                           |  31 +++++++
 gdb/stap-probe.c                      |  15 ++++
 gdb/std-operator.def                  |   9 ++
 gdb/testsuite/gdb.base/stap-probe.c   |  22 +++++
 gdb/testsuite/gdb.base/stap-probe.exp | 125 ++++++++++++++++++++++++++
 7 files changed, 240 insertions(+)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 0b12dc3a42f..be2063f2366 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1825,6 +1825,25 @@ unop_cast_operation::do_generate_ax (struct expression *exp,
 					std::get<1> (m_storage));
 }
 
+void
+unop_extract_operation::do_generate_ax (struct expression *exp,
+					struct agent_expr *ax,
+					struct axs_value *value,
+					struct type *cast_type)
+{
+  std::get<0> (m_storage)->generate_ax (exp, ax, value);
+
+  struct type *to_type = get_type ();
+
+  if (!is_scalar_type (to_type))
+    error (_("can't generate agent expression to extract non-scalar type"));
+
+  if (to_type->is_unsigned ())
+    gen_extend (ax, to_type);
+  else
+    gen_sign_extend (ax, to_type);
+}
+
 void
 unop_memval_operation::do_generate_ax (struct expression *exp,
 				       struct agent_expr *ax,
diff --git a/gdb/eval.c b/gdb/eval.c
index 6ced0b261e7..c4b14ddc72c 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2467,6 +2467,25 @@ array_operation::evaluate (struct type *expect_type,
   return value_array (tem2, tem3, argvec);
 }
 
+value *
+unop_extract_operation::evaluate (struct type *expect_type,
+				  struct expression *exp,
+				  enum noside noside)
+{
+  value *old_value = std::get<0> (m_storage)->evaluate (nullptr, exp, noside);
+  struct type *type = get_type ();
+
+  if (TYPE_LENGTH (type) > TYPE_LENGTH (value_type (old_value)))
+    error (_("length type is larger than the value type"));
+
+  if (noside == EVAL_AVOID_SIDE_EFFECTS)
+    return value_zero (type, not_lval);
+
+  struct value *result = allocate_value (type);
+  value_contents_copy (result, 0, old_value, 0, TYPE_LENGTH (type));
+  return result;
+}
+
 }
 
 \f
diff --git a/gdb/expop.h b/gdb/expop.h
index 58863b8f3ab..e9fd79ba2ec 100644
--- a/gdb/expop.h
+++ b/gdb/expop.h
@@ -1960,6 +1960,37 @@ class assign_modify_operation
     override;
 };
 
+/* Not a cast!  Extract a value of a given type from the contents of a
+   value.  The new value is extracted from the least significant bytes
+   of the old value.  The new value's type must be no bigger than the
+   old values type.  */
+class unop_extract_operation
+  : public maybe_constant_operation<operation_up, struct type *>
+{
+public:
+
+  using maybe_constant_operation::maybe_constant_operation;
+
+  value *evaluate (struct type *expect_type, struct expression *exp,
+		   enum noside noside) override;
+
+  enum exp_opcode opcode () const override
+  { return UNOP_EXTRACT; }
+
+  /* Return the type referenced by this object.  */
+  struct type *get_type () const
+  {
+    return std::get<1> (m_storage);
+  }
+
+protected:
+
+  void do_generate_ax (struct expression *exp,
+		       struct agent_expr *ax,
+		       struct axs_value *value,
+		       struct type *cast_type) override;
+};
+
 /* A type cast.  */
 class unop_cast_operation
   : public maybe_constant_operation<operation_up, struct type *>
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index c4b24df7564..436ff4dd590 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -822,6 +822,21 @@ stap_parse_register_operand (struct stap_parse_info *p)
 
   operation_up reg = make_operation<register_operation> (std::move (regname));
 
+  /* If the argument has been placed into a vector register then (for most
+     architectures), the type of this register will be a union of arrays.
+     As a result, attempting to cast from the register type to the scalar
+     argument type will not be possible (GDB will throw an error during
+     expression evaluation).
+
+     The solution is to extract the scalar type from the value contents of
+     the entire register value.  */
+  if (!is_scalar_type (gdbarch_register_type (gdbarch, regnum)))
+    {
+      gdb_assert (is_scalar_type (p->arg_type));
+      reg = make_operation<unop_extract_operation> (std::move (reg),
+						    p->arg_type);
+    }
+
   if (indirect_p)
     {
       if (disp_op != nullptr)
diff --git a/gdb/std-operator.def b/gdb/std-operator.def
index baaa5947e2c..5e6cad06379 100644
--- a/gdb/std-operator.def
+++ b/gdb/std-operator.def
@@ -187,6 +187,15 @@ OP (OP_STRING)
    and they must all match.  */
 OP (OP_ARRAY)
 
+/* UNOP_EXTRACT takes a value and a type, like a cast, but, instead of
+   casting the value to the given type, a new value (of the given
+   type) is extracted from the contents of the old value, starting
+   from the least significant byte.
+
+   It is invalid for the given type to be larger than the type of the
+   given value.  */
+OP (UNOP_EXTRACT)
+
 /* UNOP_CAST is followed by a type pointer in the next exp_element.
    With another UNOP_CAST at the end, this makes three exp_elements.
    It casts the value of the following subexpression.  */
diff --git a/gdb/testsuite/gdb.base/stap-probe.c b/gdb/testsuite/gdb.base/stap-probe.c
index 2d13d02c43a..d1482c35caf 100644
--- a/gdb/testsuite/gdb.base/stap-probe.c
+++ b/gdb/testsuite/gdb.base/stap-probe.c
@@ -29,6 +29,8 @@ __extension__ unsigned short test_m4_semaphore __attribute__ ((unused)) __attrib
 __extension__ unsigned short test_pstr_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
 
 __extension__ unsigned short test_ps_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
+
+__extension__ unsigned short test_xmmreg_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
 #else
 
 int relocation_marker __attribute__ ((unused));
@@ -90,6 +92,24 @@ pstr (int val)
   return val == 0 ? a : b;
 }
 
+#ifdef __SSE2__
+static const char * __attribute__((noinline))
+use_xmm_reg (int val)
+{
+  volatile register int val_in_reg asm ("xmm0") = val;
+
+  STAP_PROBE1 (test, xmmreg, val_in_reg);
+
+  return val == 0 ? "xxx" : "yyy";
+}
+#else
+static const char * __attribute__((noinline)) ATTRIBUTE_NOCLONE
+use_xmm_reg (int val)
+{
+  /* Nothing.  */
+}
+#endif /* __SSE2__ */
+
 static void
 m4 (const struct funcs *fs, int v)
 {
@@ -111,5 +131,7 @@ main()
   m4 (&fs, 0);
   m4 (&fs, 1);
 
+  use_xmm_reg (0x1234);
+
   return 0; /* last break here */
 }
diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
index e86c7897b93..b0b690a4dcb 100644
--- a/gdb/testsuite/gdb.base/stap-probe.exp
+++ b/gdb/testsuite/gdb.base/stap-probe.exp
@@ -15,6 +15,98 @@
 
 standard_testfile
 
+# Count the number of probes of TYPE (either 'stap' or 'dtrace'),
+# from provider matching PROVIDER, with a name matching NAME, and from
+# an objec file matching OBJECT.
+#
+# The OBJECT is optional, in which case all objects will be matched.
+#
+# If any error condition is detected, then perror is called, and -1
+# returned.
+#
+# Otherwise, returns an integer, 0 or greater.
+proc gdb_count_probes { type provider name { object "" }} {
+    set cmd "info probes ${type} ${provider} ${name}"
+    if { $object != "" } {
+	set cmd "$cmd ${object}"
+    }
+
+    set probe_count 0
+    set no_probes_line false
+    gdb_test_multiple $cmd "" {
+	-re "^$cmd\r\n" {
+	    exp_continue
+	}
+	-re "^Type\\s+Provider\\s+Name\\s+Where\\s+Semaphore\\s+Object\\s*\r\n" {
+	    exp_continue
+	}
+	-re "^\\s*\r\n" {
+	    exp_continue
+	}
+	-re "^stap\[^\r\n\]+\r\n" {
+	    incr probe_count
+	    exp_continue
+	}
+	-re "^dtrace\[^\r\n\]+\r\n" {
+	    incr probe_count
+	    exp_continue
+	}
+	-re "^No probes matched\\.\r\n" {
+	    set no_probes_line true
+	    exp_continue
+	}
+	-re "^$::gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+
+    if { [expr $no_probes_line && $probe_count > 0] \
+	     || [expr !$no_probes_line && $probe_count == 0] } {
+	perror "Mismatch between no probes found line, and probes count"
+	return -1
+    }
+
+    return $probe_count
+}
+
+proc check_for_usable_xmm0_probe { binfile } {
+    set readelf_program [gdb_find_readelf]
+    set binfile [standard_output_file $binfile]
+    set command "exec $readelf_program -n $binfile"
+    verbose -log "command is $command"
+    set result [catch $command output]
+    verbose -log "result is $result"
+    verbose -log "output is $output"
+
+    # We don't actually check RESULT.  Sometimes readelf gives
+    # warnings about gaps in some of the notes data.  This is
+    # unrelated to the staps probes, but still causes readelf to exit
+    # with non-zero status.
+    #
+    # Instead, just check the output.  If readelf failed to run then
+    # the output will be empty, and the following regexps will fail to
+    # match.
+
+    # First, look for the xmmreg probe, and if we find it, grab the
+    # argument string.
+    if ![regexp {\n\s+Provider: test\n\s+Name: xmmreg\n[^\n]+\n\s+Arguments: ([^\n]+)\n} $output ignore arguments] {
+	verbose -log "APB: Couldn't find probe at all"
+	return false
+    }
+
+    verbose -log "APB: Matched on '$ignore'"
+    verbose -log "APB: arguments: '$arguments'"
+
+    # Check the the argument string mentions xmm0.
+    if ![regexp {@%?xmm0} $arguments] {
+	verbose -log "APB: Prove doesn't use xmm0 register"
+	return false
+    }
+
+    # Success!  We have a probe that uses xmm0 for an argument.
+    return true
+}
+
 # Run the tests.  We run the tests two different ways: once with a
 # plain probe, and once with a probe that has an associated semaphore.
 # This returns -1 on failure to compile or start, 0 otherwise.
@@ -119,6 +211,24 @@ proc stap_test {exec_name {args ""}} {
     " = $hex .This is another test message.*" \
     "print \$_probe_arg1 for probe ps"
 
+    # Check the probe is using the xmm0 register.
+    if [check_for_usable_xmm0_probe $exec_name] {
+
+	delete_breakpoints
+	if {[runto "-pstap test:xmmreg"]} {
+	    pass "run to -pstap test:xmmreg"
+	} else {
+	    fail "run to -pstap test:xmmreg"
+	}
+
+	gdb_test "print \$_probe_argc" " = 1" \
+	    "print \$_probe_argc for probe xmmreg"
+	gdb_test "print/x \$_probe_arg0" " = 0x1234" \
+	    "check \$_probe_arg0 for probe xmmreg"
+    } else {
+	unsupported "print probe argument from \$xmm0 register"
+    }
+
     return 0
 }
 
@@ -185,6 +295,21 @@ proc stap_test_no_debuginfo {exec_name {args ""}} {
     " = $hex .This is another test message.*" \
     "print \$_probe_arg1 for probe ps"
 
+    # Reinit GDB, set a breakpoint on probe ps.
+    if { [gdb_count_probes stap test xmmreg] > 0 } {
+	delete_breakpoints
+	if {[runto "-pstap test:xmmreg"]} {
+	    pass "run to -pstap test:xmmreg"
+	} else {
+	    fail "run to -pstap test:xmmreg"
+	}
+
+	gdb_test "print \$_probe_argc" " = 1" \
+	    "print \$_probe_argc for probe xmmreg"
+	gdb_test "print/x \$_probe_arg0" " = 0x1234" \
+	    "check \$_probe_arg0 for probe xmmreg"
+    }
+
     return 0
 }
 
-- 
2.25.4

RE: [PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Metzger, Markus T]

Hello Andrew,

>On x86 machines with xmm register, and with recent versions of
>systemtap (and gcc?), it can occur that stap probe arguments will be
>placed into xmm registers.

I ran into the same problem with solib probes

  stapsdt              0x00000042       NT_STAPSDT (SystemTap probe descriptors)
    Provider: rtld
    Name: unmap_complete
    Location: 0x0000000000002d88, Base: 0x000000000002ecdb, Semaphore: 0x0000000000000000
    Arguments: -8@-112(%rbp) 8@%xmm1

which results in

	Invalid cast.
	warning: Probes-based dynamic linker interface failed.
	Reverting to original interface.

on dlclose() and can be observed with gdb.base/unload.exp.  It doesn't lead
to a FAIL but the test could easily be extended to catch this.

I extended gdb_continue_to_breakpoint to catch this case for another test I
wrote for linker namespaces.

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index a35d08a05de..ab7058121e5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -729,6 +729,12 @@ proc gdb_continue_to_breakpoint {name {location_pattern .*}} {
 
     set kfail_pattern "Process record does not support instruction 0xfae64 at.*"
     gdb_test_multiple "continue" $full_name {
+       -re "Corrupted shared library list.*$gdb_prompt $" {
+           fail "$full_name: shared library list corrupted"
+       }
+       -re "Invalid cast\.\r\nwarning: Probes-based dynamic linker interface failed.*$gdb_prompt $" {
+           fail "$full_name: probes interface failure"
+       }
        -re "(?:Breakpoint|Temporary breakpoint) .* (at|in) $location_pattern\r\n$gdb_prompt $" {
            pass $full_name
        }


>My second plan involves adding a new expression type to GDB called
>unop_extract_operation.  This new expression takes a value and a type,
>during evaluation the value contents are fetched, and then a new value
>is extracted from the value contents (based on type).  This is similar
>to the following C expression:
>
>  result_value = *((output_type *) &input_value);
>
>Obviously we can't actually build this expression in this case, as the
>input_value is in a register, but hopefully the above makes it clearer
>what I'm trying to do.

The extract approach looks good to me and I can confirm that your patch
fixes the issue I've seen with dlclose() and the probe interface.

I was about to try changing the register operator to provide the
expected type but then I started wondering why we would want to
assign a type to registers, at all.  A register provides storage but
the actual interpretation of that storage is left to the instructions
that operate on the register and, as we can see here, compilers
may use that storage in novel ways.

I see how it might be nice to have some default display type for
printing values in 'info reg'.  But also that has become a challenge
with vector registers where we interpret the bits as vectors of
different length and element type.

Maybe we should leave it completely to the command that prints
register values (e.g. 'info reg') to define the type in which to interpret
the bits (e.g. via a set of options) and leave register values themselves
untyped.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Andrew Burgess]

"Metzger, Markus T via Gdb-patches" <gdb-patches@sourceware.org> writes:

> Hello Andrew,
>
>>On x86 machines with xmm register, and with recent versions of
>>systemtap (and gcc?), it can occur that stap probe arguments will be
>>placed into xmm registers.
>
> I ran into the same problem with solib probes
>
>   stapsdt              0x00000042       NT_STAPSDT (SystemTap probe descriptors)
>     Provider: rtld
>     Name: unmap_complete
>     Location: 0x0000000000002d88, Base: 0x000000000002ecdb, Semaphore: 0x0000000000000000
>     Arguments: -8@-112(%rbp) 8@%xmm1
>
> which results in
>
> 	Invalid cast.
> 	warning: Probes-based dynamic linker interface failed.
> 	Reverting to original interface.
>
> on dlclose() and can be observed with gdb.base/unload.exp.  It doesn't lead
> to a FAIL but the test could easily be extended to catch this.
>
> I extended gdb_continue_to_breakpoint to catch this case for another test I
> wrote for linker namespaces.
>
> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
> index a35d08a05de..ab7058121e5 100644
> --- a/gdb/testsuite/lib/gdb.exp
> +++ b/gdb/testsuite/lib/gdb.exp
> @@ -729,6 +729,12 @@ proc gdb_continue_to_breakpoint {name {location_pattern .*}} {
>  
>      set kfail_pattern "Process record does not support instruction 0xfae64 at.*"
>      gdb_test_multiple "continue" $full_name {
> +       -re "Corrupted shared library list.*$gdb_prompt $" {
> +           fail "$full_name: shared library list corrupted"
> +       }
> +       -re "Invalid cast\.\r\nwarning: Probes-based dynamic linker interface failed.*$gdb_prompt $" {
> +           fail "$full_name: probes interface failure"
> +       }
>         -re "(?:Breakpoint|Temporary breakpoint) .* (at|in) $location_pattern\r\n$gdb_prompt $" {
>             pass $full_name
>         }

I wonder if these checks would be better added within gdb_test_multiple
itself? 

>
>
>>My second plan involves adding a new expression type to GDB called
>>unop_extract_operation.  This new expression takes a value and a type,
>>during evaluation the value contents are fetched, and then a new value
>>is extracted from the value contents (based on type).  This is similar
>>to the following C expression:
>>
>>  result_value = *((output_type *) &input_value);
>>
>>Obviously we can't actually build this expression in this case, as the
>>input_value is in a register, but hopefully the above makes it clearer
>>what I'm trying to do.
>
> The extract approach looks good to me and I can confirm that your patch
> fixes the issue I've seen with dlclose() and the probe interface.

That's great news.

>
> I was about to try changing the register operator to provide the
> expected type but then I started wondering why we would want to
> assign a type to registers, at all.  A register provides storage but
> the actual interpretation of that storage is left to the instructions
> that operate on the register and, as we can see here, compilers
> may use that storage in novel ways.
>
> I see how it might be nice to have some default display type for
> printing values in 'info reg'.  But also that has become a challenge
> with vector registers where we interpret the bits as vectors of
> different length and element type.
>
> Maybe we should leave it completely to the command that prints
> register values (e.g. 'info reg') to define the type in which to interpret
> the bits (e.g. via a set of options) and leave register values themselves
> untyped.

I think I'd need to understand more about how the proposed UI would
work, the current mechanism has the advantage of being pretty intuitive
(I think) for users.  I guess if the vector registers were presented as
a single scalar and the user had to cast to the vector type, or set some
options, I fear this might be harder to figure out.

Thanks,
Andrew

RE: [PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Metzger, Markus T]

Hello Andrew,

>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index a35d08a05de..ab7058121e5 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -729,6 +729,12 @@ proc gdb_continue_to_breakpoint {name
>{location_pattern .*}} {
>>
>>      set kfail_pattern "Process record does not support instruction 0xfae64 at.*"
>>      gdb_test_multiple "continue" $full_name {
>> +       -re "Corrupted shared library list.*$gdb_prompt $" {
>> +           fail "$full_name: shared library list corrupted"
>> +       }
>> +       -re "Invalid cast\.\r\nwarning: Probes-based dynamic linker interface
>failed.*$gdb_prompt $" {
>> +           fail "$full_name: probes interface failure"
>> +       }
>>         -re "(?:Breakpoint|Temporary breakpoint) .* (at|in)
>$location_pattern\r\n$gdb_prompt $" {
>>             pass $full_name
>>         }
>
>I wonder if these checks would be better added within gdb_test_multiple
>itself?

Good idea.  This way, they also cover gdb.base/unload.exp.


>>>My second plan involves adding a new expression type to GDB called
>>>unop_extract_operation.  This new expression takes a value and a type,
>>>during evaluation the value contents are fetched, and then a new value
>>>is extracted from the value contents (based on type).  This is similar
>>>to the following C expression:
>>>
>>>  result_value = *((output_type *) &input_value);
>>>
>>>Obviously we can't actually build this expression in this case, as the
>>>input_value is in a register, but hopefully the above makes it clearer
>>>what I'm trying to do.
>>
>> The extract approach looks good to me and I can confirm that your patch
>> fixes the issue I've seen with dlclose() and the probe interface.
>
>That's great news.
>
>>
>> I was about to try changing the register operator to provide the
>> expected type but then I started wondering why we would want to
>> assign a type to registers, at all.  A register provides storage but
>> the actual interpretation of that storage is left to the instructions
>> that operate on the register and, as we can see here, compilers
>> may use that storage in novel ways.
>>
>> I see how it might be nice to have some default display type for
>> printing values in 'info reg'.  But also that has become a challenge
>> with vector registers where we interpret the bits as vectors of
>> different length and element type.
>>
>> Maybe we should leave it completely to the command that prints
>> register values (e.g. 'info reg') to define the type in which to interpret
>> the bits (e.g. via a set of options) and leave register values themselves
>> untyped.
>
>I think I'd need to understand more about how the proposed UI would
>work, the current mechanism has the advantage of being pretty intuitive
>(I think) for users.  I guess if the vector registers were presented as
>a single scalar and the user had to cast to the vector type, or set some
>options, I fear this might be harder to figure out.

For vector registers users already need to know the element type and vector
length they are interested in.  Today, they get it all at once

(gdb) i reg xmm0
xmm0           {v8_bfloat16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {0x0 <repeats 16 times>}, v8_int16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_int32 = {0x0, 0x0, 0x0, 0x0}, v2_int64 = {0x0, 0x0}, uint128 = 0x0}

or

(gdb) set print pretty on 
(gdb) i reg xmm0
xmm0           {
  v8_bfloat16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  v4_float = {0x0, 0x0, 0x0, 0x0},
  v2_double = {0x0, 0x0},
  v16_int8 = {0x0 <repeats 16 times>},
  v8_int16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
  v4_int32 = {0x0, 0x0, 0x0, 0x0},
  v2_int64 = {0x0, 0x0},
  uint128 = 0x0
}

or they select from among the options

(gdb) p/x $xmm0.v4_int32
$1 = {0x0, 0x0, 0x0, 0x0}

One could imagine formatting options similar to the 'x' /FMT options, although
without the repeat count, which is implicit in the register size.  E.g.

(gdb) i reg /xw xmm0
{0x0, 0x0, 0x0, 0x0}

This would also work for general purpose registers, which may contain vectors,
too, e.g.

(gdb) i reg /cb rax
{104 'h', 101 'e', 108 'l', 108 'l', 111 'o', 32 ' ', 119 'w', 111 'o'}

Convenience variables (e.g. $xmm0 or $rax) are variables and would hence still be
typed using the type defined in the feature xml.

Registers would no longer be typed and would be printed as a stream of bits
in hex unless specified otherwise.

OTOH changing the UI is always difficult and there's bound to be someone who
doesn't like it and someone whose scripts all got broken with that change.

Also, we would need to cast untyped registers to some type for any operation like
adding stack offsets as in 8@1600(%rbx).  It is arguably cleaner as the type now
comes from a particular interpretation of the register's content rather than from
the register itself, but that's maybe hair-splitting.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Andrew Burgess]

"Metzger, Markus T via Gdb-patches" <gdb-patches@sourceware.org> writes:

> Hello Andrew,
>
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index a35d08a05de..ab7058121e5 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -729,6 +729,12 @@ proc gdb_continue_to_breakpoint {name
>>{location_pattern .*}} {
>>>
>>>      set kfail_pattern "Process record does not support instruction 0xfae64 at.*"
>>>      gdb_test_multiple "continue" $full_name {
>>> +       -re "Corrupted shared library list.*$gdb_prompt $" {
>>> +           fail "$full_name: shared library list corrupted"
>>> +       }
>>> +       -re "Invalid cast\.\r\nwarning: Probes-based dynamic linker interface
>>failed.*$gdb_prompt $" {
>>> +           fail "$full_name: probes interface failure"
>>> +       }
>>>         -re "(?:Breakpoint|Temporary breakpoint) .* (at|in)
>>$location_pattern\r\n$gdb_prompt $" {
>>>             pass $full_name
>>>         }
>>
>>I wonder if these checks would be better added within gdb_test_multiple
>>itself?
>
> Good idea.  This way, they also cover gdb.base/unload.exp.
>
>
>>>>My second plan involves adding a new expression type to GDB called
>>>>unop_extract_operation.  This new expression takes a value and a type,
>>>>during evaluation the value contents are fetched, and then a new value
>>>>is extracted from the value contents (based on type).  This is similar
>>>>to the following C expression:
>>>>
>>>>  result_value = *((output_type *) &input_value);
>>>>
>>>>Obviously we can't actually build this expression in this case, as the
>>>>input_value is in a register, but hopefully the above makes it clearer
>>>>what I'm trying to do.
>>>
>>> The extract approach looks good to me and I can confirm that your patch
>>> fixes the issue I've seen with dlclose() and the probe interface.
>>
>>That's great news.
>>
>>>
>>> I was about to try changing the register operator to provide the
>>> expected type but then I started wondering why we would want to
>>> assign a type to registers, at all.  A register provides storage but
>>> the actual interpretation of that storage is left to the instructions
>>> that operate on the register and, as we can see here, compilers
>>> may use that storage in novel ways.
>>>
>>> I see how it might be nice to have some default display type for
>>> printing values in 'info reg'.  But also that has become a challenge
>>> with vector registers where we interpret the bits as vectors of
>>> different length and element type.
>>>
>>> Maybe we should leave it completely to the command that prints
>>> register values (e.g. 'info reg') to define the type in which to interpret
>>> the bits (e.g. via a set of options) and leave register values themselves
>>> untyped.
>>
>>I think I'd need to understand more about how the proposed UI would
>>work, the current mechanism has the advantage of being pretty intuitive
>>(I think) for users.  I guess if the vector registers were presented as
>>a single scalar and the user had to cast to the vector type, or set some
>>options, I fear this might be harder to figure out.
>
> For vector registers users already need to know the element type and vector
> length they are interested in.  Today, they get it all at once
>
> (gdb) i reg xmm0
> xmm0           {v8_bfloat16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_float = {0x0, 0x0, 0x0, 0x0}, v2_double = {0x0, 0x0}, v16_int8 = {0x0 <repeats 16 times>}, v8_int16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, v4_int32 = {0x0, 0x0, 0x0, 0x0}, v2_int64 = {0x0, 0x0}, uint128 = 0x0}
>
> or
>
> (gdb) set print pretty on 
> (gdb) i reg xmm0
> xmm0           {
>   v8_bfloat16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
>   v4_float = {0x0, 0x0, 0x0, 0x0},
>   v2_double = {0x0, 0x0},
>   v16_int8 = {0x0 <repeats 16 times>},
>   v8_int16 = {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0},
>   v4_int32 = {0x0, 0x0, 0x0, 0x0},
>   v2_int64 = {0x0, 0x0},
>   uint128 = 0x0
> }
>
> or they select from among the options
>
> (gdb) p/x $xmm0.v4_int32
> $1 = {0x0, 0x0, 0x0, 0x0}
>
> One could imagine formatting options similar to the 'x' /FMT options, although
> without the repeat count, which is implicit in the register size.  E.g.
>
> (gdb) i reg /xw xmm0
> {0x0, 0x0, 0x0, 0x0}
>
> This would also work for general purpose registers, which may contain vectors,
> too, e.g.
>
> (gdb) i reg /cb rax
> {104 'h', 101 'e', 108 'l', 108 'l', 111 'o', 32 ' ', 119 'w', 111 'o'}
>
> Convenience variables (e.g. $xmm0 or $rax) are variables and would hence still be
> typed using the type defined in the feature xml.

I always find it really interesting how differeny people use tools in
such different ways.

I never use 'info registers' for viewing a single register, it's always
'print $REG' in that case.  I only ever use 'info registers' for viewing
whole register sets, usually if I'm "searching" for a particular varlue
in an unknown register.

Also I've never thought of $REG as a convenience variable, just as
overloaded sytex, i.e. we have $REG and $VARIABLE.

As I said, doesn't really make a difference, I just find it really
interesting.

>
> Registers would no longer be typed and would be printed as a stream of bits
> in hex unless specified otherwise.
>
> OTOH changing the UI is always difficult and there's bound to be someone who
> doesn't like it and someone whose scripts all got broken with that
> change.

I'm not going to say I don't like it, but I'm certainly not convinced yet.

>
> Also, we would need to cast untyped registers to some type for any operation like
> adding stack offsets as in 8@1600(%rbx).  It is arguably cleaner as the type now
> comes from a particular interpretation of the register's content rather than from
> the register itself, but that's maybe hair-splitting.

I don't know what you mean by hair-splitting in this case.  This seems
to be the biggest drawback from this proposal, and for me, I think this
would be a huge step backwards in user experience.

Thanks,
Andrew

RE: [PATCH] gdb/x86: handle stap probe arguments in xmm registers

[Metzger, Markus T]

Hello Andrew,

>> Also, we would need to cast untyped registers to some type for any operation
>like
>> adding stack offsets as in 8@1600(%rbx).  It is arguably cleaner as the type now
>> comes from a particular interpretation of the register's content rather than
>from
>> the register itself, but that's maybe hair-splitting.
>
>I don't know what you mean by hair-splitting in this case.  This seems
>to be the biggest drawback from this proposal, and for me, I think this
>would be a huge step backwards in user experience.

I was referring to a non-user-visible internal-only change where registers would be
untyped and the type would later be added as the register's content get interpreted.
In the 8@1600(%rbx) example this would look like:

  Operation: UNOP_CAST
    Operation: UNOP_IND
      Operation: UNOP_CAST
        Operation: BINOP_ADD
          Operation: OP_LONG
            Type: long
            Constant: 0x0000000000000640
          Operation: UNOP_CAST
            Operation: OP_REGISTER
              String: rbp
            Type: uint8_t *
        Type: uint64_t *
    Type: uint64_t


In the end, this is very similar to the UNOP_EXTRACT you are proposing.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[PATCHv2] gdb/x86: handle stap probe arguments in xmm registers

[Andrew Burgess]

Changes since v1:

 - I realised that I didn't need to special case
   EVAL_AVOID_SDE_EFFECTS in unop_extract_operation::evaluate, so I
   removed that check.

Thanks,
Andrew


---

On x86 machines with xmm register, and with recent versions of
systemtap (and gcc?), it can occur that stap probe arguments will be
placed into xmm registers.

I notice this happening on a current Fedora Rawhide install with the
following package versions installed:

  $ rpm -q glibc systemtap gcc
  glibc-2.35.9000-10.fc37.x86_64
  systemtap-4.7~pre16468670g9f253544-1.fc37.x86_64
  gcc-12.0.1-0.12.fc37.x86_64

If I check the probe data in libc, I see this:

  $ readelf -n /lib64/libc.so.6
  ...
  stapsdt              0x0000004d       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: pthread_start
    Location: 0x0000000000090ac3, Base: 0x00000000001c65c4, Semaphore: 0x0000000000000000
    Arguments: 8@%xmm1 8@1600(%rbx) 8@1608(%rbx)
  stapsdt              0x00000050       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: pthread_create
    Location: 0x00000000000912f1, Base: 0x00000000001c65c4, Semaphore: 0x0000000000000000
    Arguments: 8@%xmm1 8@%r13 8@8(%rsp) 8@16(%rsp)
  ...

Notice that for both of these probes, the first argument is a uint64_t
stored in the xmm1 register.

Unfortunately, if I try to use this probe within GDB, then I can't
view the first argument.  Here's an example session:

  $ gdb $(which gdb)
  (gdb) start
  ...
  (gdb) info probes stap  libc pthread_create
  ...
  (gdb) break *0x00007ffff729e2f1       # Use address of probe.
  (gdb) continue
  ...
  (gdb) p $_probe_arg0
  Invalid cast.

What's going wrong?  If I re-run my session, but this time use 'set
debug stap-expression 1', this is what I see:

  (gdb) set debug stap-expression 1
  (gdb) p $_probe_arg0
  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: xmm1
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: r13
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: UNOP_IND
    Operation: UNOP_CAST
     Operation: BINOP_ADD
      Operation: OP_LONG
       Type: long
       Constant: 0x0000000000000008
      Operation: OP_REGISTER
       String: rsp
     Type: uint64_t *
   Type: uint64_t
  Operation: UNOP_CAST
   Operation: UNOP_IND
    Operation: UNOP_CAST
     Operation: BINOP_ADD
      Operation: OP_LONG
       Type: long
       Constant: 0x0000000000000010
      Operation: OP_REGISTER
       String: rsp
     Type: uint64_t *
   Type: uint64_t
  Invalid cast.
  (gdb)

The important bit is this:

  Operation: UNOP_CAST
   Operation: OP_REGISTER
    String: xmm1
   Type: uint64_t

Which is where we cast the xmm1 register to uint64_t.  And the final
piece of the puzzle is:

  (gdb) ptype $xmm1
  type = union vec128 {
      v8bf16 v8_bfloat16;
      v4f v4_float;
      v2d v2_double;
      v16i8 v16_int8;
      v8i16 v8_int16;
      v4i32 v4_int32;
      v2i64 v2_int64;
      uint128_t uint128;
  }

So, we are attempting to cast a union type to a scalar type, which is
not supporting in C/C++, and as a consequence GDB's expression
evaluator throws an error when we attempt to do this.

The first approach I considered for solving this problem was to try
and make use of gdbarch_stap_adjust_register.  We already have a
gdbarch method (gdbarch_stap_adjust_register) that allows us to tweak
the name of the register that we access.  Currently only x86
architectures use this to transform things like ax to eax in some
cases.

I wondered, what if we change gdbarch_stap_adjust_register to do more
than just change the register names?  What if this method instead
became gdbarch_stap_read_register.  This new method would return a
operation_up, and would take the register name, and the type we are
trying to read from the register, and return the operation that
actually reads the register.

The default implementation of this method would just use
user_reg_map_name_to_regnum, and then create a register_operation,
like we already do in stap_parse_register_operand.  But, for x86
architectures this method would fist possibly adjust the register
name, then do the default action to read the register.  Finally, for
x86 this method would spot when we were accessing an xmm register,
and, based on the type being pulled from the register, would extract
the correct field from the union.

The benefit of this approach is that it would work with the expression
types that GDB currently supports.  The draw back would be that this
approach would not be very generic.  We'd need code to handle each
sub-field size with an xmm register.  If other architectures started
using vector registers for probe arguments, those architectures would
have to create their own gdbarch_stap_read_register method.  And
finally, the type of the xmm registers comes from the type defined in
the target description, there's a risk that GDB might end up
hard-coding the names of type sub-fields, then if a target uses a
different target description, with different field names for xmm
registers, the stap probes would stop working.

And so, based on all the above draw backs, I rejected this first
approach.

My second plan involves adding a new expression type to GDB called
unop_extract_operation.  This new expression takes a value and a type,
during evaluation the value contents are fetched, and then a new value
is extracted from the value contents (based on type).  This is similar
to the following C expression:

  result_value = *((output_type *) &input_value);

Obviously we can't actually build this expression in this case, as the
input_value is in a register, but hopefully the above makes it clearer
what I'm trying to do.

The benefit of the new expression approach is that this code can be
shared across all architectures, and it doesn't care about sub-field
names within the union type.

The draw-backs that I see are potential future problems if arguments
are not stored within the least significant bytes of the register.
However if/when that becomes an issue we can adapt the
gdbarch_stap_read_register approach to allow architectures to control
how a value is extracted.

For testing, I've extended the existing gdb.base/stap-probe.exp test
to include a function that tries to force an argument into an xmm
register.  Obviously, that will only work on a x86 target, so I've
guarded the new function with an appropriate GCC define.  In the exp
script we use readelf to check if the probe exists, and is using the
xmm register.

If the probe doesn't exist then the associated tests are skipped.

If the probe exists, put isn't using the xmm register (which will
depend on systemtap/gcc versions), then again, the tests are skipped.

Otherwise, we can run the test.  I think the cost of running readelf
is pretty low, so I don't feel too bad making all the non-xmm targets
running this step.

I found that on a Fedora 35 install, with these packages installed, I
was able to run this test and have the probe argument be placed in an
xmm register:

  $ rpm -q systemtap gcc glibc
  systemtap-4.6-4.fc35.x86_64
  gcc-11.2.1-9.fc35.x86_64
  glibc-2.34-7.fc35.x86_64

Finally, as this patch adds a new operation type, then I need to
consider how to generate an agent expression for the new operation
type.

I have kicked the can down the road a bit on this.  In the function
stap_parse_register_operand, I only create a unop_extract_operation in
the case where the register type is non-scalar, this means that in
most cases I don't need to worry about generating an agent expression
at all.

In the xmm register case, when an unop_extract_operation will be
created, I have sketched out how the agent expression could be
handled, however, this code is currently not reached.  When we try to
generate the agent expression to place the xmm register on the stack,
GDB hits this error:

  (gdb) trace -probe-stap test:xmmreg
  Tracepoint 1 at 0x401166
  (gdb) actions
  Enter actions for tracepoint 1, one per line.
  End with a line saying just "end".
  >collect $_probe_arg0
  Value not scalar: cannot be an rvalue.

This is because GDB doesn't currently support placing non-scalar types
on the agent expression evaluation stack.  Solving this is clearly
related to the original problem, but feels a bit like a second
problem.  I'd like to get feedback on whether my approach to solving
the original problem is acceptable or not before I start looking at
how to handle xmm registers within agent expressions.
---
 gdb/ax-gdb.c                          |  19 ++++
 gdb/eval.c                            |  16 ++++
 gdb/expop.h                           |  31 +++++++
 gdb/stap-probe.c                      |  15 ++++
 gdb/std-operator.def                  |   9 ++
 gdb/testsuite/gdb.base/stap-probe.c   |  22 +++++
 gdb/testsuite/gdb.base/stap-probe.exp | 125 ++++++++++++++++++++++++++
 7 files changed, 237 insertions(+)

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index 0b12dc3a42f..be2063f2366 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -1825,6 +1825,25 @@ unop_cast_operation::do_generate_ax (struct expression *exp,
 					std::get<1> (m_storage));
 }
 
+void
+unop_extract_operation::do_generate_ax (struct expression *exp,
+					struct agent_expr *ax,
+					struct axs_value *value,
+					struct type *cast_type)
+{
+  std::get<0> (m_storage)->generate_ax (exp, ax, value);
+
+  struct type *to_type = get_type ();
+
+  if (!is_scalar_type (to_type))
+    error (_("can't generate agent expression to extract non-scalar type"));
+
+  if (to_type->is_unsigned ())
+    gen_extend (ax, to_type);
+  else
+    gen_sign_extend (ax, to_type);
+}
+
 void
 unop_memval_operation::do_generate_ax (struct expression *exp,
 				       struct agent_expr *ax,
diff --git a/gdb/eval.c b/gdb/eval.c
index 6ced0b261e7..4a4837d85f6 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -2467,6 +2467,22 @@ array_operation::evaluate (struct type *expect_type,
   return value_array (tem2, tem3, argvec);
 }
 
+value *
+unop_extract_operation::evaluate (struct type *expect_type,
+				  struct expression *exp,
+				  enum noside noside)
+{
+  value *old_value = std::get<0> (m_storage)->evaluate (nullptr, exp, noside);
+  struct type *type = get_type ();
+
+  if (TYPE_LENGTH (type) > TYPE_LENGTH (value_type (old_value)))
+    error (_("length type is larger than the value type"));
+
+  struct value *result = allocate_value (type);
+  value_contents_copy (result, 0, old_value, 0, TYPE_LENGTH (type));
+  return result;
+}
+
 }
 
 \f
diff --git a/gdb/expop.h b/gdb/expop.h
index 58863b8f3ab..e9fd79ba2ec 100644
--- a/gdb/expop.h
+++ b/gdb/expop.h
@@ -1960,6 +1960,37 @@ class assign_modify_operation
     override;
 };
 
+/* Not a cast!  Extract a value of a given type from the contents of a
+   value.  The new value is extracted from the least significant bytes
+   of the old value.  The new value's type must be no bigger than the
+   old values type.  */
+class unop_extract_operation
+  : public maybe_constant_operation<operation_up, struct type *>
+{
+public:
+
+  using maybe_constant_operation::maybe_constant_operation;
+
+  value *evaluate (struct type *expect_type, struct expression *exp,
+		   enum noside noside) override;
+
+  enum exp_opcode opcode () const override
+  { return UNOP_EXTRACT; }
+
+  /* Return the type referenced by this object.  */
+  struct type *get_type () const
+  {
+    return std::get<1> (m_storage);
+  }
+
+protected:
+
+  void do_generate_ax (struct expression *exp,
+		       struct agent_expr *ax,
+		       struct axs_value *value,
+		       struct type *cast_type) override;
+};
+
 /* A type cast.  */
 class unop_cast_operation
   : public maybe_constant_operation<operation_up, struct type *>
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index c4b24df7564..436ff4dd590 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -822,6 +822,21 @@ stap_parse_register_operand (struct stap_parse_info *p)
 
   operation_up reg = make_operation<register_operation> (std::move (regname));
 
+  /* If the argument has been placed into a vector register then (for most
+     architectures), the type of this register will be a union of arrays.
+     As a result, attempting to cast from the register type to the scalar
+     argument type will not be possible (GDB will throw an error during
+     expression evaluation).
+
+     The solution is to extract the scalar type from the value contents of
+     the entire register value.  */
+  if (!is_scalar_type (gdbarch_register_type (gdbarch, regnum)))
+    {
+      gdb_assert (is_scalar_type (p->arg_type));
+      reg = make_operation<unop_extract_operation> (std::move (reg),
+						    p->arg_type);
+    }
+
   if (indirect_p)
     {
       if (disp_op != nullptr)
diff --git a/gdb/std-operator.def b/gdb/std-operator.def
index baaa5947e2c..5e6cad06379 100644
--- a/gdb/std-operator.def
+++ b/gdb/std-operator.def
@@ -187,6 +187,15 @@ OP (OP_STRING)
    and they must all match.  */
 OP (OP_ARRAY)
 
+/* UNOP_EXTRACT takes a value and a type, like a cast, but, instead of
+   casting the value to the given type, a new value (of the given
+   type) is extracted from the contents of the old value, starting
+   from the least significant byte.
+
+   It is invalid for the given type to be larger than the type of the
+   given value.  */
+OP (UNOP_EXTRACT)
+
 /* UNOP_CAST is followed by a type pointer in the next exp_element.
    With another UNOP_CAST at the end, this makes three exp_elements.
    It casts the value of the following subexpression.  */
diff --git a/gdb/testsuite/gdb.base/stap-probe.c b/gdb/testsuite/gdb.base/stap-probe.c
index 2d13d02c43a..d1482c35caf 100644
--- a/gdb/testsuite/gdb.base/stap-probe.c
+++ b/gdb/testsuite/gdb.base/stap-probe.c
@@ -29,6 +29,8 @@ __extension__ unsigned short test_m4_semaphore __attribute__ ((unused)) __attrib
 __extension__ unsigned short test_pstr_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
 
 __extension__ unsigned short test_ps_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
+
+__extension__ unsigned short test_xmmreg_semaphore __attribute__ ((unused)) __attribute__ ((section (".probes")));
 #else
 
 int relocation_marker __attribute__ ((unused));
@@ -90,6 +92,24 @@ pstr (int val)
   return val == 0 ? a : b;
 }
 
+#ifdef __SSE2__
+static const char * __attribute__((noinline))
+use_xmm_reg (int val)
+{
+  volatile register int val_in_reg asm ("xmm0") = val;
+
+  STAP_PROBE1 (test, xmmreg, val_in_reg);
+
+  return val == 0 ? "xxx" : "yyy";
+}
+#else
+static const char * __attribute__((noinline)) ATTRIBUTE_NOCLONE
+use_xmm_reg (int val)
+{
+  /* Nothing.  */
+}
+#endif /* __SSE2__ */
+
 static void
 m4 (const struct funcs *fs, int v)
 {
@@ -111,5 +131,7 @@ main()
   m4 (&fs, 0);
   m4 (&fs, 1);
 
+  use_xmm_reg (0x1234);
+
   return 0; /* last break here */
 }
diff --git a/gdb/testsuite/gdb.base/stap-probe.exp b/gdb/testsuite/gdb.base/stap-probe.exp
index e86c7897b93..b0b690a4dcb 100644
--- a/gdb/testsuite/gdb.base/stap-probe.exp
+++ b/gdb/testsuite/gdb.base/stap-probe.exp
@@ -15,6 +15,98 @@
 
 standard_testfile
 
+# Count the number of probes of TYPE (either 'stap' or 'dtrace'),
+# from provider matching PROVIDER, with a name matching NAME, and from
+# an objec file matching OBJECT.
+#
+# The OBJECT is optional, in which case all objects will be matched.
+#
+# If any error condition is detected, then perror is called, and -1
+# returned.
+#
+# Otherwise, returns an integer, 0 or greater.
+proc gdb_count_probes { type provider name { object "" }} {
+    set cmd "info probes ${type} ${provider} ${name}"
+    if { $object != "" } {
+	set cmd "$cmd ${object}"
+    }
+
+    set probe_count 0
+    set no_probes_line false
+    gdb_test_multiple $cmd "" {
+	-re "^$cmd\r\n" {
+	    exp_continue
+	}
+	-re "^Type\\s+Provider\\s+Name\\s+Where\\s+Semaphore\\s+Object\\s*\r\n" {
+	    exp_continue
+	}
+	-re "^\\s*\r\n" {
+	    exp_continue
+	}
+	-re "^stap\[^\r\n\]+\r\n" {
+	    incr probe_count
+	    exp_continue
+	}
+	-re "^dtrace\[^\r\n\]+\r\n" {
+	    incr probe_count
+	    exp_continue
+	}
+	-re "^No probes matched\\.\r\n" {
+	    set no_probes_line true
+	    exp_continue
+	}
+	-re "^$::gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+
+    if { [expr $no_probes_line && $probe_count > 0] \
+	     || [expr !$no_probes_line && $probe_count == 0] } {
+	perror "Mismatch between no probes found line, and probes count"
+	return -1
+    }
+
+    return $probe_count
+}
+
+proc check_for_usable_xmm0_probe { binfile } {
+    set readelf_program [gdb_find_readelf]
+    set binfile [standard_output_file $binfile]
+    set command "exec $readelf_program -n $binfile"
+    verbose -log "command is $command"
+    set result [catch $command output]
+    verbose -log "result is $result"
+    verbose -log "output is $output"
+
+    # We don't actually check RESULT.  Sometimes readelf gives
+    # warnings about gaps in some of the notes data.  This is
+    # unrelated to the staps probes, but still causes readelf to exit
+    # with non-zero status.
+    #
+    # Instead, just check the output.  If readelf failed to run then
+    # the output will be empty, and the following regexps will fail to
+    # match.
+
+    # First, look for the xmmreg probe, and if we find it, grab the
+    # argument string.
+    if ![regexp {\n\s+Provider: test\n\s+Name: xmmreg\n[^\n]+\n\s+Arguments: ([^\n]+)\n} $output ignore arguments] {
+	verbose -log "APB: Couldn't find probe at all"
+	return false
+    }
+
+    verbose -log "APB: Matched on '$ignore'"
+    verbose -log "APB: arguments: '$arguments'"
+
+    # Check the the argument string mentions xmm0.
+    if ![regexp {@%?xmm0} $arguments] {
+	verbose -log "APB: Prove doesn't use xmm0 register"
+	return false
+    }
+
+    # Success!  We have a probe that uses xmm0 for an argument.
+    return true
+}
+
 # Run the tests.  We run the tests two different ways: once with a
 # plain probe, and once with a probe that has an associated semaphore.
 # This returns -1 on failure to compile or start, 0 otherwise.
@@ -119,6 +211,24 @@ proc stap_test {exec_name {args ""}} {
     " = $hex .This is another test message.*" \
     "print \$_probe_arg1 for probe ps"
 
+    # Check the probe is using the xmm0 register.
+    if [check_for_usable_xmm0_probe $exec_name] {
+
+	delete_breakpoints
+	if {[runto "-pstap test:xmmreg"]} {
+	    pass "run to -pstap test:xmmreg"
+	} else {
+	    fail "run to -pstap test:xmmreg"
+	}
+
+	gdb_test "print \$_probe_argc" " = 1" \
+	    "print \$_probe_argc for probe xmmreg"
+	gdb_test "print/x \$_probe_arg0" " = 0x1234" \
+	    "check \$_probe_arg0 for probe xmmreg"
+    } else {
+	unsupported "print probe argument from \$xmm0 register"
+    }
+
     return 0
 }
 
@@ -185,6 +295,21 @@ proc stap_test_no_debuginfo {exec_name {args ""}} {
     " = $hex .This is another test message.*" \
     "print \$_probe_arg1 for probe ps"
 
+    # Reinit GDB, set a breakpoint on probe ps.
+    if { [gdb_count_probes stap test xmmreg] > 0 } {
+	delete_breakpoints
+	if {[runto "-pstap test:xmmreg"]} {
+	    pass "run to -pstap test:xmmreg"
+	} else {
+	    fail "run to -pstap test:xmmreg"
+	}
+
+	gdb_test "print \$_probe_argc" " = 1" \
+	    "print \$_probe_argc for probe xmmreg"
+	gdb_test "print/x \$_probe_arg0" " = 0x1234" \
+	    "check \$_probe_arg0 for probe xmmreg"
+    }
+
     return 0
 }
 
-- 
2.25.4

Re: [PATCHv2] gdb/x86: handle stap probe arguments in xmm registers

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> Notice that for both of these probes, the first argument is a uint64_t
Andrew> stored in the xmm1 register.

I wonder how much the ideas in here overlap with Zoran's still-pending
patches to rework the DWARF expression evaluator.  IIUC his work
involved some of this same sort of extraction.  However, since yours is
via the stap probe stuff, there probably isn't much opportunity for code
reuse.

Andrew> architectures this method would fist possibly adjust the register

fist->first

Andrew> Finally, as this patch adds a new operation type, then I need to
Andrew> consider how to generate an agent expression for the new operation
Andrew> type.

Andrew> I have kicked the can down the road a bit on this.

This seems completely fine to me.

Andrew> This is because GDB doesn't currently support placing non-scalar types
Andrew> on the agent expression evaluation stack.  Solving this is clearly
Andrew> related to the original problem, but feels a bit like a second
Andrew> problem.  I'd like to get feedback on whether my approach to solving
Andrew> the original problem is acceptable or not before I start looking at
Andrew> how to handle xmm registers within agent expressions.

Note that there are many things that can't be represented in agent
expressions.  I recall filing a bug report about this -- there are some
DWARF expressions that can't be translated, and IIRC, floating point
isn't handled at all.  So, I wouldn't worry too much about this.  My
sense is that tracepoints aren't used a whole lot.

Anyway your patch looked reasonable to me.

Tom

Re: [PATCHv2] gdb/x86: handle stap probe arguments in xmm registers

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:

Andrew> +/* UNOP_EXTRACT takes a value and a type, like a cast, but, instead of
Andrew> +   casting the value to the given type, a new value (of the given
Andrew> +   type) is extracted from the contents of the old value, starting
Andrew> +   from the least significant byte.
Andrew> +
Andrew> +   It is invalid for the given type to be larger than the type of the
Andrew> +   given value.  */
Andrew> +OP (UNOP_EXTRACT)

One thing I meant to mention is that I hope that, someday, we can start
removing the OP_ constants.  In most cases they aren't really needed --
there's only a handful of checks for these, I think.  Maybe we can have
some kind of OP_NOT_REALLY_THAT_SPECIAL constant that's used instead.

In the meantime this seems fine and normal though.

Tom

Re: [PATCHv2] gdb/x86: handle stap probe arguments in xmm registers

[Pedro Alves]

On 2022-03-16 17:23, Tom Tromey wrote:
> Andrew> This is because GDB doesn't currently support placing non-scalar types
> Andrew> on the agent expression evaluation stack.  Solving this is clearly
> Andrew> related to the original problem, but feels a bit like a second
> Andrew> problem.  I'd like to get feedback on whether my approach to solving
> Andrew> the original problem is acceptable or not before I start looking at
> Andrew> how to handle xmm registers within agent expressions.
> 
> Note that there are many things that can't be represented in agent
> expressions.  I recall filing a bug report about this -- there are some
> DWARF expressions that can't be translated, and IIRC, floating point
> isn't handled at all.  So, I wouldn't worry too much about this.  My
> sense is that tracepoints aren't used a whole lot.

Yeah.  They were definitely being used in production a few years back when we were
revamping them, adding support to gdbserver, etc.  But it seems like users stopped using
them since.  It's possible that the fact that more and more DWARF expressions
can't be converted carries some weight.  Or users simply started using other
tools for the job.

In any case, if I were to tackle the whole problem set today, instead of extending agent 
expressions bytecode to be able to do more of what DWARF can do, I'd be looking at expressing
what should be collected via DWARF expressions/locations.  I mean, send DWARF expressions
to the target side to be evaluated.

Re: [PATCHv2] gdb/x86: handle stap probe arguments in xmm registers

[Andrew Burgess]

Pedro, Tom,

Thanks for your feedback on this patch.  I've gone ahead and pushed
this.  For now I don't plan to priorities working on agent expression
support for this change, but I'm happy to do so if this becomes
important.

Thanks,
Andrew



Pedro Alves <pedro@palves.net> writes:

> On 2022-03-16 17:23, Tom Tromey wrote:
>> Andrew> This is because GDB doesn't currently support placing non-scalar types
>> Andrew> on the agent expression evaluation stack.  Solving this is clearly
>> Andrew> related to the original problem, but feels a bit like a second
>> Andrew> problem.  I'd like to get feedback on whether my approach to solving
>> Andrew> the original problem is acceptable or not before I start looking at
>> Andrew> how to handle xmm registers within agent expressions.
>> 
>> Note that there are many things that can't be represented in agent
>> expressions.  I recall filing a bug report about this -- there are some
>> DWARF expressions that can't be translated, and IIRC, floating point
>> isn't handled at all.  So, I wouldn't worry too much about this.  My
>> sense is that tracepoints aren't used a whole lot.
>
> Yeah.  They were definitely being used in production a few years back when we were
> revamping them, adding support to gdbserver, etc.  But it seems like users stopped using
> them since.  It's possible that the fact that more and more DWARF expressions
> can't be converted carries some weight.  Or users simply started using other
> tools for the job.
>
> In any case, if I were to tackle the whole problem set today, instead of extending agent 
> expressions bytecode to be able to do more of what DWARF can do, I'd be looking at expressing
> what should be collected via DWARF expressions/locations.  I mean, send DWARF expressions
> to the target side to be evaluated.