From: "Clément Chigot" <chigot@adacore.com>
To: David Malcolm <dmalcolm@redhat.com>
Cc: gcc-patches@gcc.gnu.org, binutils@sourceware.org,
Nick Clifton <nickc@redhat.com>,
Simon Sobisch <simonsobisch@gnu.org>
Subject: Re: [PATCH] binutils: experimental use of libdiagnostics in gas
Date: Tue, 7 Nov 2023 10:21:58 +0100 [thread overview]
Message-ID: <CAJ307EhzsqE8xL1EfFPU22t_Caqy+SrkCHFi4RbEUxuJ2fiJAg@mail.gmail.com> (raw)
In-Reply-To: <20231106222959.2707741-4-dmalcolm@redhat.com>
Hi David,
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.
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.
Apart from that, just a few remarks on the implementation details, see below.
Thanks,
Clément
On Mon, Nov 6, 2023 at 11:30 PM David Malcolm <dmalcolm@redhat.com> wrote:
>
> Here's a patch for gas in binutils that makes it use libdiagnostics
> (with some nasty hardcoded paths to specific places on my hard drive
> to make it easier to develop the API).
>
> For now this hardcodes adding two sinks: a text sink on stderr, and
> also a SARIF output to stderr (which happens after all regular output).
>
> For example, without this patch:
>
> gas testsuite/gas/all/warn-1.s
>
> emits:
> VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> testsuite/gas/all/warn-1.s: Assembler messages:
> testsuite/gas/all/warn-1.s:3: Warning: a warning message
> testsuite/gas/all/warn-1.s:4: Error: .warning argument must be a string
> testsuite/gas/all/warn-1.s:5: Warning: .warning directive invoked in source file
> testsuite/gas/all/warn-1.s:6: Warning: .warning directive invoked in source file
> testsuite/gas/all/warn-1.s:7: Warning:
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> whereas with this patch:
> LD_LIBRARY_PATH=/home/david/coding-3/gcc-newgit-canvas-2023/build/gcc ./as-new testsuite/gas/all/warn-1.s
> emits:
>
> VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> testsuite/gas/all/warn-1.s:3: warning: a warning message
> 3 | .warning "a warning message" ;# { dg-warning "Warning: a warning message" }
> |
> testsuite/gas/all/warn-1.s:4: error: .warning argument must be a string
> 4 | .warning a warning message ;# { dg-error "Error: .warning argument must be a string" }
> |
> testsuite/gas/all/warn-1.s:5: warning: .warning directive invoked in source file
> 5 | .warning ;# { dg-warning "Warning: .warning directive invoked in source file" }
> |
> testsuite/gas/all/warn-1.s:6: warning: .warning directive invoked in source file
> 6 | .warning ".warning directive invoked in source file" ;# { dg-warning "Warning: .warning directive invoked in source file" }
> |
> testsuite/gas/all/warn-1.s:7: warning:
> 7 | .warning "" ;# { dg-warning "Warning: " }
> |
> {"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "version": "2.1.0", "runs": [{"tool": {"driver": {"rules": []}}, "invocations": [{"executionSuccessful": true, "toolExecutionNotifications": []}], "originalUriBaseIds": {"PWD": {"uri": "file:///home/david/coding-3/binutils-gdb/gas/"}}, "artifacts": [{"location": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "contents": {"text": ";# Test .warning directive.\n;# { dg-do assemble }\n .warning \"a warning message\"\t;# { dg-warning \"Warning: a warning message\" }\n .warning a warning message\t;# { dg-error \"Error: .warning argument must be a string\" }\n .warning\t\t\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n .warning \".warning directive invoked in source file\"\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n .warning \"\"\t\t\t;# { dg-warning \"Warning: \" }\n"}}], "results": [{"ruleId": "warning", "level": "warning", "message": {"text": "a warning message"}, "locations": [{"physicalLocation": {"artifactLocation": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "region": {"startLine": 3, "startColumn": 0, "endColumn": 1}, "contextRegion": {"startLine": 3, "snippet": {"text": " .warning \"a warning message\"\t;# { dg-warning \"Warning: a warning message\" }\n"}}}}], "relatedLocations": [{"physicalLocation": {"artifactLocation": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "region": {"startLine": 4, "startColumn": 0, "endColumn": 1}, "contextRegion": {"startLine": 4, "snippet": {"text": " .warning a warning message\t;# { dg-error \"Error: .warning argument must be a string\" }\n"}}}, "message": {"text": ".warning argument must be a string"}}, {"physicalLocation": {"artifactLocation": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "region": {"startLine": 5, "startColumn": 0, "endColumn": 1}, "contextRegion": {"startLine": 5, "snippet": {"text": " .warning\t\t\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n"}}}, "message": {"text": ".warning directive invoked in source file"}}, {"physicalLocation": {"artifactLocation": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "region": {"startLine": 6, "startColumn": 0, "endColumn": 1}, "contextRegion": {"startLine": 6, "snippet": {"text": " .warning \".warning directive invoked in source file\"\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n"}}}, "message": {"text": ".warning directive invoked in source file"}}, {"physicalLocation": {"artifactLocation": {"uri": "testsuite/gas/all/warn-1.s", "uriBaseId": "PWD"}, "region": {"startLine": 7, "startColumn": 0, "endColumn": 1}, "contextRegion": {"startLine": 7, "snippet": {"text": " .warning \"\"\t\t\t;# { dg-warning \"Warning: \" }\n"}}}, "message": {"text": ""}}]}]}]}
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> which I see:
> - drops the leading "Assembler messages" warning,
> - changes the capitalization of the "Warning" -> "warning" etc
> - quotes the pertinent line in the .s file
>
> All of the locations are just lines; does gas do column numbers at all?
> (or ranges?)
>
> For reference, running the above SARIF through "python -m json.tool" to
> prettyify it gives:
>
> VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> {
> "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
> "version": "2.1.0",
> "runs": [
> {
> "tool": {
> "driver": {
> "rules": []
> }
> },
> "invocations": [
> {
> "executionSuccessful": true,
> "toolExecutionNotifications": []
> }
> ],
> "originalUriBaseIds": {
> "PWD": {
> "uri": "file:///home/david/coding-3/binutils-gdb/gas/"
> }
> },
> "artifacts": [
> {
> "location": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "contents": {
> "text": ";# Test .warning directive.\n;# { dg-do assemble }\n .warning \"a warning message\"\t;# { dg-warning \"Warning: a warning message\" }\n .warning a warning message\t;# { dg-error \"Error: .warning argument must be a string\" }\n .warning\t\t\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n .warning \".warning directive invoked in source file\"\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n .warning \"\"\t\t\t;# { dg-warning \"Warning: \" }\n"
> }
> }
> ],
> "results": [
> {
> "ruleId": "warning",
> "level": "warning",
> "message": {
> "text": "a warning message"
> },
> "locations": [
> {
> "physicalLocation": {
> "artifactLocation": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "region": {
> "startLine": 3,
> "startColumn": 0,
> "endColumn": 1
> },
> "contextRegion": {
> "startLine": 3,
> "snippet": {
> "text": " .warning \"a warning message\"\t;# { dg-warning \"Warning: a warning message\" }\n"
> }
> }
> }
> }
> ],
> "relatedLocations": [
> {
> "physicalLocation": {
> "artifactLocation": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "region": {
> "startLine": 4,
> "startColumn": 0,
> "endColumn": 1
> },
> "contextRegion": {
> "startLine": 4,
> "snippet": {
> "text": " .warning a warning message\t;# { dg-error \"Error: .warning argument must be a string\" }\n"
> }
> }
> },
> "message": {
> "text": ".warning argument must be a string"
> }
> },
> {
> "physicalLocation": {
> "artifactLocation": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "region": {
> "startLine": 5,
> "startColumn": 0,
> "endColumn": 1
> },
> "contextRegion": {
> "startLine": 5,
> "snippet": {
> "text": " .warning\t\t\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n"
> }
> }
> },
> "message": {
> "text": ".warning directive invoked in source file"
> }
> },
> {
> "physicalLocation": {
> "artifactLocation": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "region": {
> "startLine": 6,
> "startColumn": 0,
> "endColumn": 1
> },
> "contextRegion": {
> "startLine": 6,
> "snippet": {
> "text": " .warning \".warning directive invoked in source file\"\t;# { dg-warning \"Warning: .warning directive invoked in source file\" }\n"
> }
> }
> },
> "message": {
> "text": ".warning directive invoked in source file"
> }
> },
> {
> "physicalLocation": {
> "artifactLocation": {
> "uri": "testsuite/gas/all/warn-1.s",
> "uriBaseId": "PWD"
> },
> "region": {
> "startLine": 7,
> "startColumn": 0,
> "endColumn": 1
> },
> "contextRegion": {
> "startLine": 7,
> "snippet": {
> "text": " .warning \"\"\t\t\t;# { dg-warning \"Warning: \" }\n"
> }
> }
> },
> "message": {
> "text": ""
> }
> }
> ]
> }
> ]
> }
> ]
> }
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Thoughts?
>
> gas/ChangeLog:
> * Makefile.am (GASLIBS): Add nasty hack with hardcoded path
> on my hard drive.
> * Makefile.in (GASLIBS): Likewise.
> * as.c (gas_init): Call messages_init.
> (main): Call messages_end.
> * as.h (messages_init): New decl.
> (messages_end): New decl.
> * messages.c (USE_LIBDIAGNOSTICS): New define.
> Add #include with nasty hardcoded path.
> (diag_mgr): New.
> (messages_init): New.
> (messages_end): New.
> (as_warn_internal): Add support for libdiagnostics.
> (as_bad_internal): Likewise.
> ---
> gas/Makefile.am | 3 ++-
> gas/Makefile.in | 4 ++-
> gas/as.c | 3 +++
> gas/as.h | 3 +++
> gas/messages.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 79 insertions(+), 2 deletions(-)
>
> diff --git a/gas/Makefile.am b/gas/Makefile.am
> index 0e98ca3ec85..fe92997129c 100644
> --- a/gas/Makefile.am
> +++ b/gas/Makefile.am
> @@ -408,7 +408,8 @@ AM_CPPFLAGS = -I. -I$(srcdir) -I../bfd -I$(srcdir)/config \
> # How to link with both our special library facilities
> # and the system's installed libraries.
>
> -GASLIBS = @OPCODES_LIB@ ../bfd/libbfd.la ../libiberty/libiberty.a
> +# FIXME:
> +GASLIBS = @OPCODES_LIB@ ../bfd/libbfd.la ../libiberty/libiberty.a /home/david/coding-3/gcc-newgit-canvas-2023/build/gcc/libdiagnostics.so
>
> # Files to be copied away after each stage in building.
> STAGESTUFF = *.@OBJEXT@ $(noinst_PROGRAMS)
> diff --git a/gas/Makefile.in b/gas/Makefile.in
> index fae3a47c144..2161d68f9c7 100644
> --- a/gas/Makefile.in
> +++ b/gas/Makefile.in
> @@ -885,7 +885,9 @@ AM_CPPFLAGS = -I. -I$(srcdir) -I../bfd -I$(srcdir)/config \
>
> # How to link with both our special library facilities
> # and the system's installed libraries.
> -GASLIBS = @OPCODES_LIB@ ../bfd/libbfd.la ../libiberty/libiberty.a
> +
> +# FIXME:
> +GASLIBS = @OPCODES_LIB@ ../bfd/libbfd.la ../libiberty/libiberty.a /home/david/coding-3/gcc-newgit-canvas-2023/build/gcc/libdiagnostics.so
>
> # Files to be copied away after each stage in building.
> STAGESTUFF = *.@OBJEXT@ $(noinst_PROGRAMS)
> diff --git a/gas/as.c b/gas/as.c
> index 6839c841588..2a8ce3734a0 100644
> --- a/gas/as.c
> +++ b/gas/as.c
> @@ -1300,6 +1300,7 @@ gas_early_init (int *argcp, char ***argvp)
> static void
> gas_init (void)
> {
> + messages_init ();
> symbol_begin ();
> frag_init ();
> subsegs_begin ();
> @@ -1486,6 +1487,8 @@ main (int argc, char ** argv)
>
> input_scrub_end ();
>
> + messages_end ();
> +
> /* Use xexit instead of return, because under VMS environments they
> may not place the same interpretation on the value given. */
> if (had_errors () != 0)
> diff --git a/gas/as.h b/gas/as.h
> index 99ffe77afd2..9d878a87df5 100644
> --- a/gas/as.h
> +++ b/gas/as.h
> @@ -474,6 +474,9 @@ void as_bad_value_out_of_range (const char *, offsetT, offsetT, offsetT,
> void print_version_id (void);
> char * app_push (void);
>
> +void messages_init (void);
> +void messages_end (void);
> +
> /* Number of littlenums required to hold an extended precision number. */
> #define MAX_LITTLENUMS 6
>
> diff --git a/gas/messages.c b/gas/messages.c
> index 7c018acf69f..3cb8525fad9 100644
> --- a/gas/messages.c
> +++ b/gas/messages.c
> @@ -21,6 +21,14 @@
> #include <limits.h>
> #include <signal.h>
>
> +// FIXME: do some configury thing for this
> +#define USE_LIBDIAGNOSTICS 1
> +
> +#if USE_LIBDIAGNOSTICS
> +// FIXME: need to fix this path, obviously
> +#include "/home/david/coding-3/gcc-newgit-canvas-2023/src/gcc/libdiagnostics.h"
> +#endif
> +
> /* If the system doesn't provide strsignal, we get it defined in
> libiberty but no declaration is supplied. Because, reasons. */
> #if !defined (HAVE_STRSIGNAL) && !defined (strsignal)
> @@ -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.
> +#endif
> +
> +void messages_init (void)
> +{
> +#if USE_LIBDIAGNOSTICS
> + diag_mgr = 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.
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.
> + diag_mgr = NULL;
> +#endif
> +}
> +
> /* Nonzero if we've hit a 'bad error', and should not write an obj file,
> and exit with a nonzero error code. */
>
> @@ -172,16 +203,34 @@ as_tsktsk (const char *format, ...)
> static void
> as_warn_internal (const char *file, unsigned int line, char *buffer)
> {
> +#if !USE_LIBDIAGNOSTICS
> bool context = false;
> +#endif
>
> ++warning_count;
>
> if (file == NULL)
> {
> file = as_where_top (&line);
> +#if !USE_LIBDIAGNOSTICS
> context = true;
> +#endif
> }
>
> +#if USE_LIBDIAGNOSTICS
> + const diagnostic_file *file_obj
> + = diagnostic_manager_new_file (diag_mgr, file, NULL);
> +
> + diagnostic_location_t loc
> + = diagnostic_manager_new_location_from_file_and_line (diag_mgr,
> + file_obj,
> + line);
> +
> + diagnostic *d = diagnostic_begin (diag_mgr,
> + DIAGNOSTIC_LEVEL_WARNING);
> + diagnostic_set_location (d, loc);
> + diagnostic_finish (d, "%s", buffer);
> +#else
> identify (file);
> if (file)
> {
> @@ -199,6 +248,7 @@ as_warn_internal (const char *file, unsigned int line, char *buffer)
> #ifndef NO_LISTING
> listing_warning (buffer);
> #endif
> +#endif /* #else clause of #if USE_LIBDIAGNOSTICS */
> }
>
> /* Send to stderr a string as a warning, and locate warning
> @@ -246,16 +296,33 @@ as_warn_where (const char *file, unsigned int line, const char *format, ...)
> static void
> as_bad_internal (const char *file, unsigned int line, char *buffer)
> {
> +#if !USE_LIBDIAGNOSTICS
> bool context = false;
> +#endif
>
> ++error_count;
>
> if (file == NULL)
> {
> file = as_where_top (&line);
> +#if !USE_LIBDIAGNOSTICS
> context = true;
> +#endif
> }
>
> +#if USE_LIBDIAGNOSTICS
> + const diagnostic_file *file_obj
> + = diagnostic_manager_new_file (diag_mgr, file, NULL);
> + diagnostic_location_t loc
> + = diagnostic_manager_new_location_from_file_and_line (diag_mgr,
> + file_obj,
> + line);
> +
> + diagnostic *d = diagnostic_begin (diag_mgr,
> + DIAGNOSTIC_LEVEL_ERROR);
> + diagnostic_set_location (d, loc);
> + diagnostic_finish (d, "%s", buffer);
> +#else
> identify (file);
> if (file)
> {
> @@ -269,6 +336,7 @@ as_bad_internal (const char *file, unsigned int line, char *buffer)
>
> if (context)
> as_report_context ();
> +#endif /* #else clause of #if USE_LIBDIAGNOSTICS */
>
> #ifndef NO_LISTING
> listing_error (buffer);
> --
> 2.26.3
>
next prev parent reply other threads:[~2023-11-07 9:22 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-06 22:29 [PATCH/RFC] libdiagnostics: a shared library for emitting diagnostics David Malcolm
2023-11-06 22:29 ` [PATCH 1/2] libdiagnostics: header and examples David Malcolm
2023-11-06 22:29 ` [PATCH 2/2] libdiagnostics: work-in-progress implementation David Malcolm
2023-11-07 7:54 ` Simon Sobisch
2023-11-07 14:59 ` David Malcolm
2023-11-07 15:35 ` Simon Sobisch
2023-11-06 22:29 ` [PATCH] binutils: experimental use of libdiagnostics in gas David Malcolm
2023-11-07 7:04 ` Simon Sobisch
2023-11-07 14:51 ` David Malcolm
2023-11-07 9:21 ` Clément Chigot [this message]
2023-11-07 14:09 ` David Malcolm
2023-11-07 15:57 ` Clément Chigot
2023-11-07 16:18 ` David Malcolm
2023-11-07 10:03 ` Jan Beulich
2023-11-07 14:32 ` David Malcolm
2023-11-07 14:59 ` Jan Beulich
2023-11-21 22:20 ` [PATCH 0/6] v2 of libdiagnostics David Malcolm
2023-11-21 22:20 ` [PATCH 1/5] libdiagnostics v2: header and examples David Malcolm
2023-11-21 22:20 ` [PATCH 2/5] libdiagnostics v2: work-in-progress implementation David Malcolm
2023-11-21 22:20 ` [PATCH 3/5] libdiagnostics v2: add C++ wrapper API David Malcolm
2023-11-21 22:20 ` [PATCH 4/5] diagnostics: add diagnostic_context::get_location_text David Malcolm
2023-11-28 1:25 ` David Malcolm
2023-11-21 22:20 ` [PATCH 5/5] diagnostics: don't print annotation lines when there's no column info David Malcolm
2023-11-28 1:25 ` David Malcolm
2023-11-21 22:20 ` [PATCH] binutils: v2: experimental use of libdiagnostics in gas David Malcolm
2023-11-22 7:36 ` Jan Beulich
2023-11-21 22:35 ` [PATCH 0/6] v2 of libdiagnostics Simon Sobisch
2023-11-23 17:36 ` Pedro Alves
2024-01-27 23:28 ` Simon Sobisch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJ307EhzsqE8xL1EfFPU22t_Caqy+SrkCHFi4RbEUxuJ2fiJAg@mail.gmail.com \
--to=chigot@adacore.com \
--cc=binutils@sourceware.org \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nickc@redhat.com \
--cc=simonsobisch@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).