From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1034.google.com (mail-pj1-x1034.google.com [IPv6:2607:f8b0:4864:20::1034]) by sourceware.org (Postfix) with ESMTPS id A391B385B534 for ; Tue, 7 Nov 2023 15:57:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A391B385B534 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org A391B385B534 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::1034 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699372644; cv=none; b=EyQ7hyPYMhMzUiB1cbbtNo2OTe217iq0LJVVV0xiN2Eh/V594FAtNpHeEJZebFqmRVl2mw04H81hW3y/lPOTQdwCKcUGABDjWb8fvmYPEyX9MHC7810d5vnsIy0Vj7LoLeFzylmTnzf0OzNjKKBe5ucDLBeN8ro+uZGUDoQhvJY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699372644; c=relaxed/simple; bh=lJgIx1ipVMtnvgZgM7eJvot2d5xQ0XziEZBD802bH2I=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=h/aFTuuVQ4vOAO5RVi5wB4oESOgQ6wER2vGdyW3s12Qg9AP2PTnk3KbynQxpkn4sx1AtAU0TZ6VjPXSR4yjfWbktoyoE5XKXrG9HlV9+j5va3+I1vbe+mf1vK/iYecEm/7abED5KXVrCl/MaLIm4gjaVcIa3XqgPPmdHHbhLrBg= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pj1-x1034.google.com with SMTP id 98e67ed59e1d1-280260db156so5363620a91.2 for ; Tue, 07 Nov 2023 07:57:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=adacore.com; s=google; t=1699372640; x=1699977440; darn=sourceware.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=EoLhTGM+opWbJfL0eiqPMvY9LUeCsxbI9K0MPDYFDDk=; b=fN/LdbSHTJDVnpl6EU8TWFfs8RmGsJdxLfSU9gKghebseJGD+JcQhX8EAEP+KGBtXn zN2mglomj72cO+M0GbsiXbDWfn0po6VoOm6XB4mmg+58lJwu+QgBTwoHpqnejGX2cVSe 7Cybq5VhJqiIHCen4eOKkRn3IfekZL2kCidfQYhIF/aOMgtaBo9bNZxfHPvi5++qsiHg j11dMO4ejE8LTw4pA2MbK/VZdcvj15ZUVyGLB/ZoP9+lCKvNEzNlucs0rygrFOBKqTuh sDB/iLopben2IzfQ8SXp+hFpv43HhIi5OIc8AXU4XnCQ7VYKpH9t+MCfuTKwuj85ZJOB If1A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699372640; x=1699977440; 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=EoLhTGM+opWbJfL0eiqPMvY9LUeCsxbI9K0MPDYFDDk=; b=LN+FOndoqUcLgeaUfzuZNUgUtwUMqcY15Uu/VaE4D3FQ3ogkBM7TUNOIS5uc/CD7Cd TVmmwvNdd35RuuCjDHg8TlzRDbvcPmYG+iqz0MT2mjY+BhYbfZ44mWH6gPDhTx5GTvWE 9U7Wg/1qSCQLftDKQ+6/sSirNd3vK1GxFyRywNYFIJZ4Nc5Yv4F7CyMr0IFT7TFL1V/M GvCF0kmt4Bz4G77LNuiy53clYIOZwf0BjegkoeW063ZdH+nYXeic9jH+ItE+pMOetc3E /Mna/wKTIELw/jI4Oduit4w1VE2ZW/Fi5z/NjYCpDsB+PPRX83yvs6pCSXRwI22XcGIL jq2A== X-Gm-Message-State: AOJu0YyyFhjf+7Qhzj8ialnQiw190rLhsZcARDvGnHvyzJuL61isf1yo mXkdvsyeDf00rOY0drMtR1xH3xrNAPF2YHyxHu1nDw== X-Google-Smtp-Source: AGHT+IE5NW0Ru1aJIjgw6nE/6dG//ZbQzYRL/XDBm5IwgP6MGy5p+0HTc722+9k+m3SnHBgX5+n4EUaEmbc8oRBpyto= X-Received: by 2002:a17:90a:208:b0:25e:a8ab:9157 with SMTP id c8-20020a17090a020800b0025ea8ab9157mr27435775pjc.22.1699372640484; Tue, 07 Nov 2023 07:57:20 -0800 (PST) MIME-Version: 1.0 References: <20231106222959.2707741-1-dmalcolm@redhat.com> <20231106222959.2707741-4-dmalcolm@redhat.com> <0eca4e65d12aa0caac7a3ee49cd7deb252541abe.camel@redhat.com> In-Reply-To: <0eca4e65d12aa0caac7a3ee49cd7deb252541abe.camel@redhat.com> From: =?UTF-8?Q?Cl=C3=A9ment_Chigot?= Date: Tue, 7 Nov 2023 16:57:07 +0100 Message-ID: Subject: Re: [PATCH] binutils: experimental use of libdiagnostics in gas To: David Malcolm Cc: gcc-patches@gcc.gnu.org, binutils@sourceware.org, Nick Clifton , Simon Sobisch Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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: > > 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." I guess starting by a configure-time choice before such flags like -fdiagnostics-plain-output are implemented is enough. I merely wish that there is a way to preserve the old output, giving people the time to experiment and then transitioning all their tools. We can also introduce a flag like "-fdiagnostics-legacy-output`. Though, I'm fearing it will never be removed, meaning maintaining the previous code forever... A configure-time choice can be more easily enabled by default in a few versions and then removed completely after another bunch of versions. > 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? If we keep a way to produce the old output, I don't think so. And it would probably be better to be coherent across gcc, gas, etc. > > > > Apart from that, just a few remarks on the implementation details, > > see below. > > [...snip...] > > > > @@ -101,6 +109,29 @@ had_warnings (void) > > > return warning_count; > > > } > > > > > > +#if USE_LIBDIAGNOSTICS > > > +static diagnostic_manager *diag_mgr; > > > > Would it make sense for an application to have several > > "diagnostic_manager" ? > > 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. Thanks for the explanation. I'm fine with that now. > > > > > +#endif > > > + > > > +void messages_init (void) > > > +{ > > > +#if USE_LIBDIAGNOSTICS > > > + diag_mgr =3D diagnostic_manager_new (); > > > + diagnostic_manager_add_text_sink (diag_mgr, stderr, > > > + DIAGNOSTIC_COLORIZE_IF_TTY); > > > + diagnostic_manager_add_sarif_sink (diag_mgr, stderr, > > > + > > > DIAGNOSTIC_SARIF_VERSION_2_1_0); > > > +#endif > > > +} > > > + > > > +void messages_end (void) > > > +{ > > > +#if USE_LIBDIAGNOSTICS > > > + diagnostic_manager_release (diag_mgr); > > > > 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. Arf, missed that sorry. > 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. Yes, we would probably need something like that here. Otherwise, we would miss some sink outputs if they can be completed only at the end, making them usable. > [...snip...] > > Thanks for the feedback; hope the above sounds reasonable > Dave It does ! Thanks, Cl=C3=A9ment