Re: [PATCH 2/2] C++-ify print_variable_and_value_data

[Andrew Burgess <aburgess@redhat.com>]

Hi Tom,

This mostly looks fine, there's just one thing that I think I wanted to
discus...

Tom Tromey <tromey@adacore.com> writes:

> While working on a different patch, I decided to C++-ify
> print_variable_and_value_data somewhat.  This patch gives it a
> constructor and changes it to use bool.
>
> Also, I noticed that this code does not really need to use a frame_id.
> Using a frame_info_ptr is just as good due to the "reinflation"
> behavior of that class.

This is a super slim edge case, but I think it might be worth mentioning
at this point in the commit message.  I'm kind of aware of this right
now because I'm looking at some frame related problems, so I'm thinking
a log about frame_info_ptr, and reinflation...

> @@ -2276,32 +2291,25 @@ print_variable_and_value_data::operator() (const char *print_name,
>    if (language_def (sym->language ())->symbol_printing_suppressed (sym))
>      return;
>  
> -  frame = frame_find_by_id (frame_id);
> -  if (frame == NULL)
> -    {
> -      warning (_("Unable to restore previously selected frame."));
> -      return;
> -    }
> -

This code handles the case where we cannot re-find the frame for
FRAME_ID.  But if we look in frame_info_ptr::reinflate, there's no such
handling.  We just: gdb_assert (m_ptr != nullptr), in the above code it
would be like replacing the `if` block with gdb_assert (frame != NULL);

I think this is a bug in the reinflate logic which has existed since it
was added.

What would actually need to happen to trigger this bug?  In this case I
think something like a Python pretty printer would need to change the
inferior state in such a way that the backtrace changed, and then flush
the frame cache.

I don't think this is something a well written pretty printed should be
doing.  But it is possible, and GDB shouldn't be asserting if it did.

But also, I don't think this issue is limited to your code, so I don't
think this is something you need to fix.

But it might be worth mentioning in the commit message, that with this
change, this isn't a pure refactor, there is a small edge case which
changes behaviour.

Otherwise, this looks great.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew