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 2961B385B52D for ; Sat, 28 Oct 2023 00:25:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2961B385B52D Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 2961B385B52D Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698452755; cv=none; b=giznRR1Z43uBQt08WoZn6WMVTWMtWizhPQdFAX9y9XZs0w1JovaftDB5sCM9vJDHVljsCKBSiOqgwv+kAL+8eePKzWsvPIK1e3sa/f3aNYD6iyChvGXW1JMUpTLYHjPOLCy4hIK+DzvwfwMed5+SofOGYqZYiyjUbLTju8Mrh5o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698452755; c=relaxed/simple; bh=yqd3tnD3aQtPuS4RNd3VmnNOpXXlaElGu+G9CiJFhtc=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=u+x5xZ6IYfQGhvSSgHv2TK6m9VooGlZpvQCjfoSkrXJpkDbT1AVxhepZ7pM+vNHslu8h1StfsHPics4u66FRhU4bJJhpmD4+B/j4m7a++CIZGegbz/r6G6Uir4yabqWg++Jw5kBzby6WsYfSRZiJ9WEhZ/SCW7y67HatxScNa1s= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1698452752; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=9HrROfDJdFgrfFKPDB1utA9q89qc9qLxi2Y1sodIMZc=; b=E5fVdf8IW8tAWiocCr6udsE4Sbsbz5/lx6qxv7FFd8ciB58SfTG5Qx9AeuffCgTZc0t81q gzy++0bIWe+kl0P7Db3HFR1cRlait82eBI/rnSI2rChjl0DsI4VwEwlL1Bw0wHoJAT8G/i yKzT5J6wLNrheopAbmQPvWZjTq5LNng= Received: from mail-pj1-f71.google.com (mail-pj1-f71.google.com [209.85.216.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-125-GhbOsD1bM0CcLVtyBLeBVw-1; Fri, 27 Oct 2023 20:25:51 -0400 X-MC-Unique: GhbOsD1bM0CcLVtyBLeBVw-1 Received: by mail-pj1-f71.google.com with SMTP id 98e67ed59e1d1-28016806be2so433207a91.1 for ; Fri, 27 Oct 2023 17:25:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698452750; x=1699057550; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=9HrROfDJdFgrfFKPDB1utA9q89qc9qLxi2Y1sodIMZc=; b=eYxkZC2iO3qf3puf+fOlc4Sce64IYHXfiklx65dkRk4TypWSj55nKpMqjJFpJaQNdJ WzcnnTzYUWpk66TWCqfMNo6b6NhiqF6k9teAAH4IIkJS2kGb23fmSAWB9wTAY15IP6C3 wsy8Hyg+7lvotEbicUQv7y3dO3vkrXKB7NLsY2czQW+B1kf3dzaGIjcgj4i/beapziP0 L2EtZxyeK7RrqdZ3vAH2Nuuguk4c4qzzO03KSkJHx/SDgtXBK1j6rWyRCW+7G9Xb2qzw FBJZI0oS49oPR7nu8+iKBtwizBBAO3pAJSbppRqvBy4zZN709EfBfn8QeVmb7L0tjG7F 3Tfw== X-Gm-Message-State: AOJu0YxOU/YGtvgN3j2+I35nTYpEERYb2dyQ6x5OGyX6x0adIgXMDdml a7cnDVC7IdSZzvMqGLsPnN390MiwQRiQowoZZveD2OCQ4X5Ym9Ombri4+jBSKhXMUHg1MPSnpvL N00ypY2ObZIecc9GQU2ITQM2z5dXPnnICcGSqf7q3t3wSPMY= X-Received: by 2002:a17:90b:4c05:b0:277:4be4:7a84 with SMTP id na5-20020a17090b4c0500b002774be47a84mr8862199pjb.19.1698452750255; Fri, 27 Oct 2023 17:25:50 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH1WJyDdZiVTK8X3lfrinOcXPCK6QVxW1cmw2ZRkSzqN3BVRkw7f2VFffAx+k2D4q1s3dEsjUNdVZjCECwL0sE= X-Received: by 2002:a17:90b:4c05:b0:277:4be4:7a84 with SMTP id na5-20020a17090b4c0500b002774be47a84mr8862183pjb.19.1698452749843; Fri, 27 Oct 2023 17:25:49 -0700 (PDT) MIME-Version: 1.0 References: <20230816044259.2675531-5-amerey@redhat.com> <20231010215521.2726542-1-amerey@redhat.com> <87pm142rjj.fsf@redhat.com> In-Reply-To: <87pm142rjj.fsf@redhat.com> From: Aaron Merey Date: Fri, 27 Oct 2023 20:25:38 -0400 Message-ID: Subject: Re: [PATCH v6] gdb: Buffer output streams during events that might download debuginfo To: Andrew Burgess Cc: gdb-patches@sourceware.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Oct 24, 2023 at 7:23=E2=80=AFAM Andrew Burgess wrote: > > Unless I'm missing something, none of this cur_prefix_state stuff can > trigger after this commit; the cur_prefix_state starts as > prefix_state_t::NEWLINE_OFF and can never change from that state. > > I know this changes after later patches in this series, but it feels > like this work has ended up in this patch by mistake. > > Could we remove these changes from this commit and push them into a > later commit in this series? Moved to patch 4/4. > > +void > > +buffer_group::output_unit::flush () const > > +{ > > + m_stream->puts (m_msg.c_str ()); > > Maybe we should do: > > if (!m_msg.empty ()) > m_stream->puts (m_msg.c_str ()); > > I guess it doesn't make much difference, but when we queue up a > wrap_here call we add an empty string. Done. > > +buffered_streams::buffered_streams (buffer_group *group, ui_out *uiout= ) > > + : m_buffered_stdout (group, gdb_stdout), > > + m_buffered_stderr (group, gdb_stderr), > > + m_buffered_stdlog (group, gdb_stdlog), > > + m_buffered_stdtarg (group, gdb_stdtarg), > > + m_buffered_stdtargerr (group, gdb_stdtargerr), > > + m_uiout (uiout) > > + { > > + gdb_stdout =3D &m_buffered_stdout; > > + gdb_stderr =3D &m_buffered_stderr; > > + gdb_stdlog =3D &m_buffered_stdlog; > > + gdb_stdtarg =3D &m_buffered_stdtarg; > > + gdb_stdtargerr =3D &m_buffered_stdtargerr; > > + > > + ui_file *stream =3D current_uiout->current_stream (); > > + if (stream !=3D nullptr) > > + { > > + m_buffered_current_uiout.emplace (group, stream); > > + current_uiout->redirect (&(*m_buffered_current_uiout)); > > + } > > When I was thinking about this problem, I imagined replacing this > current_uiout code with the code you've added below. My thinking is > that M_UIOUT is what we are about to pass to the printing routine, > right? So if we are actually using CURRENT_UIOUT within the printing > routine, then something is going horribly wrong. > > Caching CURRENT_UIOUT feels like a work around for possibly broken code > elsewhere in GDB. > > Have you tried this patch without the CURRENT_UIOUT buffering? Only > buffer M_UIOUT? Hopefully that works fine, but if it throws up some > errors, maybe we should look into those and see if we can fix them. It does not work without buffering both CURRENT_UIOUT and M_UIOUT but I don't think this is an indication that something has gone wrong. Before this patch, some of the functions we now apply output buffering to (via do_with_buffered_output) would set CURRENT_UIOUT to a local variable and print to that CURRENT_UIOUT alias for their duration. Other buffered functions take a uiout argument that may or may not be the CURRENT_UIOUT. I think the current approach of buffering both CURRENT_UIOUT and M_UIOUT is a simple way to cover all of these cases without having to tweak very much within the buffered functions. > > > + > > + stream =3D m_uiout->current_stream (); > > + if (stream !=3D nullptr && current_uiout !=3D m_uiout) > > + { > > + m_buffered_uiout.emplace (group, stream); > > + m_uiout->redirect (&(*m_buffered_uiout)); > > + } > > + > > + m_buffers_in_place =3D true; > > + }; > > + > > +/* See ui-out.h. */ > > + > > +void > > +buffered_streams::remove_buffers () > > + { > > + if (!m_buffers_in_place) > > + return; > > + > > + m_buffers_in_place =3D false; > > + > > + gdb_stdout =3D m_buffered_stdout.stream (); > > + gdb_stderr =3D m_buffered_stderr.stream (); > > + gdb_stdlog =3D m_buffered_stdlog.stream (); > > + gdb_stdtarg =3D m_buffered_stdtarg.stream (); > > + gdb_stdtargerr =3D m_buffered_stdtargerr.stream (); > > + > > + if (m_buffered_current_uiout.has_value ()) > > + current_uiout->redirect (nullptr); > > + > > + if (m_buffered_uiout.has_value ()) > > + m_uiout->redirect (nullptr); > > + } > > + > > +buffer_group::buffer_group (ui_out *uiout) > > + : m_buffered_streams (new buffered_streams (this, uiout)) > > +{ /* Nothing. */ } > > + > > +buffer_group::~buffer_group () > > +{ /* Nothing. */ } > > + > > +/* See ui-out.h. */ > > + > > +void > > +buffer_group::flush () const > > +{ > > + m_buffered_streams->remove_buffers (); > > + > > + for (const output_unit &ou : m_buffered_output) > > + ou.flush (); > > +} > > diff --git a/gdb/ui-out.h b/gdb/ui-out.h > > index 07567a1df35..45d8007587b 100644 > > --- a/gdb/ui-out.h > > +++ b/gdb/ui-out.h > > @@ -278,6 +278,9 @@ class ui_out > > escapes. */ > > virtual bool can_emit_style_escape () const =3D 0; > > > > + /* Return the ui_file currently used for output. */ > > + virtual ui_file *current_stream () const =3D 0; > > + > > /* An object that starts and finishes displaying progress updates. = */ > > class progress_update > > { > > @@ -293,6 +296,21 @@ class ui_out > > BAR > > }; > > > > + /* Used to communicate the status of a newline prefix for the next= progress > > + update message. */ > > + enum prefix_state > > + { > > + /* Do not modify the next progress update message. */ > > + NEWLINE_OFF, > > + > > + /* The next progress update message should include a newline pre= fix. */ > > + NEWLINE_NEEDED, > > + > > + /* A newline prefix was included in a debuginfod progress update > > + message. */ > > + NEWLINE_PRINTED > > + }; > > + > > /* SHOULD_PRINT indicates whether something should be printed for = a tty. */ > > progress_update () > > { > > @@ -390,6 +408,11 @@ class ui_out > > ui_out_level *current_level () const; > > }; > > > > +typedef ui_out::progress_update::prefix_state prefix_state_t; > > + > > +/* Current state of the newline prefix. */ > > +extern prefix_state_t cur_prefix_state; > > + > > /* Start a new tuple or list on construction, and end it on > > destruction. Normally this is used via the typedefs > > ui_out_emit_tuple and ui_out_emit_list. */ > > @@ -470,4 +493,181 @@ class ui_out_redirect_pop > > struct ui_out *m_uiout; > > }; > > > > +struct buffered_streams; > > + > > +/* Organizes writes to a collection of buffered output streams > > + so that when flushed, output is written to all streams in > > + chronological order. */ > > + > > +struct buffer_group > > +{ > > + buffer_group (ui_out *uiout); > > + > > + ~buffer_group (); > > + > > + /* Flush all buffered writes to the underlying output streams. */ > > + void flush () const; > > + > > + /* Record contents of BUF and associate it with STREAM. */ > > + void write (const char *buf, long length_buf, ui_file *stream); > > + > > + /* Record a wrap_here and associate it with STREAM. */ > > + void wrap_here (int indent, ui_file *stream); > > + > > +private: > > + > > + struct output_unit > > + { > > + output_unit (std::string msg, int wrap_hint =3D -1) > > + : m_msg (msg), m_wrap_hint (wrap_hint) > > + {} > > + > > + /* Write contents of this output_unit to the underlying stream. *= / > > + void flush () const; > > + > > + /* Underlying stream for which this output unit will be written to= . */ > > + ui_file *m_stream; > > + > > + /* String to be written to underlying buffer. */ > > + std::string m_msg; > > + > > + /* Argument to wrap_here. -1 indicates no wrap. Used to call wra= p_here > > + at appropriate times during flush. */ > > + int m_wrap_hint; > > + }; > > + > > + /* Output_units to be written to buffered output streams. */ > > + std::vector m_buffered_output; > > + > > + /* Buffered output streams. */ > > + std::unique_ptr m_buffered_streams; > > +}; > > + > > +/* If FILE is a buffering_file, return it's underlying stream. */ > > + > > +extern ui_file *get_unbuffered (ui_file *file); > > + > > +/* Buffer output to gdb_stdout and gdb_stderr for the duration of FUNC= . */ > > + > > +template > > +void > > +do_with_buffered_output (F func, ui_out *uiout, Arg... args) > > +{ > > + buffer_group g (uiout); > > + > > + try > > + { > > + func (uiout, std::forward (args)...); > > + } > > + catch (gdb_exception &ex) > > + { > > + /* Ideally flush would be called in the destructor of buffer_gro= up, > > + however flushing might cause an exception to be thrown. Catch i= t > > + and ensure the first exception propagates. */ > > + try > > + { > > + g.flush (); > > + } > > + catch (const gdb_exception &) > > + { > > + } > > + > > + throw_exception (std::move (ex)); > > + } > > + > > + /* Try was successful. Let any further exceptions propagate. */ > > + g.flush (); > > +} > > + > > +/* Accumulate writes to an underlying ui_file. Output to the > > + underlying file is deferred until required. */ > > + > > +struct buffering_file : public ui_file > > +{ > > + buffering_file (buffer_group *group, ui_file *stream) > > + : m_group (group), > > + m_stream (stream) > > + { /* Nothing. */ } > > + > > + /* Return the underlying output stream. */ > > + ui_file *stream () const > > + { > > + return m_stream; > > + } > > + > > + /* Record the contents of BUF. */ > > + void write (const char *buf, long length_buf) override > > + { > > + m_group->write (buf, length_buf, m_stream); > > + } > > + > > + /* Record a wrap_here call with argument INDENT. */ > > + void wrap_here (int indent) override > > + { > > + m_group->wrap_here (indent, m_stream); > > + } > > + > > + /* Return true if the underlying stream is a tty. */ > > + bool isatty () override > > + { > > + return m_stream->isatty (); > > + } > > + > > + /* Return true if ANSI escapes can be used on the underlying stream.= */ > > + bool can_emit_style_escape () override > > + { > > + return m_stream->can_emit_style_escape (); > > + } > > + > > + /* Flush the underlying output stream. */ > > + void flush () override > > + { > > + return m_stream->flush (); > > + } > > This doesn't seem right. Some caller is going to perform a series of > write, wrap_here, and flush calls, and we should be queuing them up, > then replaying them later, right? > > We queue up the write and wrap_here calls, but the flush calls we send > through immediately. > > I'm not sure we actually need to add the flush as a separate output_unit > in the buffer_group though, maybe all that's needed is a single bool > flag per-stream somewhere? Then in buffer_group::flush, we can somehow > arrange to call flush on those streams that have a queued flush pending? Fixed. I ended up recording flush with a separate output_unit because this functionality fits well within the design of output_unit. Plus it ensures that the flush occurs exactly at the intended points in the output. The remaining patches in this series have been reposted here with these changes: https://sourceware.org/pipermail/gdb-patches/2023-October/203589.html Aaron