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 [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 80F003858025 for ; Fri, 5 Mar 2021 14:18:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 80F003858025 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-121-BGQfDyjeM0CwV6seQIPJ8Q-1; Fri, 05 Mar 2021 09:18:42 -0500 X-MC-Unique: BGQfDyjeM0CwV6seQIPJ8Q-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D5D49801814; Fri, 5 Mar 2021 14:18:41 +0000 (UTC) Received: from t14s.localdomain (ovpn-114-98.phx2.redhat.com [10.3.114.98]) by smtp.corp.redhat.com (Postfix) with ESMTP id 699ED694C9; Fri, 5 Mar 2021 14:18:41 +0000 (UTC) Message-ID: <113d395803c4ec0923707c451947d4bac45aaa19.camel@redhat.com> Subject: Re: [PATCH] Extract a common logger from jit and analyzer frameworks From: David Malcolm To: Philip Herron , gcc-patches@gcc.gnu.org Date: Fri, 05 Mar 2021 09:18:40 -0500 In-Reply-To: <68dd7ea2-18fd-9931-f2b4-0b0d412b307d@embecosm.com> References: <20210304161755.243332-1-philip.herron@embecosm.com> <3f0f8467e7f2594e8eee30fae7d50f63f8e1d8d8.camel@redhat.com> <68dd7ea2-18fd-9931-f2b4-0b0d412b307d@embecosm.com> User-Agent: Evolution 3.38.3 (3.38.3-1.fc33) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.1 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 05 Mar 2021 14:18:47 -0000 On Fri, 2021-03-05 at 10:58 +0000, Philip Herron wrote: > On 04/03/2021 16:45, David Malcolm wrote: > > On Thu, 2021-03-04 at 16:17 +0000, Philip Herron wrote: > > > In development of the Rust FE logging is useful in debugging. > > > Instead > > > of > > > rolling our own logger it became clear the loggers part of > > > analyzer > > > and jit > > > projects could be extracted and made generic so they could be > > > reused > > > in > > > other projects. [...] > > [...] > > > > > @@ -144,19 +152,27 @@ logger::log_partial (const char *fmt, ...) > > >  void > > >  logger::log_va_partial (const char *fmt, va_list *ap) > > >  { > > > -  text_info text; > > > -  text.format_spec = fmt; > > > -  text.args_ptr = ap; > > > -  text.err_no = 0; > > > -  pp_format (m_pp, &text); > > > -  pp_output_formatted_text (m_pp); > > I think there's an issue here: what format codes are accepted by > > this > > function? > > > > > +  if (!has_pretty_printer ()) > > > +    vfprintf (m_f_out, fmt, *ap); > > ...here we're formatting using vfprintf... > > > > > +  else > > > +    { > > > +      text_info text; > > > +      text.format_spec = fmt; > > > +      text.args_ptr = ap; > > > +      text.err_no = 0; > > > +      pp_format (m_pp, &text); > > ...whereas here we're formatting using pp_format, which has > > different > > conventions. > > > > The jit implementation decls have e.g.: > >    GNU_PRINTF(2, 3); > > whereas the analyzer decls have e.g.: > >    ATTRIBUTE_GCC_DIAG(2, 3); > > > > I'm not quite sure how to square this circle - templates?  Gah. > > > > I guess the analyzer implementation drifted away from the jit > > implementation (the analyzer code was originally copied from the > > jit > > code).  Sorry about that. > > > > [...] > > > > Hope this is constructive > > Dave > > Hi David, > > Thanks for the great feedback and for reviewing my earlier attempts. > It > seems as though there are really two distinct loggers one with a > pretty > printer and one without. Maybe there is a case for creating > BaseLogger > which is using GNU_PRINTF and vfprintf etc. Then a > PrettyPrinterLogger > based on the BaseLogger but allows for the extensions and the pretty > printer I think could solve this. Nit: we don't use CamelCase in GCC [1] > Though at that point maybe it is > getting too complex.  Agreed, that would be too complicated. > For gccrs I am interested in the logger in the > analyzer since it can give such detailed output on the trees FWIW, I'm not quite sure what you mean by the above. > which could > be useful so maybe just extracting that is the right thing to do for > GCC > projects. > > What do you think? I don't think the jit logger exposes the formatted printing API to end- users, so at some point it ought to be possible to port the jit logger from vfprintf to pretty_printer. How's this for a plan? - for the Rust FE you extract just the analyzer logging, and at some later point I can port the jit logging over to the shared gccrs/analyzer logging (so that the Rust FE project doesn't need to depend on the jit porting being done). Obviously it would be better to have just one hierarchical logging framework rather than two, but at least this way we don't have three, and it gives us a path to getting down to a single one, and hopefully is sufficiently non-controversial to not make it harder to (eventually) get the Rust FE merged into mainline gcc. Thoughts? Hope this sounds sane Dave [1] except in template declarations, for template arguments