* [PATCH v2] gdb/debuginfod: Rework progress updates @ 2022-01-26 0:58 Aaron Merey 2022-02-09 2:25 ` [PATCH v3] gdb: Improve debuginfod " Aaron Merey 0 siblings, 1 reply; 11+ messages in thread From: Aaron Merey @ 2022-01-26 0:58 UTC (permalink / raw) To: gdb-patches; +Cc: patrick, tom, Aaron Merey For more information regarding v1 see https://sourceware.org/pipermail/gdb-patches/2022-January/185034.html Previously debuginfod would use a progress bar to indicate the progress of each download: Downloading 1.23 MB separate debug info for /lib/libxyz.so.1... [############ ] This format assumed that an accurate transfer size would always become available. Progress updates would not start printing until the size could be displayed. However there are cases where the total size of the transfer is not available. In this case gdb would appear to hang while the transfer occured. Additionally there are cases where the transfer size was reported to gdb as -1. This resulted in progress updates indicating that a file of size -0.00 MB is downloading. This patch reworks progress updates to account for cases where the transfer size is not known. It also ensures that progress is promptly reported and it provides ways to update progress output when new information becomes available. When the transfer size is not known, a progress throbber now spins in front of the description of the download in order to indicate that the transfer is ongoing. The throbber consists of the characters -\|/ cycling at a rate of one tick per second. <THROBBER> Downloading separate debug info for /lib/libxyz.so.1 When the transfer size is known, the percentage of the transfer that has completed will be displayed in front of the description of the download. (34%) Downloading 1.23 MB separate debug info for /lib/libxyz.so.1 Progress output is written on a single line that is overwritten with whitespace and rewritten to for each update or tick of the throbber. This facilitates seamlessly switching from a throbber to a percentage indicator part way through the transfer. To reduce visual noise, when a transfer completes without an error the line of progress output is again overwritten with whitespace. This prevents many lines of progress output from accumulating when many debuginfo files are downloaded. In case a user would like to see a persistant record of the files gdb aquired through debuginfod, an additional 'set debuginfod verbose N' setting has been added. If N is at least 2, then the following will be printed for each file that is downloaded or aquired from a local debuginfod cache: Retrieved 1.23 MB separate debug info for /lib/libxyz.so.1 When --interpreter=mi or the output stream is not a tty, progress updates consist of the following printed at the start of each download: Downloading separate debug info for /lib/libxyz.so.1... No further progress updating will occur in this case. --- gdb/cli-out.c | 149 +++++++++++++++++++++++++++++---------- gdb/cli-out.h | 30 +++----- gdb/debuginfod-support.c | 140 +++++++++++++++++++++++++++--------- gdb/mi/mi-out.c | 33 +++++++++ gdb/mi/mi-out.h | 20 ++++-- gdb/ui-out.h | 62 ++++++++++++---- 6 files changed, 328 insertions(+), 106 deletions(-) diff --git a/gdb/cli-out.c b/gdb/cli-out.c index 5ff645b4a83..9cc7e456583 100644 --- a/gdb/cli-out.c +++ b/gdb/cli-out.c @@ -265,10 +265,17 @@ cli_ui_out::do_redirect (ui_file *outstream) m_streams.pop_back (); } -/* The cli_ui_out::do_progress_* functions result in the following: - - printed for tty, SHOULD_PRINT == true: - <NAME - [##### ]\r> +/* The cli_ui_out::do_progress_{start, notify} functions result in + the following: + + - printed for tty, SHOULD_PRINT == true + - next state == PERCENT: + <(XX%) NAME\r> + - next state == SPIN: + <-\|/ NAME\r> + - next state == BAR: + <NAME + [##### ]\r> - printed for tty, SHOULD_PRINT == false: <> - printed for not-a-tty: @@ -280,15 +287,14 @@ void cli_ui_out::do_progress_start (const std::string &name, bool should_print) { struct ui_file *stream = m_streams.back (); - cli_progress_info meter; + cli_progress_info info; - meter.last_value = 0; - meter.name = name; + info.name = name; if (!stream->isatty ()) { - fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); + fprintf_unfiltered (stream, "%s...\n", info.name.c_str ()); gdb_flush (stream); - meter.printing = WORKING; + info.state = progress_update::WORKING; } else { @@ -296,74 +302,141 @@ cli_ui_out::do_progress_start (const std::string &name, bool should_print) 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; + info.state = should_print ? progress_update::START + : progress_update::NO_PRINT; } - m_meters.push_back (std::move (meter)); + m_progress_info.push_back (std::move (info)); } +/* Pick a reasonable limit for the progress update length. */ +#define MAX_CHARS_PER_LINE 4096 + void -cli_ui_out::do_progress_notify (double howmuch) +cli_ui_out::do_progress_notify (double howmuch, + progress_update::state next_state) { struct ui_file *stream = m_streams.back (); - cli_progress_info &meter (m_meters.back ()); + cli_progress_info &info (m_progress_info.back ()); - if (meter.printing == NO_PRINT) + if (info.state == progress_update::NO_PRINT) return; - if (meter.printing == START) + int chars_per_line = get_chars_per_line (); + if (chars_per_line > MAX_CHARS_PER_LINE) + chars_per_line = MAX_CHARS_PER_LINE; + + if (info.state == progress_update::START) { - fprintf_unfiltered (stream, "%s\n", meter.name.c_str ()); + fprintf_unfiltered (stream, "%s", info.name.c_str ()); + if (chars_per_line <= 0) + fprintf_unfiltered (stream, "...\n"); gdb_flush (stream); - meter.printing = WORKING; + info.state = progress_update::WORKING; } - if (meter.printing == WORKING && howmuch >= 1.0) + if (chars_per_line <= 0) + return; + + if (info.state == progress_update::WORKING && howmuch >= 1.0) return; if (!stream->isatty ()) return; - int chars_per_line = get_chars_per_line (); - if (chars_per_line > 0) + if (next_state == progress_update::PERCENT) + { + fprintf_unfiltered (stream, "\r(%2.0f%%) %s", + howmuch * 100, info.name.c_str ()); + gdb_flush (stream); + info.state = progress_update::PERCENT; + } + else if (next_state == progress_update::SPIN) + { + using namespace std::chrono; + seconds diff = duration_cast<seconds> + (steady_clock::now () - info.last_update); + + /* Advance the spinner no faster than 1 tick per second. */ + if (diff.count () >= 1.0) + { + static int spin = 0; + + fprintf_unfiltered (stream, "\r%c %s", "-\\|/"[spin++ % 4], + info.name.c_str ()); + gdb_flush (stream); + info.last_update = steady_clock::now (); + } + info.state = progress_update::SPIN; + } + else if (next_state == progress_update::BAR) { int i, max; int width = chars_per_line - 3; - max = width * howmuch; + + if (info.state == progress_update::SPIN + || info.state == progress_update::PERCENT) + { + /* Ensure the progress bar prints on its own line so that + progress updates don't overwrite name. */ + fprintf_unfiltered (stream, "\r%s\n", info.name.c_str ()); + gdb_flush (stream); + } + fprintf_unfiltered (stream, "\r["); + for (i = 0; i < width; ++i) fprintf_unfiltered (stream, i < max ? "#" : " "); fprintf_unfiltered (stream, "]"); gdb_flush (stream); - meter.printing = PROGRESS; + info.state = progress_update::BAR; } + + return; +} + +/* Set NAME as the new description of the most recent progress update. */ + +void +cli_ui_out::update_progress_name (const std::string &name) +{ + cli_progress_info &info = m_progress_info.back (); + info.name = name; } +cli_ui_out::progress_update::state +cli_ui_out::get_progress_state () +{ + cli_progress_info &info = m_progress_info.back (); + return info.state; +} + +/* Clear the current line of the most recent progress update. */ + void cli_ui_out::do_progress_end () { struct ui_file *stream = m_streams.back (); - cli_progress_info &meter = m_meters.back (); + m_progress_info.pop_back (); if (!stream->isatty ()) - { - fprintf_unfiltered (stream, "\n"); - gdb_flush (stream); - } - else if (meter.printing == PROGRESS) - { - int i; - int width = get_chars_per_line () - 3; + return; - fprintf_unfiltered (stream, "\r"); - for (i = 0; i < width + 2; ++i) - fprintf_unfiltered (stream, " "); - fprintf_unfiltered (stream, "\r"); - gdb_flush (stream); - } + int chars_per_line = get_chars_per_line (); + if (chars_per_line <= 0 + || chars_per_line > MAX_CHARS_PER_LINE) + chars_per_line = MAX_CHARS_PER_LINE; + + int i; + int width = get_chars_per_line () - 3; + + fprintf_unfiltered (stream, "\r"); + for (i = 0; i < width + 2; ++i) + fprintf_unfiltered (stream, " "); + fprintf_unfiltered (stream, "\r"); - m_meters.pop_back (); + gdb_flush (stream); } /* local functions */ diff --git a/gdb/cli-out.h b/gdb/cli-out.h index 4af5524495a..ee287161cf0 100644 --- a/gdb/cli-out.h +++ b/gdb/cli-out.h @@ -21,6 +21,7 @@ #define CLI_OUT_H #include "ui-out.h" +#include <chrono> #include <vector> class cli_ui_out : public ui_out @@ -72,8 +73,10 @@ class cli_ui_out : public ui_out virtual void do_redirect (struct ui_file *outstream) override; virtual void do_progress_start (const std::string &, bool) override; - virtual void do_progress_notify (double) override; + virtual void do_progress_notify (double, progress_update::state) override; virtual void do_progress_end () override; + virtual void update_progress_name (const std::string &) override; + virtual progress_update::state get_progress_state () override; bool suppress_output () { return m_suppress_output; } @@ -85,32 +88,19 @@ class cli_ui_out : public ui_out 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, - /* Progress printing has already started. */ - PROGRESS, - /* Printing should not be done. */ - NO_PRINT - }; - - /* The state of a recent progress meter. */ + /* The state of a recent progress update. */ struct cli_progress_info { /* The current state. */ - enum meter_state printing; + progress_update::state state; /* The name to print. */ std::string name; - /* The last notification value. */ - double last_value; + /* Time of last spinner update. */ + std::chrono::steady_clock::time_point last_update; }; - /* Stack of progress meters. */ - std::vector<cli_progress_info> m_meters; + /* Stack of progress info. */ + std::vector<cli_progress_info> m_progress_info; }; extern cli_ui_out *cli_out_new (struct ui_file *stream); diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 56d8e7781c5..20cf6688ccc 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -23,6 +23,7 @@ #include "gdbsupport/gdb_optional.h" #include "cli/cli-cmds.h" #include "cli/cli-style.h" +#include "cli-out.h" #include "target.h" /* Set/show debuginfod commands. */ @@ -76,13 +77,13 @@ debuginfod_debuginfo_query (const unsigned char *build_id, struct user_data { - user_data (const char *desc, const char *fname) - : desc (desc), fname (fname) + user_data (const char *desc, string_file &styled_fname) + : desc (desc), styled_fname (styled_fname) { } const char * const desc; - const char * const fname; - gdb::optional<ui_out::progress_meter> meter; + string_file & styled_fname; + gdb::optional<ui_out::progress_update> progress; }; /* Deleter for a debuginfod_client. */ @@ -98,6 +99,25 @@ struct debuginfod_client_deleter using debuginfod_client_up = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>; +/* Convert SIZE into a unit suitable for use with progress updates. + SIZE should in given in bytes and will be converted into KB or MB. + UNIT will be set to "KB" or "MB" accordingly. */ + +static void +get_size_and_unit (double *size, const char **unit) +{ + *size /= 1024; + + /* If size is greater than 0.01 MB then set unit to MB. */ + if (*size > 10.24) + { + *size /= 1024; + *unit = "MB"; + } + else + *unit = "KB"; +} + static int progressfn (debuginfod_client *c, long cur, long total) { @@ -106,31 +126,57 @@ progressfn (debuginfod_client *c, long cur, long total) if (check_quit_flag ()) { - printf_filtered ("Cancelling download of %s %ps...\n", - data->desc, - styled_string (file_name_style.style (), data->fname)); + if (data->progress.has_value ()) + data->progress.reset (); + + printf_filtered ("Cancelled download of %s %s\n", + data->desc, data->styled_fname.c_str ()); return 1; } - if (total == 0) + if (debuginfod_verbose == 0 + || (data->progress.has_value () + && data->progress->get_state () == ui_out::progress_update::WORKING)) return 0; - if (!data->meter.has_value ()) + /* Print progress update. Include the transfer size if available. */ + if (total > 0) { - float size_in_mb = 1.0f * total / (1024 * 1024); - string_file styled_filename (current_uiout->can_emit_style_escape ()); - fprintf_styled (&styled_filename, - file_name_style.style (), - "%s", - data->fname); - std::string message - = string_printf ("Downloading %.2f MB %s %s", size_in_mb, data->desc, - styled_filename.c_str()); - data->meter.emplace (current_uiout, message, 1); + /* Transfer size is known. */ + if (!data->progress.has_value () + || data->progress->get_state () != ui_out::progress_update::PERCENT) + { + double size = (double)total; + const char *unit = ""; + + get_size_and_unit (&size, &unit); + std::string message = string_printf ("Downloading %.2f %s %s %s", + size, unit, data->desc, + data->styled_fname.c_str ()); + + if (!data->progress.has_value ()) + data->progress.emplace (current_uiout, message, 1); + else + data->progress->update_name (message); + } + + double percent = (double)cur / (double)total; + if (percent >= 0.0 || percent <= 100.0) + { + current_uiout->update_progress_percent (percent); + return 0; + } } - current_uiout->progress ((double)cur / (double)total); + if (!data->progress.has_value () + || data->progress->get_state () != ui_out::progress_update::SPIN) + { + std::string message = string_printf ("Downloading %s %s", data->desc, + data->styled_fname.c_str ()); + data->progress.emplace (current_uiout, message, 1); + } + current_uiout->update_progress_spin (); return 0; } @@ -186,6 +232,40 @@ debuginfod_is_enabled () return true; } +/* Print the result of the most recent attempted download. */ + +static void +print_outcome (user_data &data, int fd) +{ + /* Clears the current line of progress output. */ + if (data.progress.has_value ()) + data.progress.reset (); + + if (debuginfod_verbose > 1 && fd >= 0) + { + struct stat s; + + if (fstat (fd, &s) == 0) + { + double size = (double)s.st_size; + const char *unit = ""; + + get_size_and_unit (&size, &unit); + printf_filtered (_("Retrieved %.02f %s %s %s\n"), size, unit, + data.desc, data.styled_fname.c_str ()); + } + else + warning (_("Retrieved %s %s but size cannot be read: %s\n"), + data.desc, data.styled_fname.c_str (), + safe_strerror (errno)); + } + else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT) + printf_filtered (_("Download failed: %s. " \ + "Continuing without %s %s.\n"), + safe_strerror (-fd), data.desc, + data.styled_fname.c_str ()); +} + /* See debuginfod-support.h */ scoped_fd @@ -202,7 +282,9 @@ debuginfod_source_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - user_data data ("source file", srcpath); + string_file styled_srcpath (current_uiout->can_emit_style_escape ()); + fprintf_styled (&styled_srcpath, file_name_style.style (), "%s", srcpath); + user_data data ("source file", styled_srcpath); debuginfod_set_user_data (c, &data); gdb::optional<target_terminal::scoped_restore_terminal_state> term_state; @@ -218,11 +300,7 @@ debuginfod_source_query (const unsigned char *build_id, srcpath, nullptr)); debuginfod_set_user_data (c, nullptr); - - if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT) - printf_filtered (_("Download failed: %s. Continuing without source file %ps.\n"), - safe_strerror (-fd.get ()), - styled_string (file_name_style.style (), srcpath)); + print_outcome (data, fd.get ()); if (fd.get () >= 0) *destname = make_unique_xstrdup (srcpath); @@ -247,7 +325,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id, return scoped_fd (-ENOMEM); char *dname = nullptr; - user_data data ("separate debug info for", filename); + string_file styled_filename (current_uiout->can_emit_style_escape ()); + fprintf_styled (&styled_filename, file_name_style.style (), "%s", filename); + user_data data ("separate debug info for", styled_filename); debuginfod_set_user_data (c, &data); gdb::optional<target_terminal::scoped_restore_terminal_state> term_state; @@ -260,11 +340,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id, scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname)); debuginfod_set_user_data (c, nullptr); - - if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT) - printf_filtered (_("Download failed: %s. Continuing without debug info for %ps.\n"), - safe_strerror (-fd.get ()), - styled_string (file_name_style.style (), filename)); + print_outcome (data, fd.get ()); if (fd.get () >= 0) destname->reset (dname); diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c index 20c6f0f9194..ad89d7400f8 100644 --- a/gdb/mi/mi-out.c +++ b/gdb/mi/mi-out.c @@ -258,6 +258,39 @@ mi_ui_out::main_stream () return (string_file *) m_streams.back (); } +/* Indicate that a task described by NAME is in progress: + + - SHOULD_PRINT == true: + <NAME... + > + - SHOULD_PRINT == false: + <> +*/ + +void +mi_ui_out::do_progress_start (const std::string &name, bool should_print) +{ + struct ui_file *stream = gdb_stdout; + mi_progress_info info; + + if (should_print) + { + fprintf_unfiltered (stream, "%s...\n", name.c_str ()); + gdb_flush (stream); + } + info.state = progress_update::WORKING; + m_progress_info.push_back (std::move (info)); +} + +/* Get the state of the most recent progress update. */ + +mi_ui_out::progress_update::state +mi_ui_out::get_progress_state () +{ + mi_progress_info &info = m_progress_info.back (); + return info.state; +} + /* Clear the buffer. */ void diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h index 1b7fa96a182..d915b35f633 100644 --- a/gdb/mi/mi-out.h +++ b/gdb/mi/mi-out.h @@ -25,7 +25,6 @@ struct ui_out; struct ui_file; - class mi_ui_out : public ui_out { public: @@ -83,15 +82,18 @@ class mi_ui_out : public ui_out virtual bool do_is_mi_like_p () const override { return true; } - virtual void do_progress_start (const std::string &, bool) override + virtual void do_progress_start (const std::string &, bool) override; + virtual progress_update::state get_progress_state () override; + + virtual void do_progress_notify (double, progress_update::state) override { } - virtual void do_progress_notify (double) override + virtual void do_progress_end () override { } - virtual void do_progress_end () override + virtual void update_progress_name (const std::string &) override { } @@ -101,6 +103,16 @@ class mi_ui_out : public ui_out void open (const char *name, ui_out_type type); void close (ui_out_type type); + /* The state of a recent progress_update. */ + struct mi_progress_info + { + /* The current state. */ + progress_update::state state; + }; + + /* Stack of progress info. */ + std::vector<mi_progress_info> m_progress_info; + /* Convenience method that returns the MI out's string stream cast to its appropriate type. Assumes/asserts that output was not redirected. */ diff --git a/gdb/ui-out.h b/gdb/ui-out.h index 05312150c21..4c490c11b82 100644 --- a/gdb/ui-out.h +++ b/gdb/ui-out.h @@ -280,26 +280,52 @@ class ui_out escapes. */ virtual bool can_emit_style_escape () const = 0; - /* An object that starts and finishes a progress meter. */ - class progress_meter + /* An object that starts and finishes displaying progress updates. */ + class progress_update { public: + /* Represents the printing state of a progress update. */ + enum state + { + /* Printing will start with the next update. */ + START, + /* Printing has already started. */ + WORKING, + /* Progress bar printing has already started. */ + BAR, + /* Spinner printing has already started. */ + SPIN, + /* Percent printing has already started. */ + PERCENT, + /* Printing should not be done. */ + NO_PRINT + }; + /* SHOULD_PRINT indicates whether something should be printed for a tty. */ - progress_meter (struct ui_out *uiout, const std::string &name, - bool should_print) + progress_update (struct ui_out *uiout, const std::string &name, + bool should_print) : m_uiout (uiout) { m_uiout->do_progress_start (name, should_print); } - ~progress_meter () + ~progress_update () { - m_uiout->do_progress_notify (1.0); m_uiout->do_progress_end (); } - progress_meter (const progress_meter &) = delete; - progress_meter &operator= (const progress_meter &) = delete; + void update_name (std::string &name) + { + m_uiout->update_progress_name (name); + } + + state get_state () + { + return m_uiout->get_progress_state (); + } + + progress_update (const progress_update &) = delete; + progress_update &operator= (const progress_update &) = delete; private: @@ -307,10 +333,20 @@ class ui_out }; /* Emit some progress corresponding to the most recently created - progress meter. HOWMUCH may range from 0.0 to 1.0. */ - void progress (double howmuch) + progress_update object. */ + void update_progress_bar (double howmuch) + { + do_progress_notify (howmuch, progress_update::BAR); + } + + void update_progress_percent (double howmuch) + { + do_progress_notify (howmuch, progress_update::PERCENT); + } + + void update_progress_spin () { - do_progress_notify (howmuch); + do_progress_notify (0, progress_update::SPIN); } protected: @@ -348,8 +384,10 @@ class ui_out virtual void do_redirect (struct ui_file *outstream) = 0; virtual void do_progress_start (const std::string &, bool) = 0; - virtual void do_progress_notify (double) = 0; + virtual void do_progress_notify (double, progress_update::state) = 0; virtual void do_progress_end () = 0; + virtual void update_progress_name (const std::string &) = 0; + virtual progress_update::state get_progress_state () = 0; /* Set as not MI-like by default. It is overridden in subclasses if necessary. */ -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] gdb: Improve debuginfod progress updates 2022-01-26 0:58 [PATCH v2] gdb/debuginfod: Rework progress updates Aaron Merey @ 2022-02-09 2:25 ` Aaron Merey 2022-02-14 0:56 ` Patrick Monnerat 2022-03-04 18:15 ` Tom Tromey 0 siblings, 2 replies; 11+ messages in thread From: Aaron Merey @ 2022-02-09 2:25 UTC (permalink / raw) To: gdb-patches; +Cc: patrick, tom, Aaron Merey v2 can be found here: https://sourceware.org/pipermail/gdb-patches/2022-January/185432.html v3 adds truncation of progress update messages when printed to a terminal that is not wide enough to fit the message on one line. The new function build_message will try to fit as much of the untruncated message as possible. For example if the original message is: Downloading XX MB separate debug info for /aa/bb/cc/dd/ee Then possible truncated messages are: Downloading XX MB separate debug info for /aa/bb/.../ee Downloading XX MB separate debug info for ee Downloading XX MB separate debug info Downloading XX MB Downloading --- gdb/cli-out.c | 170 ++++++++++++++++++------ gdb/cli-out.h | 31 ++--- gdb/debuginfod-support.c | 276 ++++++++++++++++++++++++++++++++++----- gdb/mi/mi-out.c | 34 +++++ gdb/mi/mi-out.h | 20 ++- gdb/ui-out.h | 62 +++++++-- 6 files changed, 486 insertions(+), 107 deletions(-) diff --git a/gdb/cli-out.c b/gdb/cli-out.c index 5ff645b4a83..3523b074e75 100644 --- a/gdb/cli-out.c +++ b/gdb/cli-out.c @@ -265,10 +265,17 @@ cli_ui_out::do_redirect (ui_file *outstream) m_streams.pop_back (); } -/* The cli_ui_out::do_progress_* functions result in the following: - - printed for tty, SHOULD_PRINT == true: - <NAME - [##### ]\r> +/* The cli_ui_out::do_progress_{start, notify} functions result in + the following: + + - printed for tty, SHOULD_PRINT == true + - next state == PERCENT: + <(XX%) NAME\r> + - next state == SPIN: + <-\|/ NAME\r> + - next state == BAR: + <NAME + [##### ]\r> - printed for tty, SHOULD_PRINT == false: <> - printed for not-a-tty: @@ -280,15 +287,14 @@ void cli_ui_out::do_progress_start (const std::string &name, bool should_print) { struct ui_file *stream = m_streams.back (); - cli_progress_info meter; + cli_progress_info info; - meter.last_value = 0; - meter.name = name; + info.name = name; if (!stream->isatty ()) { - fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); + fprintf_unfiltered (stream, "%s\n", info.name.c_str ()); gdb_flush (stream); - meter.printing = WORKING; + info.state = progress_update::WORKING; } else { @@ -296,74 +302,158 @@ cli_ui_out::do_progress_start (const std::string &name, bool should_print) 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; + info.state = should_print ? progress_update::START + : progress_update::NO_PRINT; } - m_meters.push_back (std::move (meter)); + m_progress_info.push_back (std::move (info)); } +/* Pick a reasonable limit for the progress update length. */ +#define MAX_CHARS_PER_LINE 4096 + void -cli_ui_out::do_progress_notify (double howmuch) +cli_ui_out::do_progress_notify (double howmuch, + progress_update::state next_state) { struct ui_file *stream = m_streams.back (); - cli_progress_info &meter (m_meters.back ()); + cli_progress_info &info (m_progress_info.back ()); - if (meter.printing == NO_PRINT) + if (info.state == progress_update::NO_PRINT) return; - if (meter.printing == START) + int chars_per_line = get_chars_per_line (); + if (chars_per_line > MAX_CHARS_PER_LINE) + chars_per_line = MAX_CHARS_PER_LINE; + + if (info.state == progress_update::START) { - fprintf_unfiltered (stream, "%s\n", meter.name.c_str ()); + fprintf_unfiltered (stream, "%s", info.name.c_str ()); + if (chars_per_line <= 0) + fprintf_unfiltered (stream, "\n"); gdb_flush (stream); - meter.printing = WORKING; + info.state = progress_update::WORKING; } - if (meter.printing == WORKING && howmuch >= 1.0) + if (chars_per_line <= 0) + return; + + if (info.state == progress_update::WORKING && howmuch >= 1.0) return; if (!stream->isatty ()) return; - int chars_per_line = get_chars_per_line (); - if (chars_per_line > 0) + if (next_state == progress_update::PERCENT) + { + fprintf_unfiltered (stream, "\r(%2.0f%%) %s", + howmuch * 100, info.name.c_str ()); + gdb_flush (stream); + info.state = progress_update::PERCENT; + } + else if (next_state == progress_update::SPIN) + { + using namespace std::chrono; + seconds diff = duration_cast<seconds> + (steady_clock::now () - info.last_update); + + /* Advance the spinner no faster than 1 tick per second. */ + if (diff.count () >= 1.0) + { + static int spin = 0; + + fprintf_unfiltered (stream, "\r%c %s", "-\\|/"[spin++ % 4], + info.name.c_str ()); + gdb_flush (stream); + info.last_update = steady_clock::now (); + } + info.state = progress_update::SPIN; + } + else if (next_state == progress_update::BAR) { int i, max; int width = chars_per_line - 3; - max = width * howmuch; + + if (info.state == progress_update::SPIN + || info.state == progress_update::PERCENT) + { + /* Ensure the progress bar prints on its own line so that + progress updates don't overwrite NAME. */ + fprintf_unfiltered (stream, "\r%s\n", info.name.c_str ()); + gdb_flush (stream); + } + fprintf_unfiltered (stream, "\r["); + for (i = 0; i < width; ++i) fprintf_unfiltered (stream, i < max ? "#" : " "); fprintf_unfiltered (stream, "]"); gdb_flush (stream); - meter.printing = PROGRESS; + info.state = progress_update::BAR; } + + return; } +/* Clear the current line of the most recent progress update. Overwrites + the current line with whitespace. */ + void -cli_ui_out::do_progress_end () +cli_ui_out::clear_current_line () { struct ui_file *stream = m_streams.back (); - cli_progress_info &meter = m_meters.back (); + int chars_per_line = get_chars_per_line (); - if (!stream->isatty ()) - { - fprintf_unfiltered (stream, "\n"); - gdb_flush (stream); - } - else if (meter.printing == PROGRESS) - { - int i; - int width = get_chars_per_line () - 3; + if (chars_per_line <= 0 + || chars_per_line > MAX_CHARS_PER_LINE) + chars_per_line = MAX_CHARS_PER_LINE; - fprintf_unfiltered (stream, "\r"); - for (i = 0; i < width + 2; ++i) - fprintf_unfiltered (stream, " "); - fprintf_unfiltered (stream, "\r"); - gdb_flush (stream); - } + int i; + int width = chars_per_line; + + fprintf_unfiltered (stream, "\r"); + for (i = 0; i < width; ++i) + fprintf_unfiltered (stream, " "); + fprintf_unfiltered (stream, "\r"); + + gdb_flush (stream); +} + +/* Set NAME as the new description of the most recent progress update. */ + +void +cli_ui_out::update_progress_name (const std::string &name) +{ + struct ui_file *stream = m_streams.back (); + cli_progress_info &info = m_progress_info.back (); + info.name = name; + + if (stream->isatty ()) + clear_current_line (); +} + +/* Get the current state of the most recent progress update. */ + +cli_ui_out::progress_update::state +cli_ui_out::get_progress_state () +{ + cli_progress_info &info = m_progress_info.back (); + return info.state; +} + + +/* Remove the most recent progress update from the stack and + overwrite the current line with whitespace. */ + +void +cli_ui_out::do_progress_end () +{ + struct ui_file *stream = m_streams.back (); + m_progress_info.pop_back (); - m_meters.pop_back (); + if (stream->isatty ()) + clear_current_line (); } /* local functions */ diff --git a/gdb/cli-out.h b/gdb/cli-out.h index 4af5524495a..2afd5b32706 100644 --- a/gdb/cli-out.h +++ b/gdb/cli-out.h @@ -21,6 +21,7 @@ #define CLI_OUT_H #include "ui-out.h" +#include <chrono> #include <vector> class cli_ui_out : public ui_out @@ -72,8 +73,10 @@ class cli_ui_out : public ui_out virtual void do_redirect (struct ui_file *outstream) override; virtual void do_progress_start (const std::string &, bool) override; - virtual void do_progress_notify (double) override; + virtual void do_progress_notify (double, progress_update::state) override; virtual void do_progress_end () override; + virtual void update_progress_name (const std::string &) override; + virtual progress_update::state get_progress_state () override; bool suppress_output () { return m_suppress_output; } @@ -85,32 +88,20 @@ class cli_ui_out : public ui_out 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, - /* Progress printing has already started. */ - PROGRESS, - /* Printing should not be done. */ - NO_PRINT - }; - - /* The state of a recent progress meter. */ + /* The state of a recent progress update. */ struct cli_progress_info { /* The current state. */ - enum meter_state printing; + progress_update::state state; /* The name to print. */ std::string name; - /* The last notification value. */ - double last_value; + /* Time of last spinner update. */ + std::chrono::steady_clock::time_point last_update; }; - /* Stack of progress meters. */ - std::vector<cli_progress_info> m_meters; + /* Stack of progress info. */ + std::vector<cli_progress_info> m_progress_info; + void clear_current_line (); }; extern cli_ui_out *cli_out_new (struct ui_file *stream); diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c index 56d8e7781c5..34a72618663 100644 --- a/gdb/debuginfod-support.c +++ b/gdb/debuginfod-support.c @@ -23,7 +23,9 @@ #include "gdbsupport/gdb_optional.h" #include "cli/cli-cmds.h" #include "cli/cli-style.h" +#include "cli-out.h" #include "target.h" +#include <sstream> /* Set/show debuginfod commands. */ static cmd_list_element *set_debuginfod_prefix_list; @@ -76,13 +78,13 @@ debuginfod_debuginfo_query (const unsigned char *build_id, struct user_data { - user_data (const char *desc, const char *fname) + user_data (const char *desc, std::string &fname) : desc (desc), fname (fname) { } const char * const desc; - const char * const fname; - gdb::optional<ui_out::progress_meter> meter; + std::string & fname; + gdb::optional<ui_out::progress_update> progress; }; /* Deleter for a debuginfod_client. */ @@ -98,6 +100,149 @@ struct debuginfod_client_deleter using debuginfod_client_up = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>; +/* Convert SIZE into a unit suitable for use with progress updates. + SIZE should in given in bytes and will be converted into KB or MB. + UNIT will be set to "KB" or "MB" accordingly. */ + +static void +get_size_and_unit (double *size, const char **unit) +{ + *size /= 1024; + + /* If size is greater than 0.01 MB then set unit to MB. */ + if (*size > 10.24) + { + *size /= 1024; + *unit = "MB"; + } + else + *unit = "KB"; +} + +/* Ensure the progress message can fit on a single line. Otherwise + garbled output is possible with \r. + + An example of possible truncations, starting with the original message: + "Downloading XX MB separate debug info for /aa/bb/cc/dd/ee" + "Downloading XX MB separate debug info for /aa/bb/.../ee" + "Downloading XX MB separate debug info for ee" + "Downloading XX MB separate debug info" + "Downloading XX MB" + "Downloading" + */ + +static std::string +build_message (std::string size, std::string unit, std::string desc, std::string fname) +{ + int width = get_chars_per_line (); + std::stringstream message; + + message << "Downloading"; + /* Leave room for spinner and percent indicator. */ + int message_size = message.str ().length () + 6; + + if (!size.empty () && !unit.empty ()) + { + message_size += size.size () + unit.size () + 2; + if (message_size > width) + return message.str (); + + /* "Downloading XX MB" */ + message << " " << size << " " << unit; + } + + /* If FNAME does not fit then message will end with DESC_END. + In case DESC_END is "separate debug info for", remove " for". */ + std::string desc_end = desc; + if (desc.substr (desc.size () - 4) == " for") + desc_end = desc.substr (0, desc.size () - 4); + + if (message_size + desc_end.size () + 1 > width) + return message.str (); + + string_file styled_fname (current_uiout->can_emit_style_escape ()); + if (message_size + desc.size () + fname.size () + 2 <= width) + { + /* Truncation is not necessary. Return untruncated message. + "Downloading XX MB separate debug info for /usr/libxyz.so" */ + fprintf_styled (&styled_fname, file_name_style.style (), "%s", + fname.c_str ()); + + message << " " << desc << " " << styled_fname.c_str (); + return message.str (); + } + + while (fname.back () == '/') + fname.pop_back (); + + /* Find path separators for the first, second and final components. + If FNAME does not have path separators and it does not fit in the + available space, do not include it in message. */ + size_t sep1 = fname.find ('/'); + if (sep1 == std::string::npos) + { + message << " " << desc_end; + return message.str (); + } + + size_t sep2 = fname.find ('/', sep1 + 1); + size_t sep3; + if (sep2 == std::string::npos) + sep3 = std::string::npos; + else + sep3 = fname.find ('/', sep2 + 1); + size_t seplast = fname.find_last_of ('/'); + + /* If the first, second, and final path components are distinct, try to + truncate FNAME so that it fits in the available space. Preserve the + first, second and final path components. For example, + "/aa/bb/cc/dd/ee" becomes "/aa/bb/.../ee" and + "../aa/bb/cc/dd/" becomes "../aa/.../ee" */ + std::stringstream trunc; + if (sep2 != sep3 && sep2 != seplast && sep2 != std::string::npos) + { + std::stringstream fnametmp; + + if (sep1 == 0 && sep3 != seplast && sep3 != std::string::npos) + fnametmp << fname.substr (0, sep3 + 1) + << "..." << fname.substr (seplast); + else if (sep1 != 0) + fnametmp << fname.substr (0, sep2 + 1) + << "..." << fname.substr (seplast); + + if (!fnametmp.str ().empty ()) + { + trunc << " " << desc << " "; + if (message_size + trunc.str ().size () + fnametmp.str ().size () <= width) + { + fprintf_styled (&styled_fname, file_name_style.style (), "%s", + fnametmp.str ().c_str ()); + message << trunc.str () << styled_fname.c_str (); + return message.str (); + } + } + } + + /* The first, second and final components are not distinct or + "/aa/bb/.../ee" does not fit. Try "ee" instead. */ + trunc.str (""); + trunc << " " << desc << " "; + fname = fname.substr (seplast + 1); + if (message_size + trunc.str ().size () + fname.size () <= width) + { + fprintf_styled (&styled_fname, file_name_style.style (), "%s", + fname.c_str ()); + message << trunc.str () << styled_fname.c_str (); + return message.str (); + } + + /* We aren't able to fit anything from FNAME. End message with DESC_END + since we already confirmed it will fit. */ + message << " " << desc_end; + return message.str (); +} + + static int progressfn (debuginfod_client *c, long cur, long total) { @@ -106,31 +251,68 @@ progressfn (debuginfod_client *c, long cur, long total) if (check_quit_flag ()) { - printf_filtered ("Cancelling download of %s %ps...\n", - data->desc, - styled_string (file_name_style.style (), data->fname)); + if (data->progress.has_value ()) + data->progress.reset (); + + string_file styled_fname (current_uiout->can_emit_style_escape ()); + fprintf_styled (&styled_fname, file_name_style.style (), "%s", + data->fname.c_str ()); + + printf_filtered ("Cancelled download of %s %s\n", + data->desc, styled_fname.c_str ()); return 1; } - if (total == 0) + if (debuginfod_verbose == 0 + || (data->progress.has_value () + && data->progress->get_state () == ui_out::progress_update::WORKING)) return 0; - if (!data->meter.has_value ()) + /* Print progress update. Include the transfer size if available. */ + if (total > 0) { - float size_in_mb = 1.0f * total / (1024 * 1024); - string_file styled_filename (current_uiout->can_emit_style_escape ()); - fprintf_styled (&styled_filename, - file_name_style.style (), - "%s", - data->fname); - std::string message - = string_printf ("Downloading %.2f MB %s %s", size_in_mb, data->desc, - styled_filename.c_str()); - data->meter.emplace (current_uiout, message, 1); + /* Transfer size is known. */ + double percent = (double)cur / (double)total; + + if (percent >= 0.0 && percent <= 1.0) + { + if (!data->progress.has_value () + || data->progress->get_state () + != ui_out::progress_update::PERCENT) + { + double size = (double)total; + const char *unit = ""; + + get_size_and_unit (&size, &unit); + std::string fsize = string_printf ("%.2f", size); + std::string message = build_message (fsize, unit, data->desc, + data->fname); + if (!data->progress.has_value ()) + data->progress.emplace (current_uiout, message, 1); + else + data->progress->update_name (message); + } + + /* Ensure PERCENT doesn't require three digits to display. */ + if (percent > 0.99 && percent <= 1.0) + percent = .99; + current_uiout->update_progress_percent (percent); + return 0; + } } - current_uiout->progress ((double)cur / (double)total); + if (!data->progress.has_value () + || data->progress->get_state () != ui_out::progress_update::SPIN) + { + std::string message = build_message ("", "", data->desc, data->fname); + + if (!data->progress.has_value ()) + data->progress.emplace (current_uiout, message, 1); + else + data->progress->update_name (message); + } + current_uiout->update_progress_spin (); return 0; } @@ -186,6 +368,44 @@ debuginfod_is_enabled () return true; } +/* Print the result of the most recent attempted download. */ + +static void +print_outcome (user_data &data, int fd) +{ + /* Clears the current line of progress output. */ + if (data.progress.has_value ()) + data.progress.reset (); + + string_file styled_fname (current_uiout->can_emit_style_escape ()); + fprintf_styled (&styled_fname, file_name_style.style (), "%s", + data.fname.c_str ()); + + if (debuginfod_verbose > 1 && fd >= 0) + { + struct stat s; + + if (fstat (fd, &s) == 0) + { + double size = (double)s.st_size; + const char *unit = ""; + + get_size_and_unit (&size, &unit); + printf_filtered (_("Retrieved %.02f %s %s %s\n"), size, unit, + data.desc, styled_fname.c_str ()); + } + else + warning (_("Retrieved %s %s but size cannot be read: %s\n"), + data.desc, styled_fname.c_str (), + safe_strerror (errno)); + } + else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT) + printf_filtered (_("Download failed: %s. " \ + "Continuing without %s %s.\n"), + safe_strerror (-fd), data.desc, + styled_fname.c_str ()); +} + /* See debuginfod-support.h */ scoped_fd @@ -202,7 +422,8 @@ debuginfod_source_query (const unsigned char *build_id, if (c == nullptr) return scoped_fd (-ENOMEM); - user_data data ("source file", srcpath); + std::string fname = srcpath; + user_data data ("source file", fname); debuginfod_set_user_data (c, &data); gdb::optional<target_terminal::scoped_restore_terminal_state> term_state; @@ -218,11 +439,7 @@ debuginfod_source_query (const unsigned char *build_id, srcpath, nullptr)); debuginfod_set_user_data (c, nullptr); - - if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT) - printf_filtered (_("Download failed: %s. Continuing without source file %ps.\n"), - safe_strerror (-fd.get ()), - styled_string (file_name_style.style (), srcpath)); + print_outcome (data, fd.get ()); if (fd.get () >= 0) *destname = make_unique_xstrdup (srcpath); @@ -247,7 +464,8 @@ debuginfod_debuginfo_query (const unsigned char *build_id, return scoped_fd (-ENOMEM); char *dname = nullptr; - user_data data ("separate debug info for", filename); + std::string fname = filename; + user_data data ("separate debug info for", fname); debuginfod_set_user_data (c, &data); gdb::optional<target_terminal::scoped_restore_terminal_state> term_state; @@ -260,11 +478,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id, scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len, &dname)); debuginfod_set_user_data (c, nullptr); - - if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT) - printf_filtered (_("Download failed: %s. Continuing without debug info for %ps.\n"), - safe_strerror (-fd.get ()), - styled_string (file_name_style.style (), filename)); + print_outcome (data, fd.get ()); if (fd.get () >= 0) destname->reset (dname); diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c index 20c6f0f9194..a7f697b4f28 100644 --- a/gdb/mi/mi-out.c +++ b/gdb/mi/mi-out.c @@ -258,6 +258,40 @@ mi_ui_out::main_stream () return (string_file *) m_streams.back (); } +/* Indicate that a task described by NAME is in progress: + + - SHOULD_PRINT == true: + <NAME + > + - SHOULD_PRINT == false: + <> +*/ + +void +mi_ui_out::do_progress_start (const std::string &name, bool should_print) +{ + struct ui_file *stream = gdb_stdout; + mi_progress_info info; + + if (should_print) + { + fprintf_unfiltered (stream, "%s\n", name.c_str ()); + gdb_flush (stream); + } + + info.state = progress_update::WORKING; + m_progress_info.push_back (std::move (info)); +} + +/* Get the state of the most recent progress update. */ + +mi_ui_out::progress_update::state +mi_ui_out::get_progress_state () +{ + mi_progress_info &info = m_progress_info.back (); + return info.state; +} + /* Clear the buffer. */ void diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h index 1b7fa96a182..d915b35f633 100644 --- a/gdb/mi/mi-out.h +++ b/gdb/mi/mi-out.h @@ -25,7 +25,6 @@ struct ui_out; struct ui_file; - class mi_ui_out : public ui_out { public: @@ -83,15 +82,18 @@ class mi_ui_out : public ui_out virtual bool do_is_mi_like_p () const override { return true; } - virtual void do_progress_start (const std::string &, bool) override + virtual void do_progress_start (const std::string &, bool) override; + virtual progress_update::state get_progress_state () override; + + virtual void do_progress_notify (double, progress_update::state) override { } - virtual void do_progress_notify (double) override + virtual void do_progress_end () override { } - virtual void do_progress_end () override + virtual void update_progress_name (const std::string &) override { } @@ -101,6 +103,16 @@ class mi_ui_out : public ui_out void open (const char *name, ui_out_type type); void close (ui_out_type type); + /* The state of a recent progress_update. */ + struct mi_progress_info + { + /* The current state. */ + progress_update::state state; + }; + + /* Stack of progress info. */ + std::vector<mi_progress_info> m_progress_info; + /* Convenience method that returns the MI out's string stream cast to its appropriate type. Assumes/asserts that output was not redirected. */ diff --git a/gdb/ui-out.h b/gdb/ui-out.h index 05312150c21..4c490c11b82 100644 --- a/gdb/ui-out.h +++ b/gdb/ui-out.h @@ -280,26 +280,52 @@ class ui_out escapes. */ virtual bool can_emit_style_escape () const = 0; - /* An object that starts and finishes a progress meter. */ - class progress_meter + /* An object that starts and finishes displaying progress updates. */ + class progress_update { public: + /* Represents the printing state of a progress update. */ + enum state + { + /* Printing will start with the next update. */ + START, + /* Printing has already started. */ + WORKING, + /* Progress bar printing has already started. */ + BAR, + /* Spinner printing has already started. */ + SPIN, + /* Percent printing has already started. */ + PERCENT, + /* Printing should not be done. */ + NO_PRINT + }; + /* SHOULD_PRINT indicates whether something should be printed for a tty. */ - progress_meter (struct ui_out *uiout, const std::string &name, - bool should_print) + progress_update (struct ui_out *uiout, const std::string &name, + bool should_print) : m_uiout (uiout) { m_uiout->do_progress_start (name, should_print); } - ~progress_meter () + ~progress_update () { - m_uiout->do_progress_notify (1.0); m_uiout->do_progress_end (); } - progress_meter (const progress_meter &) = delete; - progress_meter &operator= (const progress_meter &) = delete; + void update_name (std::string &name) + { + m_uiout->update_progress_name (name); + } + + state get_state () + { + return m_uiout->get_progress_state (); + } + + progress_update (const progress_update &) = delete; + progress_update &operator= (const progress_update &) = delete; private: @@ -307,10 +333,20 @@ class ui_out }; /* Emit some progress corresponding to the most recently created - progress meter. HOWMUCH may range from 0.0 to 1.0. */ - void progress (double howmuch) + progress_update object. */ + void update_progress_bar (double howmuch) + { + do_progress_notify (howmuch, progress_update::BAR); + } + + void update_progress_percent (double howmuch) + { + do_progress_notify (howmuch, progress_update::PERCENT); + } + + void update_progress_spin () { - do_progress_notify (howmuch); + do_progress_notify (0, progress_update::SPIN); } protected: @@ -348,8 +384,10 @@ class ui_out virtual void do_redirect (struct ui_file *outstream) = 0; virtual void do_progress_start (const std::string &, bool) = 0; - virtual void do_progress_notify (double) = 0; + virtual void do_progress_notify (double, progress_update::state) = 0; virtual void do_progress_end () = 0; + virtual void update_progress_name (const std::string &) = 0; + virtual progress_update::state get_progress_state () = 0; /* Set as not MI-like by default. It is overridden in subclasses if necessary. */ -- 2.34.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-02-09 2:25 ` [PATCH v3] gdb: Improve debuginfod " Aaron Merey @ 2022-02-14 0:56 ` Patrick Monnerat 2022-02-16 2:09 ` Aaron Merey 2022-03-04 18:15 ` Tom Tromey 1 sibling, 1 reply; 11+ messages in thread From: Patrick Monnerat @ 2022-02-14 0:56 UTC (permalink / raw) To: Aaron Merey, gdb-patches; +Cc: tom Hi Aaron, I finally found time to check your patch with Insight and it seems to work. Please find some questions/remarks embedded in your code. > + /* Transfer size is known. */ > + double percent = (double)cur / (double)total; The variable name is confusing as it is <= 1.0. > + > + if (percent >= 0.0 && percent <= 1.0) I don't think this test is needed: cur and total are obtained (indirectly) from curl and IMHO you can trust it. > + progress_update object. */ > + void update_progress_bar (double howmuch) This is never called! why do you provide both PERCENT and (unused) BAR? This is a bit confusing. Thanks for your work on it. Patrick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-02-14 0:56 ` Patrick Monnerat @ 2022-02-16 2:09 ` Aaron Merey 2022-02-16 10:38 ` Patrick Monnerat 0 siblings, 1 reply; 11+ messages in thread From: Aaron Merey @ 2022-02-16 2:09 UTC (permalink / raw) To: Patrick Monnerat; +Cc: gdb-patches, Tom Tromey Hi Patrick, Thanks for taking another look. On Sun, Feb 13, 2022 at 7:56 PM Patrick Monnerat <patrick@monnerat.net> wrote: > > + /* Transfer size is known. */ > > + double percent = (double)cur / (double)total; > The variable name is confusing as it is <= 1.0. update_progress_percent uses the name "howmuch" for this. It would be better if "howmuch" was used here too. > > + > > + if (percent >= 0.0 && percent <= 1.0) > I don't think this test is needed: cur and total are obtained > (indirectly) from curl and IMHO you can trust it. I've experienced at least one case where percent was > 1.0. I haven't been able to reproduce it because it seemed to coincide with a network hiccup. If percent isn't between 0 and 1 then we default to the spinner to avoid printing a message with a nonsensical completion percentage. > > + progress_update object. */ > > + void update_progress_bar (double howmuch) > > This is never called! why do you provide both PERCENT and (unused) BAR? > This is a bit confusing. The progress update message originally included the bar but I now want to only print messages that fit entirely on one line. This makes it possible to rewrite an entire message with transfer size information once it becomes available. Because the progress bar was already implemented I figured I'd leave it in case it ends up serving a purpose in the future. Aaron ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-02-16 2:09 ` Aaron Merey @ 2022-02-16 10:38 ` Patrick Monnerat 2022-02-17 16:06 ` Aaron Merey 0 siblings, 1 reply; 11+ messages in thread From: Patrick Monnerat @ 2022-02-16 10:38 UTC (permalink / raw) To: Aaron Merey; +Cc: gdb-patches, Tom Tromey Hi Aaron, On 2/16/22 03:09, Aaron Merey wrote: > Thanks for taking another look. You're welcome. >>> + /* Transfer size is known. */ >>> + double percent = (double)cur / (double)total; >> The variable name is confusing as it is <= 1.0. > update_progress_percent uses the name "howmuch" for this. It would be > better if "howmuch" was used here too. Good choice. >>> + if (percent >= 0.0 && percent <= 1.0) >> I don't think this test is needed: cur and total are obtained >> (indirectly) from curl and IMHO you can trust it. > I've experienced at least one case where percent was > 1.0. I haven't > been able to reproduce it because it seemed to coincide with a network > hiccup. Very strange! TCP is supposed to guarantee no duplicate data reception. If this really occurs, debuginfo data are probably corrupted too! > + progress_update object. */ > + void update_progress_bar (double howmuch) >> This is never called! why do you provide both PERCENT and (unused) BAR? >> This is a bit confusing. > The progress update message originally included the bar but I now want to > only print messages that fit entirely on one line. This makes it possible > to rewrite an entire message with transfer size information once it becomes > available. Because the progress bar was already implemented I figured I'd > leave it in case it ends up serving a purpose in the future. Thanks for this precision. Maybe put it in a comment? Cheers, Patrick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-02-16 10:38 ` Patrick Monnerat @ 2022-02-17 16:06 ` Aaron Merey 0 siblings, 0 replies; 11+ messages in thread From: Aaron Merey @ 2022-02-17 16:06 UTC (permalink / raw) To: Patrick Monnerat; +Cc: gdb-patches, Tom Tromey On Wed, Feb 16, 2022 at 5:38 AM Patrick Monnerat <patrick@monnerat.net> wrote: > > I've experienced at least one case where percent was > 1.0. I haven't > > been able to reproduce it because it seemed to coincide with a network > > hiccup. > Very strange! TCP is supposed to guarantee no duplicate data reception. > If this really occurs, debuginfo data are probably corrupted too! I have not seen any corrupt debuginfo acquired from debuginfod. Each time debuginfo is downloaded there is a check to make sure it has the expected build-id. The integrity of the debuginfo is also verified using the CRC in the parent file's .gnu_debuglink section. Aaron ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-02-09 2:25 ` [PATCH v3] gdb: Improve debuginfod " Aaron Merey 2022-02-14 0:56 ` Patrick Monnerat @ 2022-03-04 18:15 ` Tom Tromey 2022-03-09 1:26 ` Aaron Merey 1 sibling, 1 reply; 11+ messages in thread From: Tom Tromey @ 2022-03-04 18:15 UTC (permalink / raw) To: Aaron Merey via Gdb-patches; +Cc: Aaron Merey, tom >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes: Aaron> The new function build_message will try to fit as much of the Aaron> untruncated message as possible. For example if the original Aaron> message is: Aaron> Downloading XX MB separate debug info for /aa/bb/cc/dd/ee Instead of this, which seems like it can hide useful info from the user, what do you think about a "cylon"-style display: Downloading mumble.so: [ # ] ... where the '#' moves back and forth inside the []. See the "indeterminate" example here: https://github.com/p-ranav/indicators (That page made me LOL with the emoji rocket, maybe gdb should be more emojified.) Anyway it seems like this sort of change would avoid some hair in the implementation. Aaron> if (!stream->isatty ()) Aaron> { Aaron> - fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); Aaron> + fprintf_unfiltered (stream, "%s\n", info.name.c_str ()); I suspect the flush can be removed here, it's normally only needed when the previous print doesn't end in a newline. Aaron> + info.state = should_print ? progress_update::START Aaron> + : progress_update::NO_PRINT; gdb style is to parenthesize the RHS of the assignment in a case like this; something like: info.state = (should_print ? progress_update::START : progress_update::NO_PRINT); Aaron> +/* Get the current state of the most recent progress update. */ Aaron> + Aaron> +cli_ui_out::progress_update::state Aaron> +cli_ui_out::get_progress_state () Aaron> +{ I didn't really get this part of the design. It seems to me that the debuginfod support code just wants to notify the user of some progress. It might know how much data to expect, or it might not -- so when first starting the notification, it seems like it can just explain this. After that, it could just say "got xyz", and the progress meter itself can be responsible for all state changes, UI display, etc. That is, I don't understand why this needs to be exported from the cli-out. Aaron> + if (data->progress.has_value ()) Aaron> + data->progress.reset (); In retrospect I also don't understand why the progress meter is even an optional. It seems like one could be made unconditionally and then the display (or not) left to the ui-out implementation. Aaron> + else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT) Aaron> + printf_filtered (_("Download failed: %s. " \ Aaron> + "Continuing without %s %s.\n"), Aaron> + safe_strerror (-fd), data.desc, Aaron> + styled_fname.c_str ()); It seems like a download failure should be reported even in non-verbose mode. Aaron> +void Aaron> +mi_ui_out::do_progress_start (const std::string &name, bool should_print) I tend to doubt that MI should be changed. Technically MI does have a progress notification approach, see mi_load_progress. I don't know if anything MI consumer actually uses this, though, and so I'm not sure if it makes sense to try to wire this up to debuginfo downloads. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-03-04 18:15 ` Tom Tromey @ 2022-03-09 1:26 ` Aaron Merey 2022-03-18 19:23 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Aaron Merey @ 2022-03-09 1:26 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Hi Tom, Thanks for the review. On Fri, Mar 4, 2022 at 1:17 PM Tom Tromey <tom@tromey.com> wrote: > > >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes: > > Aaron> The new function build_message will try to fit as much of the > Aaron> untruncated message as possible. For example if the original > Aaron> message is: > Aaron> Downloading XX MB separate debug info for /aa/bb/cc/dd/ee > > Instead of this, which seems like it can hide useful info from the user, > what do you think about a "cylon"-style display: > > Downloading mumble.so: > [ # ] > > ... where the '#' moves back and forth inside the []. I'd like to restrict the update messages to a single line so that an entire message can be overwritten with \r. Then when many downloads occur we avoid filling the terminal with "Downloading XY MB separate debuginfo for libxyz" messages. One drawback of using \r is the output becomes garbled if an update message exceeds the width of the terminal. This creates the need for message truncation which can hide useful information, like you said. However the important parts of the update message are present until the terminal is very narrow (usually <80 chars). And if a user wants to ensure they see all the details even with a narrow terminal, 'set debuginfod verbose 2' will print a persistent, untruncated message for each downloaded file regardless of terminal width: Retrieved XY MB separate debug info for /usr/lib/libxyz.so > Aaron> if (!stream->isatty ()) > Aaron> { > Aaron> - fprintf_unfiltered (stream, "%s...", meter.name.c_str ()); > Aaron> + fprintf_unfiltered (stream, "%s\n", info.name.c_str ()); > > I suspect the flush can be removed here, it's normally only needed when > the previous print doesn't end in a newline. Fixed. > Aaron> + info.state = should_print ? progress_update::START > Aaron> + : progress_update::NO_PRINT; > > gdb style is to parenthesize the RHS of the assignment in a case like > this; something like: > > info.state = (should_print > ? progress_update::START > : progress_update::NO_PRINT); Fixed. > Aaron> +/* Get the current state of the most recent progress update. */ > Aaron> + > Aaron> +cli_ui_out::progress_update::state > Aaron> +cli_ui_out::get_progress_state () > Aaron> +{ > > I didn't really get this part of the design. > > It seems to me that the debuginfod support code just wants to notify the > user of some progress. It might know how much data to expect, or it > might not -- so when first starting the notification, it seems like it > can just explain this. After that, it could just say "got xyz", and the > progress meter itself can be responsible for all state changes, UI > display, etc. That is, I don't understand why this needs to be exported > from the cli-out. Yes I think this can be reworked to avoid exporting the state information. Although it's useful to be able to check for the WORKING state in order to return early and avoid calling any update_progress_* functions. This is used when !isatty or interpreter=mi. In these cases we want only a single message to be printed when the progress_meter is created. But these conditions can be checked instead of the state. I'll change this. > > Aaron> + if (data->progress.has_value ()) > Aaron> + data->progress.reset (); > > In retrospect I also don't understand why the progress meter is even an > optional. It seems like one could be made unconditionally and then the > display (or not) left to the ui-out implementation. The meter sometimes prints a progress message when it's created but this could be done through the update_progress_* functions. Maybe this change should be implemented in another patch. > Aaron> + else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT) > Aaron> + printf_filtered (_("Download failed: %s. " \ > Aaron> + "Continuing without %s %s.\n"), > Aaron> + safe_strerror (-fd), data.desc, > Aaron> + styled_fname.c_str ()); > > It seems like a download failure should be reported even in non-verbose > mode. Fixed. > Aaron> +void > Aaron> +mi_ui_out::do_progress_start (const std::string &name, bool should_print) > > I tend to doubt that MI should be changed. > > Technically MI does have a progress notification approach, see > mi_load_progress. I don't know if anything MI consumer actually uses > this, though, and so I'm not sure if it makes sense to try to wire this > up to debuginfo downloads. These MI implementations are the easiest way to see progress updates when using gdb+debuginfod with IDEs, for instance. Otherwise I think each IDE would have to learn how to parse and display the mi_load_progress output, preferably in a way that's consistent with the CLI progress messages. Aaron ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-03-09 1:26 ` Aaron Merey @ 2022-03-18 19:23 ` Tom Tromey 2022-03-22 0:27 ` Aaron Merey 0 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2022-03-18 19:23 UTC (permalink / raw) To: Aaron Merey via Gdb-patches; +Cc: Tom Tromey, Aaron Merey >> Instead of this, which seems like it can hide useful info from the user, >> what do you think about a "cylon"-style display: >> >> Downloading mumble.so: >> [ # ] >> >> ... where the '#' moves back and forth inside the []. Aaron> I'd like to restrict the update messages to a single line so that an Aaron> entire message can be overwritten with \r. Then when many downloads Aaron> occur we avoid filling the terminal with "Downloading XY MB separate Aaron> debuginfo for libxyz" messages. Could you show what your proposed output looks like? I mean, after everything is downloaded? To me it seems like having a "Downloading..." message per .so is no big deal. gdb is often chatty. >> Technically MI does have a progress notification approach, see >> mi_load_progress. I don't know if anything MI consumer actually uses >> this, though, and so I'm not sure if it makes sense to try to wire this >> up to debuginfo downloads. Aaron> These MI implementations are the easiest way to see progress updates when Aaron> using gdb+debuginfod with IDEs, for instance. Otherwise I think each IDE Aaron> would have to learn how to parse and display the mi_load_progress output, Aaron> preferably in a way that's consistent with the CLI progress messages. I am not sure we're talking about the same thing... MI defines a progress indication message, but your patch isn't using that. So I wonder if any MI notification is needed at all. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-03-18 19:23 ` Tom Tromey @ 2022-03-22 0:27 ` Aaron Merey 2022-04-07 14:54 ` Tom Tromey 0 siblings, 1 reply; 11+ messages in thread From: Aaron Merey @ 2022-03-22 0:27 UTC (permalink / raw) To: Tom Tromey; +Cc: Aaron Merey via Gdb-patches On Fri, Mar 18, 2022 at 3:23 PM Tom Tromey <tom@tromey.com> wrote: > Aaron> I'd like to restrict the update messages to a single line so that an > Aaron> entire message can be overwritten with \r. Then when many downloads > Aaron> occur we avoid filling the terminal with "Downloading XY MB separate > Aaron> debuginfo for libxyz" messages. > > Could you show what your proposed output looks like? > I mean, after everything is downloaded? After each download, the progress update message is overwritten with whitespace. So by default there is no lasting indication that a download occured (this can be changed with 'set debuginfod verbose 2' though). For example while downloads are in progress a user might see (gdb) start Temporary breakpoint 1 at 0x40f406: file gdb.c, line 25. Starting program: /usr/local/bin/gdb \ Downloading separate debug info for /lib64/libc.so.6 where the last line is overwritten with each download's progress message. When the final download finishes, the message is overwritten with whitespace and gdb's output continues as if debuginfod wasn't used: (gdb) start Temporary breakpoint 1 at 0x40f406: file gdb.c, line 25. Starting program: /usr/local/bin/gdb [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe658) at gdb.c:25 25 { (gdb) > To me it seems like having a "Downloading..." message per .so is no big deal. > gdb is often chatty. I'd like to avoid printing a large number of "Downloading..." messages but my solution in this patch comes with the added complexity of message truncation when the terminal isn't wide enough. Unless there is a desire among users to limit these messages this way then maybe it's better to just stick to the cylon-style display you suggested. > >> Technically MI does have a progress notification approach, see > >> mi_load_progress. I don't know if anything MI consumer actually uses > >> this, though, and so I'm not sure if it makes sense to try to wire this > >> up to debuginfo downloads. > > Aaron> These MI implementations are the easiest way to see progress updates when > Aaron> using gdb+debuginfod with IDEs, for instance. Otherwise I think each IDE > Aaron> would have to learn how to parse and display the mi_load_progress output, > Aaron> preferably in a way that's consistent with the CLI progress messages. > > I am not sure we're talking about the same thing... MI defines a > progress indication message, but your patch isn't using that. So I > wonder if any MI notification is needed at all. I think I've conflated mi_ui_out with MI itself. I don't think we'll need to modify any MI notifications for these progress updates. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] gdb: Improve debuginfod progress updates 2022-03-22 0:27 ` Aaron Merey @ 2022-04-07 14:54 ` Tom Tromey 0 siblings, 0 replies; 11+ messages in thread From: Tom Tromey @ 2022-04-07 14:54 UTC (permalink / raw) To: Aaron Merey via Gdb-patches; +Cc: Tom Tromey, Aaron Merey >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes: >> To me it seems like having a "Downloading..." message per .so is no big deal. >> gdb is often chatty. Aaron> I'd like to avoid printing a large number of "Downloading..." messages but Aaron> my solution in this patch comes with the added complexity of message Aaron> truncation when the terminal isn't wide enough. Unless there is a desire Aaron> among users to limit these messages this way then maybe it's better to Aaron> just stick to the cylon-style display you suggested. Yeah, personally I think having the Downloading message is fine, so unless there are other contrary views, I'd say go with the simpler code here. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-07 14:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-26 0:58 [PATCH v2] gdb/debuginfod: Rework progress updates Aaron Merey 2022-02-09 2:25 ` [PATCH v3] gdb: Improve debuginfod " Aaron Merey 2022-02-14 0:56 ` Patrick Monnerat 2022-02-16 2:09 ` Aaron Merey 2022-02-16 10:38 ` Patrick Monnerat 2022-02-17 16:06 ` Aaron Merey 2022-03-04 18:15 ` Tom Tromey 2022-03-09 1:26 ` Aaron Merey 2022-03-18 19:23 ` Tom Tromey 2022-03-22 0:27 ` Aaron Merey 2022-04-07 14:54 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).