[1/6] New function null_stream

  This patch adds a new function null_stream, which returns a null
stream.  The null stream can be used in multiple places.  It is
used in gdb_insn_length, and the following patches will use it too.

gdb:

2017-01-13  Yao Qi  <yao.qi@linaro.org>

	* disasm.c (do_ui_file_delete): Delete.
	(gdb_insn_length): Move code creating stream to ...
	* utils.c (null_stream): ... here.  New function.
	* utils.h (null_stream): Declare.
---
 gdb/disasm.c | 17 +----------------
 gdb/utils.c  | 13 +++++++++++++
 gdb/utils.h  |  4 ++++
 3 files changed, 18 insertions(+), 16 deletions(-)
  

On 01/16/2017 04:02 AM, Yao Qi wrote:
> This patch adds a new function null_stream, which returns a null
> stream.  The null stream can be used in multiple places.  It is
> used in gdb_insn_length, and the following patches will use it too.
>
> gdb:
>
> 2017-01-13  Yao Qi  <yao.qi@linaro.org>
>
> 	* disasm.c (do_ui_file_delete): Delete.
> 	(gdb_insn_length): Move code creating stream to ...
> 	* utils.c (null_stream): ... here.  New function.
> 	* utils.h (null_stream): Declare.
> ---
>  gdb/disasm.c | 17 +----------------
>  gdb/utils.c  | 13 +++++++++++++
>  gdb/utils.h  |  4 ++++
>  3 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index f419501..ae3a2f1 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -838,28 +838,13 @@ gdb_print_insn (struct gdbarch *gdbarch, CORE_ADDR memaddr,
>    return length;
>  }
>
> -static void
> -do_ui_file_delete (void *arg)
> -{
> -  ui_file_delete ((struct ui_file *) arg);
> -}
> -
>  /* Return the length in bytes of the instruction at address MEMADDR in
>     debugged memory.  */
>
>  int
>  gdb_insn_length (struct gdbarch *gdbarch, CORE_ADDR addr)
>  {
> -  static struct ui_file *null_stream = NULL;
> -
> -  /* Dummy file descriptor for the disassembler.  */
> -  if (!null_stream)
> -    {
> -      null_stream = ui_file_new ();
> -      make_final_cleanup (do_ui_file_delete, null_stream);
> -    }
> -
> -  return gdb_print_insn (gdbarch, addr, null_stream, NULL);
> +  return gdb_print_insn (gdbarch, addr, null_stream (), NULL);
>  }
>
>  /* fprintf-function for gdb_buffered_insn_length.  This function is a
> diff --git a/gdb/utils.c b/gdb/utils.c
> index f142ffe..7bad193 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -199,6 +199,19 @@ make_cleanup_ui_file_delete (struct ui_file *arg)
>    return make_cleanup (do_ui_file_delete, arg);
>  }
>
> +struct ui_file *
> +null_stream (void)
> +{
> +  static struct ui_file *stream = NULL;
> +
> +  if (stream == NULL)
> +    {
> +      stream = ui_file_new ();
> +      make_final_cleanup (do_ui_file_delete, stream);
> +    }

Since we're explicitly setting stream to NULL, we will always execute 
the conditional block, so it is not needed. Unless this is supposed to 
reuse a stream, but in that case the code would be incorrect.

> +  return stream;
> +}
> +
>  /* Helper function for make_cleanup_ui_out_redirect_pop.  */
>
>  static void
> diff --git a/gdb/utils.h b/gdb/utils.h
> index c548a50..26e60af 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -189,6 +189,10 @@ extern struct ui_file *gdb_stdtarg;
>  extern struct ui_file *gdb_stdtargerr;
>  extern struct ui_file *gdb_stdtargin;
>
> +/* Return a null stream.  */
> +

Spurious newline between comment and prototype.

https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Empty_line_between_subprogram_description_and_the_subprogram_implementation

> +extern struct ui_file *null_stream (void);
> +
>  /* Set the screen dimensions to WIDTH and HEIGHT.  */
>
>  extern void set_screen_width_and_height (int width, int height);
>

Otherwise looks fine.
  

On 17-01-17 07:49:07, Luis Machado wrote:
> >
> >+struct ui_file *
> >+null_stream (void)
> >+{
> >+  static struct ui_file *stream = NULL;
> >+
> >+  if (stream == NULL)
> >+    {
> >+      stream = ui_file_new ();
> >+      make_final_cleanup (do_ui_file_delete, stream);
> >+    }
> 
> Since we're explicitly setting stream to NULL, we will always
> execute the conditional block, so it is not needed. Unless this is
> supposed to reuse a stream, but in that case the code would be
> incorrect.
> 

It is a reused null stream.  Different parts of gdb can use it.  Why
is it incorrect?
  

On 01/18/2017 08:45 AM, Yao Qi wrote:
> On 17-01-17 07:49:07, Luis Machado wrote:
>>>
>>> +struct ui_file *
>>> +null_stream (void)
>>> +{
>>> +  static struct ui_file *stream = NULL;
>>> +
>>> +  if (stream == NULL)
>>> +    {
>>> +      stream = ui_file_new ();
>>> +      make_final_cleanup (do_ui_file_delete, stream);
>>> +    }
>>
>> Since we're explicitly setting stream to NULL, we will always
>> execute the conditional block, so it is not needed. Unless this is
>> supposed to reuse a stream, but in that case the code would be
>> incorrect.
>>
>
> It is a reused null stream.  Different parts of gdb can use it.  Why
> is it incorrect?
>

We're setting stream to NULL at the top of the function and then 
checking if it is NULL (it obviously is) right after that? Am i missing 
something?
  

On 2017-01-18 09:53, Luis Machado wrote:
> We're setting stream to NULL at the top of the function and then
> checking if it is NULL (it obviously is) right after that? Am i
> missing something?

The "static" :).
  

On 01/18/2017 08:57 AM, Simon Marchi wrote:
> On 2017-01-18 09:53, Luis Machado wrote:
>> We're setting stream to NULL at the top of the function and then
>> checking if it is NULL (it obviously is) right after that? Am i
>> missing something?
>
> The "static" :).

