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.133.124]) by sourceware.org (Postfix) with ESMTPS id 6AD603858C66 for ; Tue, 7 Nov 2023 14:10:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6AD603858C66 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 6AD603858C66 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699366206; cv=none; b=VaqtUqCu/22l64Ze9sualg3hYAHi7N6GOgENlER/hAdtXqkCfC4bOEQM723jI7U4QZxxchFPBSXZQRhqFYUcR3wUFlUSErZZmwXXUaTKaZVFfQbxIldtsW7ZDI8btTkaiYgtzkw0RbUmRkoXWmLw3cfU1I/1AkgSoDJRFhELTFE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699366206; c=relaxed/simple; bh=qe8kELQzuFsIXav8NvBRWZESHQjLlOuwSrDiH36cSPc=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=Pu2HH7Z2jTlFqbuysFICety3yuNVxtwCizpVmR5LqhU/JWaE9WhgLx/9a+emVNVek4yZKkCLceCQmoGILEh5UF9bwxRszmYOEMkdUu3XlwpkOypZDw+iUtNFxnesrZKC18O0dgxF26jBhgPmOsrHRqsRu1L0eiRRV5hGciR8lvU= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699366204; 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=zpk7+QumSBrZhIb+sw3pc0VIDURyoUb1wlQtWNwHSow=; b=gYk6PVpe3X1EhVWwrHr50XTCknOQFe0Mf+G3RFl6/kU4T301sHkSLji6En9OtPXEwlteTS z5FfGaH9axBF8J6hxqreOz+hrSzYvsM8p6QSNNDXg8e6YaiAZQxcmDNanJp9c4HKSIoDOZ cM6sbN5T64vGzZXNHByp/RJnLFKiZZE= Received: from mail-oi1-f198.google.com (mail-oi1-f198.google.com [209.85.167.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-618-3ipyNgVrNGKOLzdyq80Pyw-1; Tue, 07 Nov 2023 09:10:02 -0500 X-MC-Unique: 3ipyNgVrNGKOLzdyq80Pyw-1 Received: by mail-oi1-f198.google.com with SMTP id 5614622812f47-3b5665e00b6so8707770b6e.2 for ; Tue, 07 Nov 2023 06:10:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699366202; x=1699971002; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=zpk7+QumSBrZhIb+sw3pc0VIDURyoUb1wlQtWNwHSow=; b=f4LGkezJtGtJsB2PXElrSVyWPlSmIX33vxrayrstLiAxK58RcgVOqEwbzGu+arX203 hLvVD7Wt7HY4DangqnL1tTIfHf5q0Ym5NWqODtpJ2HRZLho1m+0m4eXa4dsSTLeVl5EL DIk5OJ7ToquG8J+N6xaaL7jcgGmwX2fm+kjl9nstoUZFJEjKUmw/VyjtF5tLLVSRXPaX J29KWKv8Qk5Smhf1PNEdbGKOub50PtvRNwKLrY5ZIUBevCpGCig10lNtCFvMFn++Z7K+ I8vpLUYPX3qDdyHVxoUnkchycoul3iXmyq1xZmBE+YeiBo7mhqTDw9CyK5IrhfIBxOsF U1Uw== X-Gm-Message-State: AOJu0YzRfYtma0ClpkwZCfMFsrA854DYLnMnVfcxjegdBRk7IhzLgXh6 fKv4wpM9D9cmGIydsHJndSZCl4nD1RQ/hlBYwHO1BlF+EEF+TYT4GRmvaDdelyZdmVEOlp+9Tnk Msa/fYUJcnZ/W3nA6WQ== X-Received: by 2002:a05:6808:3a05:b0:3b6:a7f2:f1fe with SMTP id gr5-20020a0568083a0500b003b6a7f2f1femr4197656oib.47.1699366202029; Tue, 07 Nov 2023 06:10:02 -0800 (PST) X-Google-Smtp-Source: AGHT+IGzQ3Dy7HPy25xGD4HrIKxMODkBPZbcMhHQwSW2z5sC4zt+wpV79wkI2/BYCn7YkljzhE9QeQ== X-Received: by 2002:a05:6808:3a05:b0:3b6:a7f2:f1fe with SMTP id gr5-20020a0568083a0500b003b6a7f2f1femr4197629oib.47.1699366201709; Tue, 07 Nov 2023 06:10:01 -0800 (PST) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id f12-20020a0cf7cc000000b0065b0554ae78sm4350148qvo.100.2023.11.07.06.10.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Nov 2023 06:10:01 -0800 (PST) Message-ID: <0eca4e65d12aa0caac7a3ee49cd7deb252541abe.camel@redhat.com> Subject: Re: [PATCH] binutils: experimental use of libdiagnostics in gas From: David Malcolm To: =?ISO-8859-1?Q?Cl=E9ment?= Chigot Cc: gcc-patches@gcc.gnu.org, binutils@sourceware.org, Nick Clifton , Simon Sobisch Date: Tue, 07 Nov 2023 09:09:59 -0500 In-Reply-To: References: <20231106222959.2707741-1-dmalcolm@redhat.com> <20231106222959.2707741-4-dmalcolm@redhat.com> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 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=-3.8 required=5.0 tests=BAYES_00,BODY_8BITS,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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, 2023-11-07 at 10:21 +0100, Cl=C3=A9ment Chigot wrote: > Hi David, >=20 > Thanks for that interesting RFC ! I'm fully in favor of such > improvements, having uniformed error messages across gcc, gas and > later ld, would greatly help integration of these tools, let alone > the > SARIF format output. Indeed, I can imagine that ld might eventually want to use this as well. > However, I'm not sure how you're planning to make the transition. But > currently, it looks like libdiagnostics is either enabled and thus > the > new format being produced, either it's not and we do have the legacy > format. I think the transition should be smoother than that, there > are > probably thousands of tests, scripts, whatever out in the wild > expecting this legacy format. Allowing both formats within the same > executable, basically chosen by a flag, would probably ease the > transition. Yes. I'm assuming that consumers of libdiagnostics would have a configure-time test for the availability of libdiagnostics. In the example I gave, it was just a compile-time "choice" (I'm not an expert at autotools, so I hacked all of that up for now)... but if the feature is available, it could be a run-time choice. We've been adding new features to GCC's diagnostic output over the years (adding column numbers, showing macro expansions, quoting source code with underlines, fix-it hints, etc), but each time we've added a flag to turn them off (e.g. -fno-diagnostics-show-line-numbers, -fno- diagnostics-show-labels, etc). As of GCC 11 we have a -fdiagnostics-plain-output which "requests that diagnostic output look as plain as possible, which may be useful when running dejagnu or other utilities that need to parse diagnostics output and prefer that it remain more stable over time." In the implementation patch I made the text sink turn on everything by default here: m_dc.m_source_printing.enabled =3D true; // FIXME m_dc.m_source_printing.colorize_source_p =3D true; // FIXME m_dc.m_source_printing.show_labels_p =3D true; // FIXME m_dc.m_source_printing.show_line_numbers_p =3D true; // FIXME m_dc.m_source_printing.min_margin_width =3D 6; // FIXME and I didn't provide a way of turning things off. So maybe the API needs a way to tweak options of the text sink? Maybe: diagnostic_text_sink_set_source_printing (sink, true); diagnostic_text_sink_set_colorize_source (sink, COLORIZE_IF_AT_TTY); ...etc. Also, I made no particular effort to make the output similar to before, hence e.g. the difference in capitalization "Error: " vs "error: ". Is that capitalization something that you'd want to remain consistent? >=20 > Apart from that, just a few remarks on the implementation details, > see below. [...snip...] > > @@ -101,6 +109,29 @@ had_warnings (void) > > =C2=A0=C2=A0 return warning_count; > > =C2=A0} > >=20 > > +#if USE_LIBDIAGNOSTICS > > +static diagnostic_manager *diag_mgr; >=20 > Would it make sense for an application to have several > "diagnostic_manager" ?=C2=A0 > If no, I'm wondering if this variable shouldn't > be hidden inside libdiagnostics itself, avoiding every calls to have > this diag_mgr argument. Although it might not make sense for binutils-style use-cases to have multiple diagnostic_manager instances (since these are implemented all standalone programs), I think in general it *does* make sense. I've found it's usually a bad idea for a shared library to have global state, since at some point a consumer of the library is a shared library itself, at which point users of the 2nd library see unexpected interactions. Consider the case of a linting tool implemented as a shared library: the tool has no knowledge of where it's going to be embedded: e.g. in an IDE, or as part of some other system. Perhaps the IDE is multithreaded. So I think it's better for the user of the diagnostic API (here the lint tool) to have an explicit "context" pointer that it's sending diagnostics to, rather than having it be implicit inside the library. >=20 > > +#endif > > + > > +void messages_init (void) > > +{ > > +#if USE_LIBDIAGNOSTICS > > +=C2=A0 diag_mgr =3D diagnostic_manager_new (); > > +=C2=A0 diagnostic_manager_add_text_sink (diag_mgr, stderr, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 DIAGNOSTIC_COL= ORIZE_IF_TTY); > > +=C2=A0 diagnostic_manager_add_sarif_sink (diag_mgr, stderr, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > DIAGNOSTIC_SARIF_VERSION_2_1_0); > > +#endif > > +} > > + > > +void messages_end (void) > > +{ > > +#if USE_LIBDIAGNOSTICS > > +=C2=A0 diagnostic_manager_release (diag_mgr); >=20 > IIUC, diagnostic_manager_release must be called to produce any > output. The text sink emits (and flushes) each diagnostic to the FILE * stream after diagnostic_finish is called on it. The sarif sink accumulates a JSON representation in memory, and only writes to its FILE * after the manager is released (since there are aspects of the metadata part of the format that requiring knowing about all diagnostics upfront). > However, nothing prevents the application to exit earlier see > "as_fatal". Thus, this probably need to be called using atexit to > ensure that whatever happens the messages are being outputted. atexit handlers are per-process global state, so I'm thinking that being something the client would register, rather than libdiagnostics doing it automatically. [...snip...] Thanks for the feedback; hope the above sounds reasonable Dave