On 04/13/2017 05:15 AM, Tom Tromey wrote:
> From: Tom Tromey <tromey@redhat.com>
> 
> PR cli/19551 notes that the "Reading symbols" messages can be messy, for
> example:
> 
> (gdb) file /bin/gdb
> Reading symbols from /bin/gdb...Reading symbols from /bin/gdb...(no debugging symbols found)...done.
> (no debugging symbols found)...done.
> 
> In this case the first message is being interrupted by the message for
> the minidebug info; then the subsequent output is emitted strangely.
> 
> This patch changes gdb to use a progress bar when reading debug info.

Nice!

> It modifies the DWARF reader(s) to update the progress.  

What does GDB show with the reader?  Is their output now broken, same,
etc?

> Any printing
> is deferred until the first progress report, so the messages no longer
> clash.

Can you clarify what printing you're referring to?
What happens if gdb emits e.g., complaints while a progress bar
is half full?

> 
> While printing the status message it looks like:
> 
> Reading symbols from ./gdb
> [##############                                                  ]
> 
> The "#"s show the progress; these are only printed on a terminal.
> 
> When it is finished it looks like:
> 
> Reading symbols from .gnu_debugdata for /usr/bin/gdb
> Reading symbols from /bin/gdb

I played with this a bit and it looks like a real nice improvement
to me.

I noticed that if you clear the screen such that the prompt is not at
the bottom when gdb reads symbols (e.g., start gdb on itself,
run to main, do ctrl-l to clear the screen, and do
"nosharedlibrary / sharedlibrary"), and if symbol reading is quick, then
the meter appears/disappears very quickly in what looks like an
odd flash.  It looks a tiny bit annoying to me.  Though not a deal breaker
and I'll get used to it for sure.  One way around it would be to
not print the meter unless the operation is taking longer than some
minimum time, like 1s or some such.

> 
> I made the MI implementation do nothing.  MI has a
> "status-async-output" production in the grammar:
> 
> 'STATUS-ASYNC-OUTPUT ==>'
>      '[ TOKEN ] "+" ASYNC-OUTPUT NL'
> 
> ... which maybe could be used for this sort of thing.  Currently I think
> it's only used for "load" progress (see mi_load_progress); so it wasn't
> clear to me whether this would be a good idea.

I think it'd be a good idea, but of course you don't have to do
it yourself.

Looks like the testsuite changes needs some more work.  I thought
I'd see how it looks, and ran (only) the gdb.base/attach.exp test,
and got:

 Running rc/gdb/testsuite/gdb.base/attach.exp ...
 ERROR: internal buffer is full.

I'm attaching the resulting gdb.log.


> 
> gdb/ChangeLog
> 2017-04-12  Tom Tromey  <tom@tromey.com>

(I think the correct thing to do wrt to authorship/copyright
is add both email addresses as multiple authors.  See e.g.,
commit 3e29f34a4eef.)
> index 2a59869..e99fce2 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -236,6 +236,91 @@ cli_ui_out::do_redirect (ui_file *outstream)
>      m_streams.pop_back ();
>  }
>  
> +void
> +cli_ui_out::do_progress_start (const std::string &name, int should_print)

bool for "should_print" ?

