binutils: experimental use of libdiagnostics in gas
Checks
Commit Message
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(-)
Comments
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
>
On 06.11.2023 23:29, David Malcolm 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?)
It currently doesn't, which is primarily related to the scrubbing done
before lines are actually processed.
I take it that the lack of column information is why there are lines of
this form
|
in the example output above. Them uniformly not carrying any information
would make it desirable for them to be suppressed.
> @@ -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
I can't spot how this context information would be replaced. It works
for macros only right now, but the hope is to eventually extend it
also to .include files.
> @@ -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 */
This listing integration of course needs to remain irrespective of
which way of emitting diagnostics is used.
Jan
On Tue, 2023-11-07 at 10:21 +0100, Clément Chigot wrote:
> 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.
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 = true; // FIXME
m_dc.m_source_printing.colorize_source_p = true; // FIXME
m_dc.m_source_printing.show_labels_p = true; // FIXME
m_dc.m_source_printing.show_line_numbers_p = true; // FIXME
m_dc.m_source_printing.min_margin_width = 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?
>
> 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.
>
> > +#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.
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
On Tue, 2023-11-07 at 11:03 +0100, Jan Beulich wrote:
> On 06.11.2023 23:29, David Malcolm 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:
> > VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> > VVVVVVVVV
> > 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:
> >
> > VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
> > VVVVVVVVV
> > 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: "
> > }
> > |
[...snip...]
> > 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?)
>
> It currently doesn't, which is primarily related to the scrubbing
> done
> before lines are actually processed.
How complicated/desirable would it be to track locations in .s files at
the column level? I confess I didn't look at the parsing code at all.
>
> I take it that the lack of column information is why there are lines
> of
> this form
>
> |
>
> in the example output above.
Yes: those lines are for annotation information such as underlining
specific columns.
> Them uniformly not carrying any information
> would make it desirable for them to be suppressed.
In GCC we typically have column information, so I'd never noticed this
behavior, but it ought to be fixable, to simply not display these if
there's no column info.
>
> > @@ -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
>
> I can't spot how this context information would be replaced. It works
> for macros only right now, but the hope is to eventually extend it
> also to .include files.
I confess I hacked this up, and I didn't check what this code does.
I see now that it calls as_report_context, which iterates over macro
expansions calling as_info_where with successively larger "indent"
values.
I could extend the patch to cover that.
More ambitiously, GCC's location tracking supports recording macro
expansions and include files, and the diagnostics subsystem has a way
of printing this information. So potentially libdiagnostics could
provide API support for this - but I haven't yet looked at the
feasibility.
>
> > @@ -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 */
>
> This listing integration of course needs to remain irrespective of
> which way of emitting diagnostics is used.
Likewise; I think I just put the #endif in the wrong place above.
Thanks for the feedback; hope this is constructive.
Dave
On Tue, 2023-11-07 at 08:04 +0100, Simon Sobisch wrote:
> 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)?
Currently the source-printing code has this logic (in gcc/diagnostic-
show-locus.cc):
- filter locations within the diagnostic to "sufficiently sane" ones
(thus ignoring e.g. ranges that end before they start)
- look at all of the remaining locations that are in the same source
file as the primary location of the diagnostic
- determine a set of runs of source lines; layout::calculate_line_spans
has this comment:
/* We want to print the pertinent source code at a diagnostic. The
rich_location can contain multiple locations. This will have been
filtered into m_exploc (the caret for the primary location) and
m_layout_ranges, for those ranges within the same source file.
We will print a subset of the lines within the source file in question,
as a collection of "spans" of lines.
This function populates m_line_spans with an ordered, disjoint list of
the line spans of interest.
Printing a gap between line spans takes one line, so, when printing
line numbers, we allow a gap of up to one line between spans when
merging, since it makes more sense to print the source line rather than a
"gap-in-line-numbering" line. When not printing line numbers, it's
better to be more explicit about what's going on, so keeping them as
separate spans is preferred.
For example, if the primary range is on lines 8-10, with secondary ranges
covering lines 5-6 and lines 13-15:
004
005 |RANGE 1
006 |RANGE 1
007
008 |PRIMARY RANGE
009 |PRIMARY CARET
010 |PRIMARY RANGE
011
012
013 |RANGE 2
014 |RANGE 2
015 |RANGE 2
016
With line numbering on, we want two spans: lines 5-10 and lines 13-15.
With line numbering off (with span headers), we want three spans: lines 5-6,
lines 8-10, and lines 13-15. */
(end of quote)
This algorithm could be tweaked if you want extra lines of context,
perhaps having an integer option N for N extra lines of before/after
context around each run.
> * 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.
For the msgids, I was more thinking of the messages of the diagnostics
themselves; for instance, in GCC the error format string:
"Invalid argument %d for builtin %qF"
has fr.po translation:
"Argument %d invalide pour la fonction interne %qF"
But it sounds like gas also has capitalized severities (e.g.
"Warning"), so maybe that should simply be a flag in the text sink.
>
>
> 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)
Good catch; looks like a bug in my SARIF output code. I've filed it
for myself as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112425
> * 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
Why?
Note that the sourceLanguage can always be NULL. I considered setting
it for gas, but it's not clear what the value can be, so I just omit
it; see:
https://github.com/oasis-tcs/sarif-spec/issues/608
[..snip...]
Thanks for the feedback
Dave
On 07.11.2023 15:32, David Malcolm wrote:
> On Tue, 2023-11-07 at 11:03 +0100, Jan Beulich wrote:
>> On 06.11.2023 23:29, David Malcolm wrote:
>>> All of the locations are just lines; does gas do column numbers at
>>> all?
>>> (or ranges?)
>>
>> It currently doesn't, which is primarily related to the scrubbing
>> done
>> before lines are actually processed.
>
> How complicated/desirable would it be to track locations in .s files at
> the column level? I confess I didn't look at the parsing code at all.
At the parsing level tracking may be feasible, but as said the scrubber
(zapping in particular redundant whitespace, but also doing other
"interesting" things) is the problem point here, imo.
Jan
> > 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 = true; // FIXME
> m_dc.m_source_printing.colorize_source_p = true; // FIXME
> m_dc.m_source_printing.show_labels_p = true; // FIXME
> m_dc.m_source_printing.show_line_numbers_p = true; // FIXME
> m_dc.m_source_printing.min_margin_width = 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 = 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ément
On Tue, 2023-11-07 at 16:57 +0100, Clément Chigot wrote:
> > > 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...
One other consideration here may be bootstrapping a toolchain (e.g.
bringing up a new CPU architecture) and thus minimizing dependencies.
binutils is such an important component that perhaps you'd want to
retain a minimal implementation of diagnostics that doesn't bring in an
external requirement? especially one based on GCC 14 (which itself
requires GCC 4.8 or later to build), e.g. configuring "--without-
libdiagnostics" or somesuch?
> A configure-time choice can be more easily
> enabled by default in a few versions and then removed completely
> after
> another bunch of versions.
>
>
[...snip...]
Dave
@@ -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)
@@ -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)
@@ -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)
@@ -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
@@ -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);