[RFA,1/5] Remove some ui_out-related cleanups from Python

  This patch introduces a bit of infrastructure -- namely, a minimal
std::optional analogue called gdb::optional, and an RAII template
class that works like make_cleanup_ui_out_tuple_begin_end or
make_cleanup_ui_out_list_begin_end -- and then uses these in the
Python code.  This removes a number of cleanups and generally
simplifies this code.

std::optional is only available in C++17.  Normally I would have had
this code check __cplusplus, but my gcc apparently isn't new enough to
find <optional>, even with -std=c++1z; so, because I could not test
it, the patch does not do this.

2017-01-15  Tom Tromey  <tom@tromey.com>

	* ui-out.h (ui_out_emit_type): New class.
	(ui_out_emit_tuple, ui_out_emit_list): New typedefs.
	* python/py-framefilter.c (py_print_single_arg): Use gdb::optional
	and ui_out_emit_tuple.
	(enumerate_locals): Likewise.
	(py_mi_print_variables, py_print_locals, py_print_args): Use
	ui_out_emit_list.
	(py_print_frame): Use gdb::optional, ui_out_emit_tuple,
	ui_out_emit_list.
	* common/gdb_option.h: New file.
---
 gdb/ChangeLog               |  13 +++
 gdb/common/gdb_option.h     |  87 ++++++++++++++++++
 gdb/python/py-framefilter.c | 213 ++++++++++++--------------------------------
 gdb/ui-out.h                |  33 +++++++
 4 files changed, 189 insertions(+), 157 deletions(-)
 create mode 100644 gdb/common/gdb_option.h
  

On 2017-01-15 08:42, Tom Tromey wrote:
> This patch introduces a bit of infrastructure -- namely, a minimal
> std::optional analogue called gdb::optional, and an RAII template
> class that works like make_cleanup_ui_out_tuple_begin_end or
> make_cleanup_ui_out_list_begin_end -- and then uses these in the
> Python code.  This removes a number of cleanups and generally
> simplifies this code.
> 
> std::optional is only available in C++17.  Normally I would have had
> this code check __cplusplus, but my gcc apparently isn't new enough to
> find <optional>, even with -std=c++1z; so, because I could not test
> it, the patch does not do this.
> 
> 2017-01-15  Tom Tromey  <tom@tromey.com>
> 
> 	* ui-out.h (ui_out_emit_type): New class.
> 	(ui_out_emit_tuple, ui_out_emit_list): New typedefs.
> 	* python/py-framefilter.c (py_print_single_arg): Use gdb::optional
> 	and ui_out_emit_tuple.
> 	(enumerate_locals): Likewise.
> 	(py_mi_print_variables, py_print_locals, py_print_args): Use
> 	ui_out_emit_list.
> 	(py_print_frame): Use gdb::optional, ui_out_emit_tuple,
> 	ui_out_emit_list.
> 	* common/gdb_option.h: New file.
> ---
>  gdb/ChangeLog               |  13 +++
>  gdb/common/gdb_option.h     |  87 ++++++++++++++++++

Any reason you did not name this gdb_optional.h?

Otherwise, it looks really nice.
  

> +++ b/gdb/common/gdb_option.h

might be nice to put it in include/ but fine to do that later when
something else actually wants it.

> +/* This class attempts to be a compatible subset of std::optional,
> +   which is slated to be available in C++17.  This class optionally
> +   holds an object of some type -- by default it is constructed not
> +   holding an object, but later the object can be "emplaced".  This is
> +   similar to using std::unique_ptr, but stack allocation is
> +   guaranteed.  */

 wording nit, but stack isn't quiet what you want there, I can imagine
 putting an optional<T> in some object that lives on the heap.