Oh, that's what i was missing!

Can't we have a clearer way of reusing this stream? It looks cryptic 
this way (at least i think it looks cryptic).
  

On 2017-01-18 10:01, Luis Machado wrote:
> On 01/18/2017 08:57 AM, Simon Marchi wrote:
>> On 2017-01-18 09:53, Luis Machado wrote:
>>> We're setting stream to NULL at the top of the function and then
>>> checking if it is NULL (it obviously is) right after that? Am i
>>> missing something?
>> 
>> The "static" :).
> 
> Oh, that's what i was missing!
> 
> Can't we have a clearer way of reusing this stream? It looks cryptic
> this way (at least i think it looks cryptic).

With Pedro's ui_file c++ification series, there is a null_file 
specialization of ui_file, and it's just a global object that you can 
reference.

Search for "null_file null_stream" in

   https://sourceware.org/ml/gdb-patches/2017-01/msg00312.html
  

On 01/18/2017 09:18 AM, Simon Marchi wrote:
> On 2017-01-18 10:01, Luis Machado wrote:
>> On 01/18/2017 08:57 AM, Simon Marchi wrote:
>>> On 2017-01-18 09:53, Luis Machado wrote:
>>>> We're setting stream to NULL at the top of the function and then
>>>> checking if it is NULL (it obviously is) right after that? Am i
>>>> missing something?
>>>
>>> The "static" :).
>>
>> Oh, that's what i was missing!
>>
>> Can't we have a clearer way of reusing this stream? It looks cryptic
>> this way (at least i think it looks cryptic).
>
> With Pedro's ui_file c++ification series, there is a null_file
> specialization of ui_file, and it's just a global object that you can
> reference.
>
> Search for "null_file null_stream" in
>
>   https://sourceware.org/ml/gdb-patches/2017-01/msg00312.html

That is perfectly fine. The cryptic bit i was referring to was 
declaring/initializing a static variable inside this particular function.

It ought to be possible to initialize the static variable somewhere else 
and only do the null check/allocation in the function? Compilers will 
often zero these out too, so initializing to NULL may not even be needed?

The fact that the initialization only happens once but the source line 
is there forever can cause some confusion.
  

On 2017-01-18 10:28, Luis Machado wrote:
> That is perfectly fine. The cryptic bit i was referring to was
> declaring/initializing a static variable inside this particular
> function.

Indeed, and I pointed to Pedro's patch because it removes that.  The 
global object is statically allocated and is constructed at startup, so 
there's no more checking if it's initialized nor static variable inside 
a function.

> It ought to be possible to initialize the static variable somewhere
> else and only do the null check/allocation in the function? Compilers
> will often zero these out too, so initializing to NULL may not even be
> needed?

I think this is a common pattern when you want to have a singleton with 
lazy initialization, but I agree that it can be confusing.  The 
variables with static storage duration are put in .bss (since they are 
not in .data), so yes they'd be initialized to 0 automatically, I 
believe.  I prefer to see the = NULL though.

Of course we could move the variable outside the scope of the function, 
but then it would be possible to reference it from other functions.  The 
goal of declaring it in the function is that the only way to reach it is 
by calling the function it's in.

> The fact that the initialization only happens once but the source line
> is there forever can cause some confusion.

Well, you can thank Mr Ritchie & friends I suppose :).
  

On 01/18/2017 09:54 AM, Simon Marchi wrote:
> On 2017-01-18 10:28, Luis Machado wrote:
>> That is perfectly fine. The cryptic bit i was referring to was
>> declaring/initializing a static variable inside this particular
>> function.
>
> Indeed, and I pointed to Pedro's patch because it removes that.  The
> global object is statically allocated and is constructed at startup, so
> there's no more checking if it's initialized nor static variable inside
> a function.
>
>> It ought to be possible to initialize the static variable somewhere
>> else and only do the null check/allocation in the function? Compilers
>> will often zero these out too, so initializing to NULL may not even be
>> needed?
>
> I think this is a common pattern when you want to have a singleton with
> lazy initialization, but I agree that it can be confusing.  The
> variables with static storage duration are put in .bss (since they are
> not in .data), so yes they'd be initialized to 0 automatically, I
> believe.  I prefer to see the = NULL though.
>
> Of course we could move the variable outside the scope of the function,
> but then it would be possible to reference it from other functions.  The
> goal of declaring it in the function is that the only way to reach it is
> by calling the function it's in.
>
>> The fact that the initialization only happens once but the source line
>> is there forever can cause some confusion.
>
> Well, you can thank Mr Ritchie & friends I suppose :).
>

That's fine. If we're sticking with it, i'd at least add a comment for 
the sake of making it more obvious what the intentions for such a 
variable are and the fact it will outlive the function itself.