Hi, Upon David's request I've joined the in progress patch to the below email. I hope it makes more sense now. Best, Benjamin. ---------- Forwarded message --------- From: Benjamin Priour Date: Tue, Jul 18, 2023 at 3:30 PM Subject: [RFC] analyzer: Add optional trim of the analyzer diagnostics going too deep [PR110543] To: , David Malcolm Hi, I'd like to request comments on a patch I am writing for PR110543. The goal of this patch is to reduce the noise of the analyzer emitted diagnostics when dealing with system headers, or simply diagnostic paths that are too long. The new option only affects the display of the diagnostics, but doesn't hinder the actual analysis. I've defaulted the new option to "system", thus preventing the diagnostic paths from showing system headers. "never" corresponds to the pre-patch behavior, whereas you can also specify an unsigned value that prevents paths to go deeper than frames. fanalyzer-trim-diagnostics= > Common Joined RejectNegative ToLower Var(flag_analyzer_trim_diagnostics) > Init("system") > -fanalyzer-trim-diagnostics=[never|system|] Trim diagnostics > path that are too long before emission. > Does it sounds reasonable and user-friendly ? Regstrapping was a success against trunk, although one of the newly added test case fails for c++14. Note that the test case below was done with "never", thus behaves exactly as the pre-patch analyzer on x86_64-linux-gnu. /* { dg-additional-options "-fdiagnostics-plain-output > -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=never" > } */ > /* { dg-skip-if "" { c++98_only } } */ > > #include > struct A {int x; int y;}; > > int main () { > std::shared_ptr a; > a->x = 4; /* { dg-line deref_a } */ > /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ > > return 0; > } > > /* { dg-begin-multiline-output "" } > 'int main()': events 1-2 > | > | > +--> 'std::__shared_ptr_access<_Tp, _Lp, , > >::element_type* std::__shared_ptr_access<_Tp, _Lp, , > >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy > _Lp = __gnu_cxx::_S_atomic; bool = false; bool = > false]': events 3-4 > | > | > +--> 'std::__shared_ptr_access<_Tp, _Lp, , > >::element_type* std::__shared_ptr_access<_Tp, _Lp, > , >::_M_get() const [with _Tp = A; > __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = > false; bool = false]': events 5-6 > | > | > +--> 'std::__shared_ptr<_Tp, _Lp>::element_type* > std::__shared_ptr<_Tp, _Lp>::get() const [with _Tp = A; > __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]': events 7-8 > | > | > <------+ > | > 'std::__shared_ptr_access<_Tp, _Lp, , > >::element_type* std::__shared_ptr_access<_Tp, _Lp, > , >::_M_get() const [with _Tp = A; > __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = > false; bool = false]': event 9 > | > | > <------+ > | > 'std::__shared_ptr_access<_Tp, _Lp, , > >::element_type* std::__shared_ptr_access<_Tp, _Lp, , > >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy > _Lp = __gnu_cxx::_S_atomic; bool = false; bool = > false]': event 10 > | > | > <------+ > | > 'int main()': events 11-12 > | > | > { dg-end-multiline-output "" } */ > The first events "'int main()': events 1-2" vary in c++14 (get events 1-3). > > // c++14 with fully detailed output > ‘int main()’: events 1-3 > | > | 8 | int main () { > | | ^~~~ > | | | > | | (1) entry to ‘main’ > | 9 | std::shared_ptr a; > | | ~ > | | | > | | (2) > ‘a.std::shared_ptr ::.std::__shared_ptr __gnu_cxx::_S_atomic>::_M_ptr’ is NULL > | 10 | a->x = 4; /* { dg-line deref_a } */ > | | ~~ > | | | > | | (3) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’ > whereas c++17 and posterior give > // c++17 with fully detailed output > // ./xg++ -fanalyzer > ../../gcc/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C > -B. -shared-libgcc -fanalyzer-trim-diagnostics=never -std=c++17 > ‘int main()’: events 1-2 > | > | 8 | int main () { > | | ^~~~ > | | | > | | (1) entry to ‘main’ > | 9 | std::shared_ptr a; > | 10 | a->x = 4; /* { dg-line deref_a } */ > | | ~~ > | | | > | | (2) calling ‘std::__shared_ptr_access __gnu_cxx::_S_atomic, false, false>::operator->’ from ‘main’ > Is there a way to make dg-multiline-output check for a regex ? Or would checking the multiline-output only for c++17 and c++20 be acceptable ? This divergence results from two slightly different IPA: // c++14 -fdump-ipa-analyzer > // std::shared_ptr a; becomes > a.D.29392._M_ptr = 0B; > a.D.29392._M_refcount._M_pi = 0B; > whereas in c++17 > // c++17 -fdump-ipa-analyzer > // std::shared_ptr a; becomes > a = {}; > I know shared_ptr limited support is a coincidence more than a feature, but maybe there is still a way to make the above test case work ? Otherwise, I'd fallback to a supported system function. If you have any that you fancy, do tell me. Thanks, Benjamin. --- gcc/analyzer/analyzer.cc | 2 +- gcc/analyzer/analyzer.h | 1 + gcc/analyzer/analyzer.opt | 4 + gcc/analyzer/diagnostic-manager.cc | 132 ++++++++++++++++++ gcc/analyzer/diagnostic-manager.h | 7 + .../analyzer/fanalyzer-trim-diagnostics-0.C | 19 +++ .../analyzer/fanalyzer-trim-diagnostics-1.C | 28 ++++ .../analyzer/fanalyzer-trim-diagnostics-2.C | 44 ++++++ .../fanalyzer-trim-diagnostics-default.C | 21 +++ .../fanalyzer-trim-diagnostics-never.C | 44 ++++++ .../fanalyzer-trim-diagnostics-system.C | 19 +++ 11 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-0.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-1.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-2.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-default.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C create mode 100644 gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-system.C diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc index 5091fb7a583..b27d8e359db 100644 --- a/gcc/analyzer/analyzer.cc +++ b/gcc/analyzer/analyzer.cc @@ -274,7 +274,7 @@ is_named_call_p (const_tree fndecl, const char *funcname) Compare with cp/typeck.cc: decl_in_std_namespace_p, but this doesn't rely on being the C++ FE (or handle inline namespaces inside of std). */ -static inline bool +bool is_std_function_p (const_tree fndecl) { tree name_decl = DECL_NAME (fndecl); diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 579517c23e6..31597079153 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -386,6 +386,7 @@ extern bool is_special_named_call_p (const gcall *call, const char *funcname, extern bool is_named_call_p (const_tree fndecl, const char *funcname); extern bool is_named_call_p (const_tree fndecl, const char *funcname, const gcall *call, unsigned int num_args); +extern bool is_std_function_p (const_tree fndecl); extern bool is_std_named_call_p (const_tree fndecl, const char *funcname); extern bool is_std_named_call_p (const_tree fndecl, const char *funcname, const gcall *call, unsigned int num_args); diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt index 2760aaa8151..78ff2c07483 100644 --- a/gcc/analyzer/analyzer.opt +++ b/gcc/analyzer/analyzer.opt @@ -362,4 +362,8 @@ fdump-analyzer-untracked Common RejectNegative Var(flag_dump_analyzer_untracked) Emit custom warnings with internal details intended for analyzer developers. +fanalyzer-trim-diagnostics= +Common Joined RejectNegative ToLower Var(flag_analyzer_trim_diagnostics) Init("system") +-fanalyzer-trim-diagnostics=[never|system|] Trim diagnostics path that are too long before emission. + ; This comment is to ensure we retain the blank line above. diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index cfca305d552..d946ad5bac7 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -20,9 +20,11 @@ along with GCC; see the file COPYING3. If not see #include "config.h" #define INCLUDE_MEMORY +#define INCLUDE_VECTOR #include "system.h" #include "coretypes.h" #include "tree.h" +#include "input.h" #include "pretty-print.h" #include "gcc-rich-location.h" #include "gimple-pretty-print.h" @@ -60,6 +62,37 @@ along with GCC; see the file COPYING3. If not see #if ENABLE_ANALYZER +/* Return positive value of fanalyzer-trim-diagnostics if a depth was given + or a negative value otherwise. */ + +long trim_diagnostics_depth () +{ + if (trim_diagnostics_system_p () || trim_diagnostics_never_p ()) + return -1; + + char *end = NULL; + long depth = strtol (flag_analyzer_trim_diagnostics, &end, 10); + gcc_assert (end == NULL || *end == '\0'); + gcc_assert (!(end == flag_analyzer_trim_diagnostics && depth == 0)); + return depth; +} + +/* Return true if fanalyzer-trim-diagnostics is set to "system". */ + +bool trim_diagnostics_system_p () +{ + int len = strlen (flag_analyzer_trim_diagnostics); + return len == 6 && strcmp (flag_analyzer_trim_diagnostics, "system") == 0; +} + +/* Return true if fanalyzer-trim-diagnostics is set to "never". */ + +bool trim_diagnostics_never_p () +{ + int len = strlen (flag_analyzer_trim_diagnostics); + return len == 5 && strcmp (flag_analyzer_trim_diagnostics, "never") == 0; +} + namespace ana { class feasible_worklist; @@ -2281,6 +2314,8 @@ diagnostic_manager::prune_path (checker_path *path, path->maybe_log (get_logger (), "path"); prune_for_sm_diagnostic (path, sm, sval, state); prune_interproc_events (path); + if (! trim_diagnostics_never_p ()) + trim_diagnostic_path (path); consolidate_conditions (path); finish_pruning (path); path->maybe_log (get_logger (), "pruned"); @@ -2667,6 +2702,103 @@ diagnostic_manager::prune_interproc_events (checker_path *path) const while (changed); } +void +diagnostic_manager::trim_diagnostic_path (checker_path *path) const +{ + if (trim_diagnostics_system_p ()) + prune_system_headers (path); + else + prune_events_too_deep (path); +} + +/* Remove everything within ]callsite, IDX]. */ +static void +prune_within_frame (checker_path *path, int &idx) +{ + int nesting = 1; + while (idx >= 0 && nesting != 0) + { + if (path->get_checker_event (idx)->is_call_p ()) + nesting--; + else if (path->get_checker_event (idx)->is_return_p ()) + nesting++; + + path->delete_event (idx--); + } +} + +void +diagnostic_manager::prune_system_headers (checker_path *path) const +{ + int idx = (signed)path->num_events () - 1; + while (idx >= 0) + { + const checker_event *event = path->get_checker_event (idx); + /* Prune everything between [..., system call, (...), system return, ...]. */ + if (event->is_return_p () + && in_system_header_at (event->get_location ())) + { + int ret_idx = idx; + prune_within_frame (path, idx); + + if (get_logger ()) + { + label_text desc + (path->get_checker_event (idx)->get_desc (false)); + log ("filtering event %i-%i:" + " system header event: %s", + idx, ret_idx, desc.get ()); + } + // delete callsite + if (idx >= 0) + path->delete_event (idx); + } + + idx--; + } +} + +void +diagnostic_manager::prune_events_too_deep (checker_path *path) const +{ + long depth = 0; + long maxdepth = trim_diagnostics_depth (); + gcc_assert (maxdepth >= 0); + int idx = (signed)path->num_events () - 1; + if (idx >= 0) + { + depth = path->get_checker_event (0)->get_stack_depth (); + maxdepth += depth; + } + + while (idx >= 0) + { + const checker_event *event = path->get_checker_event (idx); + /* Prune everything between [..., call too deep, (...), return too deep, ...]. */ + if (event->is_return_p () + && event->get_stack_depth () > maxdepth) + { + int ret_idx = idx; + prune_within_frame (path, idx); + + if (get_logger ()) + { + label_text desc + (path->get_checker_event (idx)->get_desc (false)); + log ("filtering event %i-%i:" + " event too deep: %s", + idx, ret_idx, desc.get ()); + } + // delete callsite + if (idx >= 0) + path->delete_event (idx); + } + + idx--; + } + +} + /* Return true iff event IDX within PATH is on the same line as REF_EXP_LOC. */ static bool diff --git a/gcc/analyzer/diagnostic-manager.h b/gcc/analyzer/diagnostic-manager.h index 9b3e903fc5d..9afef0975be 100644 --- a/gcc/analyzer/diagnostic-manager.h +++ b/gcc/analyzer/diagnostic-manager.h @@ -21,6 +21,10 @@ along with GCC; see the file COPYING3. If not see #ifndef GCC_ANALYZER_DIAGNOSTIC_MANAGER_H #define GCC_ANALYZER_DIAGNOSTIC_MANAGER_H +long trim_diagnostics_depth (); +bool trim_diagnostics_system_p (); +bool trim_diagnostics_never_p (); + namespace ana { class epath_finder; @@ -180,6 +184,9 @@ private: state_machine::state_t state) const; void update_for_unsuitable_sm_exprs (tree *expr) const; void prune_interproc_events (checker_path *path) const; + void trim_diagnostic_path (checker_path *path) const; + void prune_system_headers (checker_path *path) const; + void prune_events_too_deep (checker_path *path) const; void consolidate_conditions (checker_path *path) const; void finish_pruning (checker_path *path) const; diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-0.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-0.C new file mode 100644 index 00000000000..ddf39ed1690 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-0.C @@ -0,0 +1,19 @@ +/* { dg-additional-options "-fdiagnostics-plain-output -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=0" } */ +/* { dg-skip-if "" { c++98_only } } */ + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-1.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-1.C new file mode 100644 index 00000000000..83a132078a1 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-1.C @@ -0,0 +1,28 @@ +/* { dg-additional-options "-fdiagnostics-plain-output -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=1" } */ +/* { dg-skip-if "" { c++98_only } } */ + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} + +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + +--> 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': event 3 + | + | + <------+ + | + 'int main()': events 4-5 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-2.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-2.C new file mode 100644 index 00000000000..cf065929ec9 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-2.C @@ -0,0 +1,44 @@ +/* { dg-additional-options "-fdiagnostics-plain-output -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=2" } */ +/* { dg-skip-if "" { c++98_only } } */ + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} + +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + +--> 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': events 3-4 + | + | + +--> 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::_M_get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': events 5-6 + | + | + +--> 'std::__shared_ptr<_Tp, _Lp>::element_type* std::__shared_ptr<_Tp, _Lp>::get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]': events 7-8 + | + | + <------+ + | + 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::_M_get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': event 9 + | + | + <------+ + | + 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': event 10 + | + | + <------+ + | + 'int main()': events 11-12 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-default.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-default.C new file mode 100644 index 00000000000..fc271309d4e --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-default.C @@ -0,0 +1,21 @@ +/* { dg-additional-options "-O0 -fdiagnostics-plain-output -fdiagnostics-path-format=inline-events" } */ +/* { dg-skip-if "" { c++98_only } } */ + +// Must behave like -fanalyzer-trim-diagnostics=system + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C new file mode 100644 index 00000000000..a69416608da --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-never.C @@ -0,0 +1,44 @@ +/* { dg-additional-options "-fdiagnostics-plain-output -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=never" } */ +/* { dg-skip-if "" { c++98_only } } */ + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} + +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + +--> 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': events 3-4 + | + | + +--> 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::_M_get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': events 5-6 + | + | + +--> 'std::__shared_ptr<_Tp, _Lp>::element_type* std::__shared_ptr<_Tp, _Lp>::get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic]': events 7-8 + | + | + <------+ + | + 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::_M_get() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': event 9 + | + | + <------+ + | + 'std::__shared_ptr_access<_Tp, _Lp, , >::element_type* std::__shared_ptr_access<_Tp, _Lp, , >::operator->() const [with _Tp = A; __gnu_cxx::_Lock_policy _Lp = __gnu_cxx::_S_atomic; bool = false; bool = false]': event 10 + | + | + <------+ + | + 'int main()': events 11-12 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file diff --git a/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-system.C b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-system.C new file mode 100644 index 00000000000..22fadc84e75 --- /dev/null +++ b/gcc/testsuite/g++.dg/analyzer/fanalyzer-trim-diagnostics-system.C @@ -0,0 +1,19 @@ +/* { dg-additional-options "-fdiagnostics-plain-output -fdiagnostics-path-format=inline-events -fanalyzer-trim-diagnostics=system" } */ +/* { dg-skip-if "" { c++98_only } } */ + +#include + +struct A {int x; int y;}; + +int main () { + std::shared_ptr a; + a->x = 4; /* { dg-line deref_a } */ + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */ + + return 0; +} +/* { dg-begin-multiline-output "" } + 'int main()': events 1-2 + | + | + { dg-end-multiline-output "" } */ \ No newline at end of file -- 2.34.1