> +template<typename T>
> +class optional
> +{
> +public:
> +
> +  optional ()
> +    : m_instantiated (false)
> +  {
> +  }
> +
> +  ~optional ()
> +  {
> +    if (m_instantiated)
> +      destroy ();
> +  }
> +
> +  /* These aren't deleted in std::optional, but it was simpler to
> +     delete them here, because currently the users of this class don't
> +     need them, and making them depend on the definition of T is
> +     somewhat complicated.  */

I think you can make <type_traits> do most of it, but fair enough.

> +  /* True if the object was ever emplaced.  */
> +  bool m_instantiated;
> +
> +  /* The object.  */
> +  union
> +  {
> +    struct { } m_dummy;
> +    T m_item;
> +  };

It doesn't matter yet, but space wise it would be better to put the bool
last right? For example if sizeof(T) is 6, or if the optional is in some
larger structure with a bool next.

Trev
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Any reason you did not name this gdb_optional.h?

No, no good reason.  I'll rename it.

Tom
  

On 01/16/2017 11:30 AM, Trevor Saunders wrote:
>> +++ b/gdb/common/gdb_option.h
> 
> might be nice to put it in include/ but fine to do that later when
> something else actually wants it.

I've been thinking about putting all these utilities and
later-standards replacements we're coming up with in its own
directory/namespace.  I had thought of "gtl", for "gdb template
library", or "gnu template library" if other projects want to
reuse it.  (I think "gnu" is kind of taken by gnulib / libgnu.a)
One difficulty with putting such a thing at the top level is that
gdb relies on gnulib headers (that exist under gdb/) while
other toolchain components don't, yet, at least.

>> > +  /* These aren't deleted in std::optional, but it was simpler to
>> > +     delete them here, because currently the users of this class don't
>> > +     need them, and making them depend on the definition of T is
>> > +     somewhat complicated.  */
> I think you can make <type_traits> do most of it, but fair enough.
> 

Back around <https://sourceware.org/ml/gdb-patches/2016-11/msg00368.html>
I backported C++17's optional to C++11.  I still have that branch
somewhere locally...  /me finds it and pushes to github.  Here:

 https://github.com/palves/gdb/commits/palves/gdb-import-gcc-optional

This <https://github.com/palves/gdb/commit/880ab485c5873eb0a8ab1dca75e624ec2a064fe8>
contains the local changes I had made to to port it to C++11.  I was surprised
that other than portable type traits stuff that's missing in C++11, the
only thing that C++11 loses is constexpr-ness in some cases.  Now,
the result is of course much more code than what Tromey is proposing.
It probably does make sense to start with something simpler and
upgrade when/if we find a need.  OTOH, looks like the current patch
doesn't have accessors for the wrapped value, so it seems like we'll
at least need to be extend it in that direction in no time.

>> +  /* True if the object was ever emplaced.  */
>> +  bool m_instantiated;
>> +
>> +  /* The object.  */
>> +  union
>> +  {
>> +    struct { } m_dummy;
>> +    T m_item;
>> +  };
> 
> It doesn't matter yet, but space wise it would be better to put the bool
> last right? For example if sizeof(T) is 6, or if the optional is in some
> larger structure with a bool next.

Agreed.  Seems like that's what gcc's optional does.

I also agree with Simon's suggestion for renaming the file.

Patch LGTM with Trevor's and Simon's nits  addressed.

I wondered about making m_instantiated a char, so that optional<T> would
pack even better when sizeof or alignof T is small, and thus ends up being
no cost in those cases space-wise.  Though maybe that ends up being
worse / not so efficient generated code -wise, or GCC would do it too?
In any case, since GCC doesn't do that, if/when we ever move to C++17,
we'd lose that again anyway.

Thanks,
Pedro Alves
  

On 02/08/2017 05:28 PM, Pedro Alves wrote:

> I wondered about making m_instantiated a char, so that optional<T> would
> pack even better when sizeof or alignof T is small, and thus ends up being
> no cost in those cases space-wise.  Though maybe that ends up being
> worse / not so efficient generated code -wise, or GCC would do it too?
> In any case, since GCC doesn't do that, if/when we ever move to C++17,
> we'd lose that again anyway.

Eh, now that I check I see that sizeof bool == 1, and looking at GCC's sources,
I see it's like that for almost all ports.  Somehow I misremembered
this and thought it was sizeof(int).

Thanks,
Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> One difficulty with putting such a thing at the top level is that
Pedro> gdb relies on gnulib headers (that exist under gdb/) while
Pedro> other toolchain components don't, yet, at least.

gnulib should just be at top-level, but I realize that's a pain.

Pedro> It probably does make sense to start with something simpler and
Pedro> upgrade when/if we find a need.  OTOH, looks like the current patch
Pedro> doesn't have accessors for the wrapped value, so it seems like we'll
Pedro> at least need to be extend it in that direction in no time.

I had forgotten about this code.  I don't mind using it instead.

Pedro> Patch LGTM with Trevor's and Simon's nits  addressed.

I have that done locally, but I've just been waiting until your series
lands, so I can rebase.  And then maybe the option stuff wouldn't be
needed anyway?  I don't remember the details from your branch, I was
just waiting for the big rebase to find out.

Tom
  

On 02/08/2017 11:04 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> One difficulty with putting such a thing at the top level is that
> Pedro> gdb relies on gnulib headers (that exist under gdb/) while
> Pedro> other toolchain components don't, yet, at least.
> 
> gnulib should just be at top-level, but I realize that's a pain.
> 
> Pedro> It probably does make sense to start with something simpler and
> Pedro> upgrade when/if we find a need.  OTOH, looks like the current patch
> Pedro> doesn't have accessors for the wrapped value, so it seems like we'll
> Pedro> at least need to be extend it in that direction in no time.
> 
> I had forgotten about this code.  I don't mind using it instead.

Let's start with your version, and leave the door open.  The version I have
would need a bit of cleaning, re-checking if GCC's implementation changed
meanwhile (ISTR there have been recent patches to address some DRs), check
that it compiles against clang and older gcc's (4.8, etc.).  I already have too
many balls in the air ATM.

> Pedro> Patch LGTM with Trevor's and Simon's nits  addressed.
> 
> I have that done locally, but I've just been waiting until your series
> lands, so I can rebase.  And then maybe the option stuff wouldn't be
> needed anyway?  I don't remember the details from your branch, I was
> just waiting for the big rebase to find out.

My series is all in master.  I think we'll still need your patch, since I only
touched ui_files.  I haven't done anything with ui_out list/tuple building.

Thanks,
Pedro Alves
  

On Wed, Feb 8, 2017 at 3:51 PM, Pedro Alves <palves@redhat.com> wrote:

>> Pedro> Patch LGTM with Trevor's and Simon's nits  addressed.
>>
>> I have that done locally, but I've just been waiting until your series
>> lands, so I can rebase.  And then maybe the option stuff wouldn't be
>> needed anyway?  I don't remember the details from your branch, I was
>> just waiting for the big rebase to find out.
>
> My series is all in master.  I think we'll still need your patch, since I only
> touched ui_files.  I haven't done anything with ui_out list/tuple building.

I haven't been following much lately due to programming mostly in
languages unsupported by gdb apologies if this has been hashed out
before.

Wondering if this would be a good opportunity to try and transition
ui_out_list construction into a more type safe manner, e.g.

result ==> variable "=" value
list ==> "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"

currently when building a list, it didn't specify whether you were
declaring a list of results or a list of values, unless/until the
first value or result was added, and IIRC this | property wasn't
really explicitly enforced.

it'd be nice to migrate this to something like:
result_list, value_list, deprecated_unspecified_list, It would be nice
to know if/when you guys think it would be a convenient time to
introduce such a change so I could get back up to speed...
  

Hi Matt,

On 02/09/2017 04:34 AM, Matt Rice wrote:

> Wondering if this would be a good opportunity to try and transition
> ui_out_list construction into a more type safe manner, e.g.
> result ==> variable "=" value
> list ==> "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"
> 
> currently when building a list, it didn't specify whether you were
> declaring a list of results or a list of values, unless/until the
> first value or result was added, and IIRC this | property wasn't
> really explicitly enforced.

More type-safety sounds good to me.  :-)

