Qing Zhao <qing.z...@oracle.com> writes:
> Control this with a new option -fdiagnostics-details.
>
> $ cat t.c
> extern void warn(void);
> static inline void assign(int val, int *regs, int *index)
> {
> if (*index >= 4)
> warn();
> *regs = val;
> }
> struct nums {int vals[4];};
>
> void sparx5_set (int *ptr, struct nums *sg, int index)
> {
> int *val = &sg->vals[index];
>
> assign(0, ptr, &index);
> assign(*val, ptr, &index);
> }
>
> $ gcc -Wall -O2 -c -o t.o t.c
> t.c: In function ‘sparx5_set’:
> t.c:12:23: warning: array subscript 4 is above array bounds of ‘int[4]’
> [-Warray-bounds=]
> 12 | int *val = &sg->vals[index];
> | ~~~~~~~~^~~~~~~
> t.c:8:18: note: while referencing ‘vals’
> 8 | struct nums {int vals[4];};
> | ^~~~
>
> In the above, Although the warning is correct in theory, the warning message
> itself is confusing to the end-user since there is information that cannot
> be connected to the source code directly.
>
> It will be a nice improvement to add more information in the warning message
> to report where such index value come from.
>
> In order to achieve this, we add a new data structure "move_history" to record
> 1. the "condition" that triggers the code movement;
> 2. whether the code movement is on the true path of the "condition";
> 3. the "compiler transformation" that triggers the code movement.
>
> Whenever there is a code movement along control flow graph due to some
> specific transformations, such as jump threading, path isolation, tree
> sinking, etc., a move_history structure is created and attached to the
> moved gimple statement.
>
> During array out-of-bound checking or -Wstringop-* warning checking, the
> "move_history" that was attached to the gimple statement is used to form
> a sequence of diagnostic events that are added to the corresponding rich
> location to be used to report the warning message.
>
> This behavior is controled by the new option -fdiagnostics-details
> which is off by default.
>
> With this change, by adding -fdiagnostics-details,
> the warning message for the above testing case is now:
>
> $ gcc -Wall -O2 -fdiagnostics-details -c -o t.o t.c
> t.c: In function ‘sparx5_set’:
> t.c:12:23: warning: array subscript 4 is above array bounds of ‘int[4]’
> [-Warray-bounds=]
> 12 | int *val = &sg->vals[index];
> | ~~~~~~~~^~~~~~~
> ‘sparx5_set’: events 1-2
> 4 | if (*index >= 4)
> | ^
> | |
> | (1) when the condition is evaluated to true
> ......
> 12 | int *val = &sg->vals[index];
> | ~~~~~~~~~~~~~~~
> | |
> | (2) out of array bounds here
> t.c:8:18: note: while referencing ‘vals’
> 8 | struct nums {int vals[4];};
> | ^~~~
>
> PR tree-optimization/109071
>
> gcc/ChangeLog:
>
> * Makefile.in (OBJS): Add diagnostic-move-history.o
> and move-history-diagnostic-path.o.
> * gcc/common.opt (fdiagnostics-details): New option.
> * gcc/doc/invoke.texi (fdiagnostics-details): Add
> documentation for the new option.
> * gimple-array-bounds.cc (build_rich_location_with_diagnostic_path):
> New function.
> (check_out_of_bounds_and_warn): Add one new parameter. Use rich
> location with move_history_diagnostic_path for warning_at.
> (array_bounds_checker::check_array_ref): Use rich location with
> move_history_diagnostic_path for warning_at.
> (array_bounds_checker::check_mem_ref): Add one new parameter.
> Use rich location with move_history_diagnostic_path for warning_at.
> (array_bounds_checker::check_addr_expr): Use rich location with
> move_history_diagnostic_path for warning_at.
> (array_bounds_checker::check_array_bounds): Call check_mem_ref with
> one more parameter.
> * gimple-array-bounds.h: Update prototype for check_mem_ref.
> * gimple-iterator.cc (gsi_remove): (gsi_remove): Remove the move
> history when removing the gimple.
> * gimple-pretty-print.cc (pp_gimple_stmt_1): Emit MV_H marking
> if the gimple has a move_history.
> * gimple-ssa-isolate-paths.cc (isolate_path): Set move history
> for the gimples of the duplicated blocks.
> * gimple-ssa-warn-restrict.cc (maybe_diag_access_bounds): Use
> rich location with move_history_diagnostic_path for warning_at.
> * gimple-ssa-warn-access.cc (warn_string_no_nul): Likewise.
> (maybe_warn_nonstring_arg): Likewise.
> (maybe_warn_for_bound): Likewise.
> (warn_for_access): Likewise.
> (check_access): Likewise.
> (pass_waccess::check_strncat): Likewise.
> (pass_waccess::maybe_check_access_sizes): Likewise.
> * tree-ssa-sink.cc (sink_code_in_bb): Create move_history for
> stmt when it is sinked.
> * toplev.cc (toplev::finalize): Call move_history_finalize.
> * tree-ssa-threadupdate.cc (ssa_redirect_edges): Create move_history
> for stmts when they are duplicated.
> (back_jt_path_registry::duplicate_thread_path): Likewise.
> * move-history-diagnostic-path.cc: New file.
> * move-history-diagnostic-path.h: New file.
> * diagnostic-move-history.cc: New file.
> * diagnostic-move-history.h: New file.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/109071
>
> * gcc.dg/pr109071.c: New test.
> * gcc.dg/pr109071_1.c: New test.
> * gcc.dg/pr109071_2.c: New test.
> * gcc.dg/pr109071_3.c: New test.
> * gcc.dg/pr109071_4.c: New test.
> * gcc.dg/pr109071_5.c: New test.
> * gcc.dg/pr109071_6.c: New test.
> ---
> gcc/Makefile.in | 2 +
> gcc/common.opt | 4 +
> gcc/diagnostic-move-history.cc | 264 ++++++++++++++++++++++++++++
> gcc/diagnostic-move-history.h | 92 ++++++++++
> gcc/doc/invoke.texi | 7 +
> gcc/gimple-array-bounds.cc | 75 ++++++--
> gcc/gimple-array-bounds.h | 2 +-
> gcc/gimple-iterator.cc | 3 +
> gcc/gimple-pretty-print.cc | 4 +
> gcc/gimple-ssa-isolate-paths.cc | 11 ++
> gcc/gimple-ssa-warn-access.cc | 153 ++++++++++------
> gcc/gimple-ssa-warn-restrict.cc | 27 +--
> gcc/move-history-diagnostic-path.cc | 119 +++++++++++++
> gcc/move-history-diagnostic-path.h | 96 ++++++++++
> gcc/testsuite/gcc.dg/pr109071.c | 43 +++++
> gcc/testsuite/gcc.dg/pr109071_1.c | 36 ++++
> gcc/testsuite/gcc.dg/pr109071_2.c | 50 ++++++
> gcc/testsuite/gcc.dg/pr109071_3.c | 42 +++++
> gcc/testsuite/gcc.dg/pr109071_4.c | 41 +++++
> gcc/testsuite/gcc.dg/pr109071_5.c | 33 ++++
> gcc/testsuite/gcc.dg/pr109071_6.c | 49 ++++++
> gcc/toplev.cc | 3 +
> gcc/tree-ssa-sink.cc | 10 ++
> gcc/tree-ssa-threadupdate.cc | 25 +++
> 24 files changed, 1105 insertions(+), 86 deletions(-)
> create mode 100644 gcc/diagnostic-move-history.cc
> create mode 100644 gcc/diagnostic-move-history.h
> create mode 100644 gcc/move-history-diagnostic-path.cc
> create mode 100644 gcc/move-history-diagnostic-path.h
> create mode 100644 gcc/testsuite/gcc.dg/pr109071.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_2.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_3.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_4.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_5.c
> create mode 100644 gcc/testsuite/gcc.dg/pr109071_6.c
>
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index 059cf2e8f79f..0d119ba46e1b 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -1432,6 +1432,8 @@ OBJS = \
> df-problems.o \
> df-scan.o \
> dfp.o \
> + diagnostic-move-history.o \
> + move-history-diagnostic-path.o \
> digraph.o \
> dojump.o \
> dominance.o \
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 12b25ff486de..84ebef080143 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1566,6 +1566,10 @@ fdiagnostics-minimum-margin-width=
> Common Joined UInteger Var(diagnostics_minimum_margin_width) Init(6)
> Set minimum width of left margin of source code when showing source.
>
> +fdiagnostics-details
> +Common Var(flag_diagnostics_details)
> +Collect and print more context information for diagnostics.
> +
> fdisable-
> Common Joined RejectNegative Var(common_deferred_options) Defer
> -fdisable-[tree|rtl|ipa]-<pass>=range1+range2 Disable an optimization
> pass.
> diff --git a/gcc/diagnostic-move-history.cc b/gcc/diagnostic-move-history.cc
> new file mode 100644
> index 000000000000..b0e8308dbf6b
> --- /dev/null
> +++ b/gcc/diagnostic-move-history.cc
> @@ -0,0 +1,264 @@
> +/* Functions to handle move history.
> + Copyright (C) 2024-2024 Free Software Foundation, Inc.
Just '2024'.
> + Contributed by Qing Zhao <qing.z...@oracle.com>
> +
> [...]
> diff --git a/gcc/diagnostic-move-history.h b/gcc/diagnostic-move-history.h
> new file mode 100644
> index 000000000000..cac9cb1e2675
> --- /dev/null
> +++ b/gcc/diagnostic-move-history.h
> @@ -0,0 +1,92 @@
> +/* Move history associated with a gimple statement to record its history
> + of movement due to different transformations.
> + The move history will be used to construct events for later diagnostic.
> +
> + Copyright (C) 2024-2024 Free Software Foundation, Inc.
> + Contributed by Qing Zhao <qing.z...@oracle.com>
> +
> + This file is part of GCC.
> +
> + GCC is free software; you can redistribute it and/or modify it under
> + the terms of the GNU General Public License as published by the Free
> + Software Foundation; either version 3, or (at your option) any later
> + version.
> +
> + GCC is distributed in the hope that it will be useful, but WITHOUT ANY
> + WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with GCC; see the file COPYING3. If not see
> + <http://www.gnu.org/licenses/>. */
> +
> +#ifndef DIAGNOSTIC_MOVE_HISTORY_H_INCLUDED
> +#define DIAGNOSTIC_MOVE_HISTORY_H_INCLUDED
I think usually the guards lack _INCLUDED.
> +
> +#include "hash-map.h"
> +#include "line-map.h"
> +
> +/* An enum for the reason why this move is made. Right now, there are
> + three reasons, we can add more if needed. */
> +enum move_reason {
> + COPY_BY_THREAD_JUMP,
> + COPY_BY_ISOLATE_PATH,
> + MOVE_BY_SINK,
> + COPY_BY_MAX
> +};
> +
> +/* This data structure records the information when a statement is
> + moved along control flow graph during different transformations.
> + Such information will be used by the later diagnostic messages
> + to report more contexts of the warnings or errors. */
> +struct move_history {
> + /* The location of the condition statement that triggered the code
> + movement. */
> + location_t condition;
> +
> + /* Whether this move is on the TRUE path of the condition. */
> + bool is_true_path;
> +
> + /* The reason for the code movement. */
> + enum move_reason reason;
> +
> + /* This statement itself might be a previous code movement. */
> + struct move_history *prev_move;
> +};
> +
> +typedef struct move_history *move_history_t;
> +
> +/* Create a new move history. */
> +extern move_history_t create_move_history (location_t, bool,
> + enum move_reason, move_history_t);
> +
> +typedef hash_map<const gimple *, move_history_t> move_history_map_t;
> +
> +/* Get the move history for the gimple STMT, return NULL when there is
> + no associated move history. */
> +extern move_history_t get_move_history (const gimple *);
> +
> +/* Remove the move history for STMT from the move_history_map. */
> +extern void remove_move_history (gimple *);
> +
> +/* Set move history for the gimple STMT. */
> +extern bool set_move_history (gimple *, location_t,
> + bool, enum move_reason);
> +
> +/* Reset all state for diagnostic-move-history.cc so that we can rerun the
> + compiler within the same process. For use by toplev::finalize. */
> +extern void move_history_finalize (void);
> +
> +/* Set move history to the stmt based on the edge ENTRY and whether this stmt
> + will be in the destination of the ENTRY. */
> +extern bool set_move_history_to_stmt (gimple *, edge,
> + bool, enum move_reason);
> +
> +/* Set move history to all the stmts in the basic block based on
> + the entry edge and whether this basic block will be the destination
> + of the entry edge. */
> +extern bool set_move_history_to_stmts_in_bb (basic_block, edge,
> + bool, enum move_reason);
> +
> +#endif // DIAGNOSTIC_MOVE_HISTORY_H_INCLUDED
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index 987b63601520..8bb7568d0e30 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -324,6 +324,7 @@ Objective-C and Objective-C++ Dialects}.
> -fdiagnostics-column-origin=@var{origin}
> -fdiagnostics-escape-format=@r{[}unicode@r{|}bytes@r{]}
>
> -fdiagnostics-text-art-charset=@r{[}none@r{|}ascii@r{|}unicode@r{|}emoji@r{]}}
> +-fdiagnostics-details
>
> @item Warning Options
> @xref{Warning Options,,Options to Request or Suppress Warnings}.
> @@ -5609,6 +5610,12 @@ left margin.
> This option controls the minimum width of the left margin printed by
> @option{-fdiagnostics-show-line-numbers}. It defaults to 6.
>
> +@opindex fdiagnostics-details
> +@item -fdiagnostics-details
> +With this option, the compiler collects more context information for
> +diagnostics and emits them to the users to provide more hints on how
> +the diagnostics come from.
> +
Two comments:
1) I don't know if we should note here that it might be a bit slower as it
requires that collection or if it's implied/obvious?
2) Should we mention a list of warnings we know are wired up to it?
(either here, or in the docs for the warnings themselves)
> @opindex fdiagnostics-parseable-fixits
> @item -fdiagnostics-parseable-fixits
> Emit fix-it hints in a machine-parseable format, suitable for consumption
> [...]
thanks,
sam
