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,
>
>>> 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