Re: [PATCHv6] gdb: Change how frames are selected for 'frame' and 'info frame'.

[Pedro Alves <palves at redhat dot com>]

On 09/19/2018 12:01 AM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2018-09-13 19:02:31 +0100]:
> 
>> Hi,
>>
>> On 08/28/2018 07:03 PM, Andrew Burgess wrote:
>>> I believe that this variant addresses all existing review comments.
>>> Philippe provided additional feedback in this message:
>>>
>>>    https://sourceware.org/ml/gdb-patches/2018-08/msg00349.html
>>>
>>> but I believe this was more about things we need to do to unify the
>>> docs/code/comments on one of either 'number' or 'level', rather than
>>> things that need to be fixed before this patch could be merged.
>>>
>>> This version of the patch is based off v5(B) and uses the keyword
>>> 'level' instead of 'number'.
>>>
>>> The only changes over the previous version are an additional change in
>>> the docs as previously discussed with Eli, and I've rebased the patch
>>> onto current HEAD (and retested).
>>>
>>> OK to commit?
>>
>> First off, I'm very much for this patch, having argued for something
>> like it before <https://sourceware.org/ml/gdb/2014-11/msg00028.html>.
>>
>> As we discussed at the Cauldron, my main concern about this
>> patch/direction is the "view" variant.  It's a bit odd in that
>> it doesn't "select" a frame from the current stack, like the
>> other variants, which makes it a little odd, but I understand where
>> it's coming from.  There's more to it however.  Let me go over
>> my main concern.
>>
>> Specifically, if you do "frame view 0xADDR", and then do "up/down", then
>> GDB moves to the next/previous frame relative to the 0xADDR frame, as I
>> would expect.  That's fine.
>>
>> However, if you do "backtrace", we still show a backtrace starting at the
>> thread's real current frame instead of at the selected "view" frame.
>> This is IMO inconsistent, and surprising.  Though, it's also how current gdb
>> behaves with "frame 0xADDR", so it's not really a behavior change.
>>
>> Also, related, after "frame view 0xADDR", if you do anything that might
>> flush the frame cache, then you lose the "view"'d frame.  E.g., that
>> can be with "info threads" (if you have multiple threads"), or if
>> in non-stop mode while threads are running, some thread hits some
>> temporary event (in which case I think GDB just loses the viewed
>> frame without the user realizing).  Again, these aren't really
>> problems the patch is adding, since you get the same problems
>> with the current "frame 0xADDR":
>>
>>  (gdb) frame 0x0011
>>  #0  0x0000000000000000 in ?? ()
>>  (gdb) frame
>>  #0  0x0000000000000000 in ?? ()
>>  (gdb) info threads 
>>    Id   Target Id                                     Frame 
>>  * 1    Thread 0x7ffff7fb6740 (LWP 32581) "threads"   0x0000000000000000 in ?? ()
>>    2    Thread 0x7ffff7803700 (LWP 32585) "threads"   0x00007ffff78c7460 in __GI___nanosleep  
>>    3    Thread 0x7ffff7002700 (LWP 32586) "threads"   0x00007ffff78c7460 in __GI___nanosleep  
>>  (gdb) info threads 
>>    Id   Target Id                                     Frame 
>>  * 1    Thread 0x7ffff7fb6740 (LWP 32581) "threads"   0x00007ffff7bc28ad in __pthread_join
>>    2    Thread 0x7ffff7803700 (LWP 32585) "threads"   0x00007ffff78c7460 in __GI___nanosleep
>>    3    Thread 0x7ffff7002700 (LWP 32586) "threads"   0x00007ffff78c7460 in __GI___nanosleep
>>  (gdb) frame
>>  #0  0x00007ffff7bc28ad in __pthread_join () at pthread_join.c:90
>>  90          lll_wait_tid (pd->tid);
>>  (gdb) 
>>
>> Notice also how thread 1's "Frame" column showed the "viewed" frame
>> the first time, which is incorrect, IMO.  The second time we see
>> the thread's current frame, because the first "info threads" command
>> lost the "viewed" frame, as can be also seen in the last "frame"
>> command.
>>
>> Again, this is not a new problem (and above I'm not using
>> your patch to show it).  But, it does highlight problems with
>> the "frame view" direction that (I think I mentioned before)
>> should ideally be addressed somehow.  Particularly if we're now
>> proposing adding some new UI to expose the functionality, it
>> makes me wonder whether it's a good idea to expose functionality
>> in the UI that we may end up removing soon enough.  I.e., my
>> concern is with whether addressing this would result in
>> getting rid of "frame view".
>>
>> E.g., the idea of using "frame level 0" to get back to the
>> thread's real current frame (as mentioned in the documentation
>> bits added by this patch) conflicts with the fact that
>> "up/down" after "frame view" moves the selected stack
>> frame within the viewed stack, which means those frames
>> also have a relative frame level within that viewed
>> stack.  One could very reasonably expect that
>> "frame view 0xADDR; up; frame level 0" would get you back
>> to the frame viewed at 0xADDR.
>>
>> At the Cauldron, you had a nice idea of adding some separate
>> command to create alternate stacks, like "(gdb) create-stack 0xADDR",
>> coupled with some way to list the created stacks, like "info stack",
>> and a way to switch between the created stacks and the thread's real
>> current stack.  I think something like that would be really nice.
>>
>> I'm not going to insist on going that direction before
>> accepting this patch, though I must admit that I'd
>> be delighted to seeing it explored.  I'll reserve the
>> right to not fell guilty about (someone) ripping out "view"
>> later on if this goes in as is.
> 
> I agree with everything you've written above.  I really hope that
> someone does rip out 'frame view' and I hope it's me.  But if someone
> beats me to it, then great.
> 