> +{
> +  struct ui_file *stream = m_streams.back ();
> +  cli_progress_info meter;
> +
> +  meter.last_value = 0;

How about adding a ctor like this:

    cli_progress_info (meter_state printing_,
		       const std::string &name_)
      : printing (printing_),
        name (name_),
	last_value (0)
    {}

and then calling emplace_back further below.  You'd
need a:

  meter_state printing;

temporary ...

> +  meter.name = name;
> +  if (!ui_file_isatty (stream))
> +    {
> +      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
> +      gdb_flush (stream);

(Note these could use the isatty/printf/flush methods of ui_file.
Likewise throughout.)

> +      meter.printing = WORKING;

... for these, of course.

> +    }
> +  else
> +    {
> +      /* Don't actually emit anything until the first call notify us
> +	 of progress.  This makes it so a second progress message can
> +	 be started before the first one has been notified, without
> +	 messy output.  */
> +      meter.printing = should_print ? START : NO_PRINT;

I find the START etc names to bit too generic to be directly
in cli_ui_out scope.  I think they should be either METER_START etc.,
or meter_state should be an enum class, or meter_state stays a
regular enum but is moved to cli_progress_info, perhaps.

> +    }
> +
> +  m_meters.push_back (meter);
> +}
> +
> +void
> +cli_ui_out::do_progress_notify (double howmuch)
> +{
> +  struct ui_file *stream = m_streams.back ();

We've dropped most of the redundant "struct" in "struct ui_file"
in cli-out.c/h recently.  Let's not add it back in new code.

> +  cli_progress_info &meter (m_meters.back ());

Below you've written this as:

    cli_progress_info &meter = m_meters.back ();

I prefer the latter using =, as it's easier to reason
about.

> +
> +  if (meter.printing == NO_PRINT)
> +    return;
> +
> +  if (meter.printing == START)
> +    {
> +      fprintf_unfiltered (stream, "%s\n", meter.name.c_str ());
> +      gdb_flush (stream);
> +      meter.printing = WORKING;
> +    }
> +
> +  if (ui_file_isatty (stream))
> +    {
> +      int i, max;
> +      int width = get_chars_per_line () - 3;
> +
> +      max = width * howmuch;
> +      fprintf_unfiltered (stream, "\r[");
> +      for (i = 0; i < width; ++i)

These can now be:

 int max = ...
 for (int i = 0; ...

etc., likewise throughout.

> +	fprintf_unfiltered (stream, i < max ? "#" : " ");
> +      fprintf_unfiltered (stream, "]");
> +      gdb_flush (stream);
> +    }
> +}
> +


>    std::vector<ui_file *> m_streams;
>    bool m_suppress_output;
> +
> +  /* Represents the printing state of a progress meter.  */
> +  enum meter_state
> +  {
> +    /* Printing will start with the next output.  */
> +    START,
> +    /* Printing has already started.  */
> +    WORKING,
> +    /* Printing should not be done.  */
> +    NO_PRINT
> +  };
> +
> +  /* The state of a recent progress meter.  */
> +  struct cli_progress_info
> +  {
> +    /* The current state.  */
> +    enum meter_state printing;
> +    /* The name to print.  */
> +    std::string name;
> +    /* The last notification value.  */
> +    double last_value;
> +  };
> +
> +  /* Stack of progress meters.  */
> +  std::vector<cli_progress_info> m_meters;

I think it'd be good to have this comment expanded a bit to
explain why we need a stack, and what happens when we're
already printing a meter and a new stack level appears.

>  };
>  
>  extern cli_ui_out *cli_out_new (struct ui_file *stream);
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index b58d0fc..7d99c99 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c

Could you add some comments about what is considered a "step"
here, how the number of total progress steps is determined, etc.?

> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index fea94f2..490c06a 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -72,6 +72,18 @@ protected:
>    virtual bool do_is_mi_like_p () override
>    { return true; }
>  
> +  virtual void do_progress_start (const std::string &, int) override
> +  {
> +  }
> +
> +  virtual void do_progress_notify (double) override
> +  {
> +  }
> +
> +  virtual void do_progress_end () override
> +  {
> +  }

If implementations can do nothing, shouldn't "nothing" be the
default implementation?

