From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 0F0043858C60 for ; Wed, 9 Mar 2022 01:26:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0F0043858C60 Received: from mail-yw1-f199.google.com (mail-yw1-f199.google.com [209.85.128.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-551-qwmdvuM-Ng6V4jlB9NGGFQ-1; Tue, 08 Mar 2022 20:26:55 -0500 X-MC-Unique: qwmdvuM-Ng6V4jlB9NGGFQ-1 Received: by mail-yw1-f199.google.com with SMTP id 00721157ae682-2d726bd83a2so5090557b3.20 for ; Tue, 08 Mar 2022 17:26:55 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3o0JX64/uqIFYaVREyxwEzpUO/hRMHTJKMWF/Q5eYY4=; b=skUc5RkcfRoj3LMaW+ZJutsRA/97u2fWOy+j9xj5k/O9VjIE0lv2UUTdvAGTplewsC pns6EImVaHF1zPyDpMzEliDBL3TRz9LjzGaetC77dutq7frU86ke4K2NEyDJx+N6OPaP dmJ0AyioNpdP8ZHLXl32ISHgoK9fW2ep2ib/q/+jquv8jplvlGVe0qONIfeJ91e6ZLiJ 31g7V4GPghL4ru1c7js4JU/juzLrelMkr+Cmy+Q1wh2erLCserT6FhLMJhw5Ag0PGlIB 5470qGId1L5dtmD8Z7e9ZsMAfOR3IpysDEf1QilG83vX4D4u14HU2ixOP0gVugWWuVYh EkRw== X-Gm-Message-State: AOAM533s+V16+PXf5lP9/AJrQRuDzkUu9pN3ae6DSSlf8GvGAPUIyGMc GIlgCUsfxb1zRWbCXazN+7gVu7N86wrc+8lE6gTjoW014+v/gQpPU3oM74CjEc54HKdet9ObboH 6o+FHGEXpDPNO73JcQsaSXrJ9CcRk653EwwIx X-Received: by 2002:a81:fc5:0:b0:2db:cdc3:6091 with SMTP id 188-20020a810fc5000000b002dbcdc36091mr15386042ywp.198.1646789214559; Tue, 08 Mar 2022 17:26:54 -0800 (PST) X-Google-Smtp-Source: ABdhPJz7CZoLo1vZbV5ijVF7fvpewMC3nSsA5OcuX1oMOIhY37lA1AvH/x//hXhHp0SDFE6Ng6isjTtnIGPOv2Kd3jI= X-Received: by 2002:a81:fc5:0:b0:2db:cdc3:6091 with SMTP id 188-20020a810fc5000000b002dbcdc36091mr15386026ywp.198.1646789214254; Tue, 08 Mar 2022 17:26:54 -0800 (PST) MIME-Version: 1.0 References: <20220126005817.56356-1-amerey@redhat.com> <20220209022548.343785-1-amerey@redhat.com> <875yotshxd.fsf@tromey.com> In-Reply-To: <875yotshxd.fsf@tromey.com> From: Aaron Merey Date: Tue, 8 Mar 2022 20:26:43 -0500 Message-ID: Subject: Re: [PATCH v3] gdb: Improve debuginfod progress updates To: Tom Tromey Cc: gdb-patches@sourceware.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Mar 2022 01:27:01 -0000 Hi Tom, Thanks for the review. On Fri, Mar 4, 2022 at 1:17 PM Tom Tromey wrote: > > >>>>> "Aaron" == Aaron Merey via Gdb-patches 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