From 6f0dabd46d0917ee1b8bdfcba7023f503525118d Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 10 Mar 2022 11:57:18 -0500 Subject: [PATCH] gdb/x86: handle stap probe arguments in xmm registers 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 266a4f7699f..1211930a377 100644 --- a/gdb/eval.c +++ b/gdb/eval.c @@ -2460,6 +2460,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; +} + } diff --git a/gdb/expop.h b/gdb/expop.h index d903ab0bb7e..a346570cb4c 100644 --- a/gdb/expop.h +++ b/gdb/expop.h @@ -1947,6 +1947,37 @@ protected: 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 +{ +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 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 (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 (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.30.2