Great.  Glad we're in sync.

> All the rest of the comments are formatting / spelling type stuff.  I
> believe that these are all addressed in the latest version of the
> patch.
> 

This is OK with the nits below addressed.


>  enum what_to_list { locals, arguments, all };
>  
> @@ -678,13 +679,123 @@ list_args_or_locals (enum what_to_list what, enum print_values values,
>      }
>  }
>  
> +/* Read a frame specification in whatever the appropriate format is from
> +   FRAME_EXP.  Call error() if the specification is in any way invalid (so
> +   this function never returns NULL).  */
> +

Could you expand the comment a bit to mention that that format
is either a level, or an address, the latter undocumented but kept for
for backward compatibility?

> +static struct frame_info *
> +parse_frame_specification (const char *frame_exp)
> +{

> +
> +# This tests GDBs frame selection as used by the 'frame',
> +# 'select-frame', and 'info frame' commands.

typo: GDB's

> +
> +# Perform "info frame" to extract the frames address.

typo: "frame's".

> +proc get_frame_address { {testname ""} } {
> +    global hex gdb_prompt
> +
> +    set frame_address "unknown"
> +    set testname "get_frame_address: ${testname}"
> +    gdb_test_multiple "info frame" $testname {
> +	-re ", frame at ($hex):\r\n.*\r\n$gdb_prompt $" {
> +	    set frame_address $expect_out(1,string)
> +	    pass $testname
> +	}
> +    }
> +
> +    return $frame_address
> +}
> +
> +# Passed a list of addresses.  Return a new address that is not in the
> +# list.
> +proc get_new_address { {addresses {}} } {
> +    return [format "%#x" [expr [lindex $addresses [llength addresses]] + 0x10 ]]
> +}

I still had trouble understanding the comment.  :-(

This seem to return the last address in the list, plus 0x10.  That
assumes that that address is not in the list yet, which is
unlike what I'd assume the implementation would do, given
the comment alone.  E.g., I'd assume a function with that comment
would iterate over all elements, get the max element, and then
return that + 0x10.  As is, the implementation is assuming
the stack grows in the same direction on all platforms (hp-pa
grows up, for example).

> +
> +
> +# 	-re "Stack level ${level}, frame at ($address):\r\n .* = $hex in ${function} \(\[^\r\n\]*\); saved .* = $hex\r\n.*\r\n$gdb_prompt $" {

Leftover ?

Thanks,
Pedro Alves