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

[Andrew Burgess <aburgess@redhat.com>]

"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