> On Oct 30, 2024, at 13:48, Sam James <s...@gentoo.org> wrote:
>
> 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.
Just checked GCC’s source code, yes, only “diagnostic-spec.h”,
“simple-predicate-analysis.h” use _INCLUDED to guard.
Other header files do not add _INCLUDED.
I can remove it in the next version.
>
>> +
>> +#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?
I think that adding such notes should not hurt anything.
>
> 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)
Yes, this is reasonable to clarify on both sides.
I will update this.
Thanks for the comments and suggestions.
Qing
>
>> @opindex fdiagnostics-parseable-fixits
>> @item -fdiagnostics-parseable-fixits
>> Emit fix-it hints in a machine-parseable format, suitable for consumption
>> [...]
>
> thanks,
> sam
