From: Simon Sobisch <simonsobisch@gnu.org>
To: David Malcolm <dmalcolm@redhat.com>,
gcc-patches@gcc.gnu.org, binutils@sourceware.org
Cc: Nick Clifton <nickc@redhat.com>
Subject: Re: [PATCH] binutils: experimental use of libdiagnostics in gas
Date: Tue, 7 Nov 2023 08:04:44 +0100 [thread overview]
Message-ID: <db8fa6bb-d382-4a3c-ae24-9aa5654ca3b8@gnu.org> (raw)
In-Reply-To: <20231106222959.2707741-4-dmalcolm@redhat.com>
Thank you very much for this proof-of-concept use!
Inspecting it raises the following questions to me, both for a possible
binutils implementation and for the library use in general:
* How should the application set the relevant context (often lines are
shown before/after)?
* Should it be possible to override msgid used to display the
warning/error type?
If this would be possible then the text sink in messages_init may be
adjusted to replace the label with _("Warning") and _("Error"), which
would leave the text output "as-is" (if the text sink is configured to
not output the source line); this would make it usable without
adjusting the testsuite and to adopt to a standard later.
Notes for the SARIF output:
* the region contains an error, according to the linked JSON spec
startColumn has a minimum of 1 (I guess you'd just leave it away if
the application did not set it)
* the application should have the option to pre-set the sourceLanguage
for the diagnostic_manager (maybe even make that a positional argument
that needs to be passed but can be NULL) and override it when
specifying a region
Thanks,
Simon
Am 06.11.2023 um 23:29 schrieb David Malcolm:
> 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;
> +#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);
> + 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);
next prev parent reply other threads:[~2023-11-07 7:04 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 [this message]
2023-11-07 14:51 ` David Malcolm
2023-11-07 9:21 ` Clément Chigot
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=db8fa6bb-d382-4a3c-ae24-9aa5654ca3b8@gnu.org \
--to=simonsobisch@gnu.org \
--cc=binutils@sourceware.org \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nickc@redhat.com \
/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).