Richi/Jakub: PR diagnostics/110522 isn't a regression per-se, but this
is a high-value bug to fix.
Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
Successful run of analyzer integration tests on x86_64-pc-linux-gnu,
albeit showing a big increase in the number of results (see notes
below).
OK for trunk?
Thanks
Dave
PR diagnostics/110522 notes that when generating the output path for
diagnostics in SARIF or HTML form, the compiler ignores the path for the
compiler output, leading to race conditions and overwritten SARIF output
when compiling the same source file multiple times in a build.
Given e.g.
gcc \
-o build-dir/foo.o \
-fdiagnostics-add-output=sarif
foo.c
my intent was for the sarif sink to write to "build-dir/foo.c.sarif",
but in GCC 13 - GCC 15 it writes to "foo.c.sarif" instead.
The driver adds the option "-dumpdir build-dir" when invoking cc1, and
opts.cc's finish_options has the logic to prepend the dump_dir_name to
the dump_base_name, which in theory ought to have affected the name for
SARIF output. The root cause is that finish_options is called *after*
processing the options for creating the diagnostic sinks, and so the
sarif_sink is created when dump_base_name is "foo.c", and thus
writes to "foo.c.sarif", rather than "build-dir/foo.c.sarif".
This patch introduces a new helper function for accessing the base name
for diagnostic sinks and refactors the prepending logic
in finish_options so that it is also used when creating diagnostic
sinks, so that in the above example the SARIF is written to
"build-dir/foo.c.sarif".
The patch took the number of .sarif files generated by the analyzer
integration tests from 5094 to 11309. The most extreme example I could
find within them was where we previously got a single .sarif file:
qemu-7.2.0/build/fpu_softfloat.c.c.sarif
and *with* the patch we get 66 .sarif files:
qemu-7.2.0/build/libqemu-aarch64-linux-user.fa.p/fpu_softfloat.c.c.sarif
qemu-7.2.0/build/libqemu-aarch64-softmmu.fa.p/fpu_softfloat.c.c.sarif
[...snip lots of different architectures...]
qemu-7.2.0/build/libqemu-xtensaeb-linux-user.fa.p/fpu_softfloat.c.c.sarif
qemu-7.2.0/build/libqemu-xtensaeb-softmmu.fa.p/fpu_softfloat.c.c.sarif
which presumably were previously all being written to the same place
during the build. Indeed, the old file is invalid JSON, and appears to
have been truncated.
Given that some build systems may be relying on the existing buggy
behavior, I attempted to implement a compatibility option to restore the
old behavior, but this ran into further issues with the ordering in
which options are processed. Hence the patch also introduces a new
environment variable GCC_DIAGNOSTICS_IGNORE_DUMPDIR which when set
restores the old behavior.
gcc/ChangeLog:
PR diagnostics/110522
* doc/invoke.texi (GCC_DIAGNOSTICS_IGNORE_DUMPDIR): New
environment variable.
* gcc.cc (driver_handle_option): Use
get_diagnostic_file_output_basename for
OPT_fdiagnostics_format_.
* opts-diagnostic.cc (get_base_filename): Likewise.
(get_diagnostic_file_output_basename): New.
* opts-diagnostic.h (get_diagnostic_file_output_basename): New
decl.
* opts.cc (maybe_prepend_dump_dir_name): New, based on code in
finish_options.
(finish_options): Move code for determining prepended
dump_base_name to maybe_prepend_dump_dir_name and call it.
(common_handle_option): Use get_diagnostic_file_output_basename
for OPT_fdiagnostics_format_.
* opts.h (maybe_prepend_dump_dir_name): New decl.
Signed-off-by: David Malcolm <[email protected]>
---
gcc/doc/invoke.texi | 30 ++++++++++++++++++++++---
gcc/gcc.cc | 3 +--
gcc/opts-diagnostic.cc | 29 +++++++++++++++++++++---
gcc/opts-diagnostic.h | 4 ++++
gcc/opts.cc | 50 +++++++++++++++++++++++++++++-------------
gcc/opts.h | 3 +++
6 files changed, 96 insertions(+), 23 deletions(-)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index cad9e993140b..1e4b4a89b04c 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -6288,7 +6288,8 @@ The default is @samp{text}.
@cindex diagnostic output formats, sarif-file
The @samp{sarif-stderr} and @samp{sarif-file} formats both emit
diagnostics in SARIF Version 2.1.0 format, either to stderr, or to a file
-named @file{@var{source}.sarif}, respectively.
+named @file{@var{source}.sarif}, respectively. See also
+@env{GCC_DIAGNOSTICS_IGNORE_DUMPDIR}.
@opindex fdiagnostics-add-output
@item -fdiagnostics-add-output=@var{DIAGNOSTICS-OUTPUT-SPEC}
@@ -6354,7 +6355,7 @@ Supported keys for the @code{sarif} scheme are:
@item file=@var{FILENAME}
Specify the filename to write the SARIF output to, potentially with a
leading absolute or relative path. If not specified, it defaults to
-@file{@var{source}.sarif}.
+@file{@var{source}.sarif} (see also @env{GCC_DIAGNOSTICS_IGNORE_DUMPDIR}).
@item serialization=@r{[}json@r{]}
Specify the serialization format to use when writing out the SARIF.
@@ -6397,7 +6398,7 @@ Add an embedded <style> to the generated HTML. Defaults
to yes.
@item file=@var{FILENAME}
Specify the filename to write the HTML output to, potentially with a
leading absolute or relative path. If not specified, it defaults to
-@file{@var{source}.html}.
+@file{@var{source}.html} (see also @env{GCC_DIAGNOSTICS_IGNORE_DUMPDIR}).
@item javascript=@r{[}yes@r{|}no@r{]}
Add an embedded <script> to the generated HTML providing a barebones UI
@@ -39315,6 +39316,29 @@ attempt to open that file and write the information
there.
The precise content and format of the information is subject to change;
it is intended for use by GCC developers, rather than end-users.
+@vindex GCC_DIAGNOSTICS_IGNORE_DUMPDIR
+@item GCC_DIAGNOSTICS_IGNORE_DUMPDIR
+When outputing files for diagnostics (such as with
+@option{-fdiagnostics-add-output=sarif} or
+@option{-fdiagnostics-format=sarif-file}), the compiler will by default
+select an output path that respects the dump directory, either when
+set implicitly by the driver to match that of the output file, or
+provided explicitly via @option{-dumpdir}.
+
+For example, with
+@smallexample
+gcc \
+ -o build-dir/foo.o \
+ -fdiagnostics-add-output=sarif
+ foo.c
+@end smallexample
+the compiler will write the SARIF to @file{build-dir/foo.c.sarif}.
+
+However in GCC 13, 14, and 15, SARIF output did not respect the dump
+directory, and instead wrote the SARIF to @file{foo.c.sarif}. Setting
+@env{GCC_DIAGNOSTICS_IGNORE_DUMPDIR} restores the old behavior, for
+compatibility with build systems that expect this.
+
@vindex GCC_COMPARE_DEBUG
@item GCC_COMPARE_DEBUG
Setting @env{GCC_COMPARE_DEBUG} is nearly equivalent to passing
diff --git a/gcc/gcc.cc b/gcc/gcc.cc
index 6b6f6f87c520..2fc743af3d90 100644
--- a/gcc/gcc.cc
+++ b/gcc/gcc.cc
@@ -4387,8 +4387,7 @@ driver_handle_option (struct gcc_options *opts,
case OPT_fdiagnostics_format_:
{
- const char *basename = (opts->x_dump_base_name ?
opts->x_dump_base_name
- : opts->x_main_input_basename);
+ const char *basename = get_diagnostic_file_output_basename (*opts);
gcc_assert (dc);
diagnostics::output_format_init (*dc,
opts->x_main_input_filename,
basename,
diff --git a/gcc/opts-diagnostic.cc b/gcc/opts-diagnostic.cc
index f60628244489..6694cf081c7f 100644
--- a/gcc/opts-diagnostic.cc
+++ b/gcc/opts-diagnostic.cc
@@ -94,9 +94,7 @@ public:
const char *
get_base_filename () const final override
{
- return (m_opts.x_dump_base_name
- ? m_opts.x_dump_base_name
- : m_opts.x_main_input_basename);
+ return get_diagnostic_file_output_basename (m_opts);
}
const gcc_options &m_opts;
@@ -170,3 +168,28 @@ handle_OPT_fdiagnostics_set_output_ (const gcc_options
&opts,
if (auto sink = try_to_make_sink (opts, dc, option_name, unparsed_spec, loc))
dc.set_sink (std::move (sink));
}
+
+/* Return the base name to use when choosing names for output file for
+ diagnostic sinks (e.g. BASENAME.sarif or BASENAME.html). */
+
+const char *
+get_diagnostic_file_output_basename (const gcc_options &opts)
+{
+ /* This might have been called before finish_options, which prepends
+ the dump dir to the dump base name. If so, make a prepended copy
+ now and use it. */
+ if (opts.x_dump_base_name
+ && ! opts.x_dump_base_name_prefixed
+ /* In GCC <= 15 we failed to prepend the dumpdir.
+ Support the old behavior if GCC_DIAGNOSTICS_IGNORE_DUMPDIR is set
+ in the environment. */
+ && !getenv ("GCC_DIAGNOSTICS_IGNORE_DUMPDIR"))
+ if (const char *prepended_dump_base_name
+ = maybe_prepend_dump_dir_name (opts))
+ /* Allocated in opts_obstack. */
+ return prepended_dump_base_name;
+
+ return (opts.x_dump_base_name
+ ? opts.x_dump_base_name
+ : opts.x_main_input_basename);
+}
diff --git a/gcc/opts-diagnostic.h b/gcc/opts-diagnostic.h
index f496ef04912c..d0a8beddd81a 100644
--- a/gcc/opts-diagnostic.h
+++ b/gcc/opts-diagnostic.h
@@ -86,4 +86,8 @@ handle_OPT_fdiagnostics_set_output_ (const gcc_options &opts,
diagnostics::context &dc,
const char *arg,
location_t loc);
+
+extern const char *
+get_diagnostic_file_output_basename (const gcc_options &opts);
+
#endif
diff --git a/gcc/opts.cc b/gcc/opts.cc
index 6323fd588016..6658b6acd378 100644
--- a/gcc/opts.cc
+++ b/gcc/opts.cc
@@ -1070,6 +1070,37 @@ validate_ipa_reorder_locality_lto_partition (struct
gcc_options *opts,
validated_p = true;
}
+/* If OPTS.x_dump_base_name doesn't contain any directory separators
+ and has not had OPTS.x_dump_dir_name prepended to it, generate
+ a new string in opts_obstack that has the dump_dir_name prepended to
+ the dump_base_name. */
+
+const char *
+maybe_prepend_dump_dir_name (const gcc_options &opts)
+{
+ const char *sep = opts.x_dump_base_name;
+
+ for (; *sep; sep++)
+ if (IS_DIR_SEPARATOR (*sep))
+ break;
+
+ if (*sep)
+ {
+ /* If dump_base_name contains subdirectories, don't prepend
+ anything. */
+ return nullptr;
+ }
+
+ if (opts.x_dump_dir_name)
+ {
+ /* We have a DUMP_DIR_NAME, prepend that. */
+ return opts_concat (opts.x_dump_dir_name,
+ opts.x_dump_base_name, NULL);
+ }
+
+ return nullptr;
+}
+
/* After all options at LOC have been read into OPTS and OPTS_SET,
finalize settings of those options and diagnose incompatible
combinations. */
@@ -1080,19 +1111,9 @@ finish_options (struct gcc_options *opts, struct
gcc_options *opts_set,
if (opts->x_dump_base_name
&& ! opts->x_dump_base_name_prefixed)
{
- const char *sep = opts->x_dump_base_name;
-
- for (; *sep; sep++)
- if (IS_DIR_SEPARATOR (*sep))
- break;
-
- if (*sep)
- /* If dump_base_path contains subdirectories, don't prepend
- anything. */;
- else if (opts->x_dump_dir_name)
- /* We have a DUMP_DIR_NAME, prepend that. */
- opts->x_dump_base_name = opts_concat (opts->x_dump_dir_name,
- opts->x_dump_base_name, NULL);
+ if (const char *prepended_dump_base_name
+ = maybe_prepend_dump_dir_name (*opts))
+ opts->x_dump_base_name = prepended_dump_base_name;
/* It is definitely prefixed now. */
opts->x_dump_base_name_prefixed = true;
@@ -3023,8 +3044,7 @@ common_handle_option (struct gcc_options *opts,
case OPT_fdiagnostics_format_:
{
- const char *basename = (opts->x_dump_base_name ?
opts->x_dump_base_name
- : opts->x_main_input_basename);
+ const char *basename = get_diagnostic_file_output_basename (*opts);
gcc_assert (dc);
diagnostics::output_format_init (*dc,
opts->x_main_input_filename,
basename,
diff --git a/gcc/opts.h b/gcc/opts.h
index a59475b1daf8..2d28620edab5 100644
--- a/gcc/opts.h
+++ b/gcc/opts.h
@@ -577,4 +577,7 @@ struct switchstr
extern label_text
get_option_url_suffix (int option_index, unsigned lang_mask);
+extern const char *
+maybe_prepend_dump_dir_name (const gcc_options &opts);
+
#endif
--
2.26.3