> @@ -78,23 +79,22 @@ require_partial_symbols (struct objfile *objfile, int verbose)
>  
>        if (objfile->sf->sym_read_psymbols)
>  	{
> +	  gdb::optional<ui_out::progress_meter> meter;
> +
>  	  if (verbose)
>  	    {
> -	      printf_unfiltered (_("Reading symbols from %s..."),
> -				 objfile_name (objfile));
> -	      gdb_flush (gdb_stdout);
> +	      std::string text = (std::string ("Reading symbols from ")
> +				  + objfile_name (objfile));
> +
> +	      meter.emplace (current_uiout, text, verbose);

It looked to me that the "progress text" string is always a 
discardable string that could be moved all the way to meter.name
instead of duped.  I.e., here you'd have:

     meter.emplace (current_uiout, std::move (text), verbose);

> +  {
> +    std::string progress_text;
> +
> +    if (should_print)
> +      {
> +	if (deprecated_pre_add_symbol_hook)
> +	  deprecated_pre_add_symbol_hook (name);
> +
> +	progress_text = std::string ("Reading symbols from ") + name;
> +      }
> +    else
> +      progress_text = "";

This else branch is unnecessary.  strings are empty by default.

> +
> +    ui_out::progress_meter meter (current_uiout, progress_text, should_print);

   ui_out::progress_meter meter (current_uiout, 
                                 std::move (progress_text), 
                                 should_print);

Etc (re. move).  Of course, the progress_meter prototype
would be adjusted accordingly.


> +    syms_from_objfile (objfile, addrs, add_flags);
> +  }
>  
>    /* We now have at least a partial symbol table.  Check to see if the
>       user requested that all symbols be read on initial access via either
>       the gdb startup command line or on a per symbol file basis.  Expand
>       all partial symbol tables for this objfile if so.  */
>  
> -  if ((flags & OBJF_READNOW))
> -    {
> -      if (should_print)
> -	{
> -	  printf_unfiltered (_("expanding to full symbols..."));
> -	  wrap_here ("");
> -	  gdb_flush (gdb_stdout);
> -	}
> +  {
> +    if ((flags & OBJF_READNOW))
> +      {
> +	std::string progress_text;
>  
> -      if (objfile->sf)
> -	objfile->sf->qf->expand_all_symtabs (objfile);
> -    }
> +	if (should_print)
> +	  progress_text = std::string ("Expanding full symbols for ") + name;
> +	else
> +	  progress_text = "";
>  
> -  if (should_print && !objfile_has_symbols (objfile))
> -    {
> -      wrap_here ("");
> -      printf_unfiltered (_("(no debugging symbols found)..."));
> -      wrap_here ("");
> -    }
> +	ui_out::progress_meter meter (current_uiout, progress_text,
> +				      should_print);
> +
> +	if (objfile->sf)
> +	  objfile->sf->qf->expand_all_symtabs (objfile);
> +      }
> +  }
>  
>    if (should_print)
>      {
> +      if (!objfile_has_symbols (objfile))
> +	printf_unfiltered (_(" (no debugging symbols found)\n"));
> +
>        if (deprecated_post_add_symbol_hook)
>  	deprecated_post_add_symbol_hook ();
> -      else
> -	printf_unfiltered (_("done.\n"));
>      }
>  
>    /* We print some messages regardless of whether 'from_tty ||
> @@ -2483,10 +2486,6 @@ reread_symbols (void)
>  	  struct cleanup *old_cleanups;
>  	  struct section_offsets *offsets;
>  	  int num_offsets;
> -	  char *original_name;
> -
> -	  printf_unfiltered (_("`%s' has changed; re-reading symbols.\n"),
> -			     objfile_name (objfile));
>  
>  	  /* There are various functions like symbol_file_add,
>  	     symfile_bfd_open, syms_from_objfile, etc., which might
> @@ -2502,6 +2501,11 @@ reread_symbols (void)
>  	  /* We need to do this whenever any symbols go away.  */
>  	  make_cleanup (clear_symtab_users_cleanup, 0 /*ignore*/);
>  
> +	  std::string text
> +	    = (std::string (_("`%s' has changed; re-reading symbols.\n"))
> +	       + objfile_name (objfile));

This transformation doesn't look right.  Note the %s.

> +	  ui_out::progress_meter meter (current_uiout, text, 1);
> +
>  	  if (exec_bfd != NULL
>  	      && filename_cmp (bfd_get_filename (objfile->obfd),
>  			       bfd_get_filename (exec_bfd)) == 0)
> @@ -2548,8 +2552,7 @@ reread_symbols (void)
>  	      error (_("Can't open %s to read symbols."), obfd_filename);
>  	  }
>  


> +/* See utils.h.  */
> +
> +int
> +get_chars_per_line (void)

You can drop the "void".

> +{
> +  return chars_per_line;
> +}
> +

> +/* Return the number of characters in a line.  */
> +
> +extern int get_chars_per_line (void);
> +

Ditto.

Thanks,
Pedro Alves