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);

Reply via email to