Can you give an example of a command that outputs a result list,
and an example of a command that outputs a value list?

> it'd be nice to migrate this to something like:
> result_list, value_list, deprecated_unspecified_list, It would be nice
> to know if/when you guys think it would be a convenient time to
> introduce such a change so I could get back up to speed...

Simon has already C++-fyed ui_out in master, so this seems like
a good time to me.

Thanks,
Pedro Alves
  

On 02/09/2017 12:48 PM, Pedro Alves wrote:
> Hi Matt,
> 
> On 02/09/2017 04:34 AM, Matt Rice wrote:
> 
>> Wondering if this would be a good opportunity to try and transition
>> ui_out_list construction into a more type safe manner, e.g.
>> result ==> variable "=" value
>> list ==> "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"
>>
>> currently when building a list, it didn't specify whether you were
>> declaring a list of results or a list of values, unless/until the
>> first value or result was added, and IIRC this | property wasn't
>> really explicitly enforced.
> 
> More type-safety sounds good to me.  :-)
> 
> Can you give an example of a command that outputs a result list,
> and an example of a command that outputs a value list?
> 
>> it'd be nice to migrate this to something like:
>> result_list, value_list, deprecated_unspecified_list, It would be nice
>> to know if/when you guys think it would be a convenient time to
>> introduce such a change so I could get back up to speed...
> 
> Simon has already C++-fyed ui_out in master, so this seems like
> a good time to me.

