Re: [PATCH v4 2/4] gdb/cli: add '.' as an argument for 'list' command

[Pedro Alves <pedro@palves.net>]

On 2023-07-17 09:21, Bruno Larsen wrote:
> On 14/07/2023 19:50, Pedro Alves wrote:
>> Hi,
>>
>> Sorry for coming late to the party here, but while trying to catch up on the
>> list I spotted a few things that I think should be fixed.  See below.
> I forgot to send the email, but I've already pushed this patch... 

No worries, I had seen that.  That's what I meant by being late to the party.  :-)

> I'll fix up the things you pointed in a moment and send a new patch, I just have a few questions.

Thanks.

>>
>> BTW, I think the feature is cool!
>>
>> On 2023-07-13 11:24, Bruno Larsen via Gdb-patches wrote:

>>> diff --git a/gdb/NEWS b/gdb/NEWS
>>> index b924834d3d7..eef440a5242 100644
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -84,6 +84,10 @@
>>>     is 64k.  To print longer strings you should increase
>>>     'max-value-size'.
>>>   +* The 'list' command now accepts '.' as an argument, which tells GDB to
>>> +  print the location where the inferior is stopped.  If the inferior hasn't
>>> +  started yet, the command will print around the main function.
>> It would be more accurate to say that it's where the current thread is stopped.
>>
>> Say you run to breakpoint hit in thread 1, and then switch to thread 2, and then do
>> "list .".  That will show you the current frame of thread 2, while "where the
>> inferior is stopped" could very well be interpreted as "thread 1".
> 
> The wording does need changing, I agree. I think using the wording that already exists in the documentation is the best way to go: "prints around the last solitary line printed as part of displaying a stack frame". The wording I originally used (and the improvement you suggested) could make it sound like I would always print the lowermost frame, when that is not the point I wanted. What I wanted was to re-do what the first usage of "list" does without needing to know the lines.

Makes sense.  So this is really just a shortcut for:

  (gdb) frame
  (gdb) list

>>> +      else if (arg[0] == '.')
>>> +    {
>>> +      try
>>> +        {
>>> +          /* Find the current line by getting the PC of the currently
>>> +         selected frame, and finding the line associated to it.  */
>>> +          frame_info_ptr frame = get_selected_frame (nullptr);
>> So this is actually using the selected frame, not the current frame.  So even
>> the "where the thread is stopped" description would be incorrect.  If you really
>> wanted to use frame #0 (where the thread stopped), then this should use get_current_frame.
> Yeah... that's the confusion I mentioned earlier... I want the selected frame, since its what "list" does when you first call it after entering a frame.
>>
>>
>>> +          CORE_ADDR curr_pc = get_frame_pc (frame);
>>> +          cursal = find_pc_line (curr_pc, 0);
>>> +        }
>>> +      catch (const gdb_exception &e)
>>> +        {
>>> +          /* If there was an exception above, it means the inferior
>>> +             is not running, so reset the current source location to
>>> +             the default.  */
>> Aww.  Please don't use a try/catch for this...  You can just check target_has_execution.
> Oh, I thought target_has_execution would query if the target can execute the inferior, rather than looking if the inferior is being executed. Will change

Actually, target_has_stack would be even more appropriate.