Though, to be clear, IIUC, you're talking about changing
GDB internals without affecting the resulting MI output, right?

Thanks,
Pedro Alves
  

On Thu, Feb 9, 2017 at 4:51 AM, Pedro Alves <palves@redhat.com> wrote:
> On 02/09/2017 12:48 PM, Pedro Alves wrote:
>> Hi Matt,
>>
>> On 02/09/2017 04:34 AM, Matt Rice wrote:
>>
>>> Wondering if this would be a good opportunity to try and transition
>>> ui_out_list construction into a more type safe manner, e.g.
>>> result ==> variable "=" value
>>> list ==> "[]" | "[" value ( "," value )* "]" | "[" result ( "," result )* "]"
>>>
>>> currently when building a list, it didn't specify whether you were
>>> declaring a list of results or a list of values, unless/until the
>>> first value or result was added, and IIRC this | property wasn't
>>> really explicitly enforced.
>>
>> More type-safety sounds good to me.  :-)
>>
>> Can you give an example of a command that outputs a result list,
>> and an example of a command that outputs a value list?
>>

There is a good example of a command which outputs both in the
documentation (in the example below "The -break-insert command")

interpreter-exec mi -break-insert main
interpreter-exec mi -break-insert -t foo
interpreter-exec mi -break-list

in the output of the -break-list command there is a list of results of the form:
body=[bkpt=... , bkpt=...]

https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Breakpoint-Commands.html#g_t_002dbreak_002dinsert

I picked that one specifically because it shows the duplicate "bkpt" keys.
inside those bkpt={... thread-groups=["i1"] ... } should be a list of values.

interpreter-exec mi -list-thread-groups is a slightly easier to read
one that returns a list of values (tuples)
^done,groups=[tuple, ...]

Additionally of note is:
https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax
* New gdb/mi commands should only output lists containing values.

So, i suppose result_list should be some variation on
compat_only_result_list or some such...

>>> it'd be nice to migrate this to something like:
>>> result_list, value_list, deprecated_unspecified_list, It would be nice
>>> to know if/when you guys think it would be a convenient time to
>>> introduce such a change so I could get back up to speed...
>>
>> Simon has already C++-fyed ui_out in master, so this seems like
>> a good time to me.
>
> Though, to be clear, IIUC, you're talking about changing
> GDB internals without affecting the resulting MI output, right?

Yes, internals change only,
  

On 2017-02-09 10:46, Matt Rice wrote:
> There is a good example of a command which outputs both in the
> documentation (in the example below "The -break-insert command")
> 
> interpreter-exec mi -break-insert main
> interpreter-exec mi -break-insert -t foo
> interpreter-exec mi -break-list
> 
> in the output of the -break-list command there is a list of results of 
> the form:
> body=[bkpt=... , bkpt=...]
> 
> https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Breakpoint-Commands.html#g_t_002dbreak_002dinsert
> 
> I picked that one specifically because it shows the duplicate "bkpt" 
> keys.
> inside those bkpt={... thread-groups=["i1"] ... } should be a list of 
> values.
> 
> interpreter-exec mi -list-thread-groups is a slightly easier to read
> one that returns a list of values (tuples)
> ^done,groups=[tuple, ...]
> 
> Additionally of note is:
> https://sourceware.org/gdb/current/onlinedocs/gdb/GDB_002fMI-Output-Syntax.html#GDB_002fMI-Output-Syntax
> * New gdb/mi commands should only output lists containing values.
> 
> So, i suppose result_list should be some variation on
> compat_only_result_list or some such...

I like the idea of enforcing proper MI output through code.  One 
situation I know GDB outputs broken MI is with multiple locations 
breakpoints.  See:

   https://sourceware.org/bugzilla/show_bug.cgi?id=14733

It'd be nice if the MI building API didn't even let us do that.

>>>> it'd be nice to migrate this to something like:
>>>> result_list, value_list, deprecated_unspecified_list, It would be 
>>>> nice
>>>> to know if/when you guys think it would be a convenient time to
>>>> introduce such a change so I could get back up to speed...

Yes that would be great.  I would say that the most convenient time is 
when you have time to contribute to the project :).

About the naming, I think that "result_list" and "value_list" is 
confusing, since a list containing results is exactly what is 
deprecated.  What about just "tuple" and "list"?  According to the 
syntax and the note you mentioned, a tuple ({...}) will only accept 
results (foo=bar) and a list ([...]) will only accept values.  And we'd 
have deprecated_result_list to build lists containing results.

Simon
  

On Wed, Feb 08, 2017 at 05:28:11PM +0000, Pedro Alves wrote:
> On 01/16/2017 11:30 AM, Trevor Saunders wrote:
> >> +++ b/gdb/common/gdb_option.h
> > 
> > might be nice to put it in include/ but fine to do that later when
> > something else actually wants it.
> 
> I've been thinking about putting all these utilities and
> later-standards replacements we're coming up with in its own
> directory/namespace.  I had thought of "gtl", for "gdb template
> library", or "gnu template library" if other projects want to
> reuse it.  (I think "gnu" is kind of taken by gnulib / libgnu.a)
> One difficulty with putting such a thing at the top level is that
> gdb relies on gnulib headers (that exist under gdb/) while
> other toolchain components don't, yet, at least.

yeah. I've been meaning to get to that too.  I think for some of these
things that may not matter, but for some I'm sure it does.

> >> > +  /* These aren't deleted in std::optional, but it was simpler to
> >> > +     delete them here, because currently the users of this class don't
> >> > +     need them, and making them depend on the definition of T is
> >> > +     somewhat complicated.  */
> > I think you can make <type_traits> do most of it, but fair enough.
> > 
> 
> Back around <https://sourceware.org/ml/gdb-patches/2016-11/msg00368.html>
> I backported C++17's optional to C++11.  I still have that branch
> somewhere locally...  /me finds it and pushes to github.  Here:
> 
>  https://github.com/palves/gdb/commits/palves/gdb-import-gcc-optional
> 
> This <https://github.com/palves/gdb/commit/880ab485c5873eb0a8ab1dca75e624ec2a064fe8>
> contains the local changes I had made to to port it to C++11.  I was surprised
> that other than portable type traits stuff that's missing in C++11, the
> only thing that C++11 loses is constexpr-ness in some cases.  Now,
> the result is of course much more code than what Tromey is proposing.
> It probably does make sense to start with something simpler and
> upgrade when/if we find a need.  OTOH, looks like the current patch
> doesn't have accessors for the wrapped value, so it seems like we'll
> at least need to be extend it in that direction in no time.

yeah, I think start small is a very reasonable approach, though I'd
agree you'll want to get at the wrapped value pretty soon.

Trev