On Fri, 2023-08-11 at 13:51 +0200, priour...@gmail.com wrote:
> From: benjamin priour <vultk...@gcc.gnu.org>
Hi Benjamin, thanks for the patch.
Overall, the patch is close to being ready, but see the various
comments inline below...
>
> This patch introduces -fanalyzer-show-events-in-system-headers,
> disabled by default.
>
> This option reduce the noise of the analyzer emitted diagnostics
> when dealing with system headers.
> The new option only affects the display of the diagnostics,
> but doesn't hinder the actual analysis.
>
> Given a diagnostics path diving into a system header in the form
> [
> prefix events...,
> system header call,
> system header entry,
> events within system headers...,
> system header return,
> suffix events...
> ]
> then disabling the option (either by default or explicitly)
> will shorten the path into:
> [
> prefix events...,
> system header call,
> system header return,
> suffix events...
> ]
>
> Signed-off-by: benjamin priour <priour...@gmail.com>
>
[...]
>
> 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);
The analyzer.{cc|h} parts of the patch make is_std_function_p "extern",
but I didn't see any use of it in the rest of the patch. Did I miss
something, or are the changes to is_std_function_p a vestige from an
earlier version of the patch?
[...]
> diff --git a/gcc/analyzer/analyzer.opt b/gcc/analyzer/analyzer.opt
> index 2760aaa8151..d97cd569f52 100644
> --- a/gcc/analyzer/analyzer.opt
> +++ b/gcc/analyzer/analyzer.opt
> @@ -290,6 +290,10 @@ fanalyzer-transitivity
> Common Var(flag_analyzer_transitivity) Init(0)
> Enable transitivity of constraints during analysis.
>
> +fanalyzer-show-events-in-system-headers
> +Common Var(flag_analyzer_show_events_in_system_headers) Init(0)
> +Trim diagnostics path that are too long before emission.
> +
There's a mismatch here between the sense of the name of the option as
opposed to the sense of the description, and the wording isn't quite
accurate.
You could either
(A) rename the option to:
fanalyzer-hide-events-in-system-headers
and make it be Init(1), and change the sense of the conditional in
diagnostic_manager::prune_path?
That way the user would suppy:
-fno-analyzer-hide-events-in-system-headers
or:
(B) change the wording to something like
"Show events within system headers in analyzer execution paths."
or somesuch
All options should have a corresponding entry in invoke.texi, so please
add one for the new option (have a look at the existing ones).
> fanalyzer-call-summaries
> Common Var(flag_analyzer_call_summaries) Init(0)
> Approximate the effect of function calls to simplify analysis.
> diff --git a/gcc/analyzer/diagnostic-manager.cc
> b/gcc/analyzer/diagnostic-manager.cc
> index cfca305d552..2a9705a464f 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
I don't see any use of std::vector in the patch; is this a vestige from
an earlier version of the patch?
> #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"
> @@ -2281,6 +2283,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 (! flag_analyzer_show_events_in_system_headers)
> + prune_system_headers (path);
> consolidate_conditions (path);
> finish_pruning (path);
> path->maybe_log (get_logger (), "pruned");
> @@ -2667,6 +2671,67 @@ diagnostic_manager::prune_interproc_events
> (checker_path *path) const
> while (changed);
> }
>
> +/* Remove everything within [call point, IDX]. For consistency,
> + IDX should represent the return event of the frame to delete,
> + or if there is none it should be the last event of the frame.
> + After this function, IDX designates the event prior to calling
> + this frame. */
> +
> +static void
> +prune_frame (checker_path *path, int &idx)
> +{
> + gcc_assert (idx >= 0);
> + int nesting = 1;
> + if (path->get_checker_event (idx)->is_return_p ())
> + nesting = 0;
> + do
> + {
> + 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--);
> + } while (idx >= 0 && nesting != 0);
> +}
FWIW I was expecting this to be based on looking at the stack depths of
the events, rather than looking at the nesting structure of
calls/returns, but the approach in this patch works.
> +
> +void
> +diagnostic_manager::prune_system_headers (checker_path *path) const
This function should have a leading comment. Your diagram from the
commit message is great, so please use/adapt that.
I wonder if there are situations involving callbacks where we could
have events like this:
prefix events...,
system header call,
system header entry,
events within system headers...,
callback into user's code
interesting stuff happens here, in user's code
return from user's code back into system header
events within system headers...,
system header return,
suffix events...
But let's not worry about that for now.
> +{
> + int idx = (signed)path->num_events () - 1;
> + while (idx >= 0)
> + {
> + const checker_event *event = path->get_checker_event (idx);
> + /* Prune everything between
> + [..., system entry, (...), system return, ...]. */
> + if (event->is_return_p ()
> + && in_system_header_at (event->get_location ()))
> + {
> + int ret_idx = idx;
> + label_text desc
> + (path->get_checker_event (ret_idx)->get_desc (false));
Generating the desc does non-trivial work, and is only used by the
logging, so please move this so it's guarded by the if (get_logger ())
conditional.
> + prune_frame (path, idx);
> +
> + if (get_logger ())
> + {
> + log ("filtering event %i-%i:"
> + " system header event: %s",
> + idx, ret_idx, desc.get ());
> + }
> + // Delete function entry within system headers.
> + if (idx >= 0)
> + {
> + event = path->get_checker_event (idx);
> + if (event->is_function_entry_p ()
> + && in_system_header_at (event->get_location ()))
> + 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..d3022b888dd 100644
> --- a/gcc/analyzer/diagnostic-manager.h
> +++ b/gcc/analyzer/diagnostic-manager.h
> @@ -180,6 +180,7 @@ 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 prune_system_headers (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-show-events-in-system-headers-default.C
>
> b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C
> new file mode 100644
> index 00000000000..828afbdd9b4
> --- /dev/null
> +++
> b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C
> @@ -0,0 +1,19 @@
> +/* { dg-additional-options "-fdiagnostics-plain-output" } */
IIRC -fdiagnostics-plain-output is already supplied by DejaGnu.
> +/* { dg-skip-if "no shared_ptr in C++98" { c++98_only } } */
> +
> +#include <memory>
> +
> +struct A {int x; int y;};
> +
> +int main () {
> + std::shared_ptr<A> a; /* { dg-line declare_a } */
> + a->x = 4; /* { dg-line deref_a } */
> + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */
> +
> + return 0;
> +}
> +
> +/* { dg-note "\\(1\\) 'a\\.std::.+::_M_ptr' is NULL" "" { target c++14_down
> } declare_a } */
> +/* { dg-note "dereference of NULL 'a\\.std.+::operator->\\(\\)'" "" { target
> *-*-* } deref_a } */
> +/* { dg-note "calling 'std::.+::operator->' from 'main'" "" { target *-*-* }
> deref_a } */
> +/* { dg-note "returning to 'main' from 'std::.+::operator->'" "" { target
> *-*-* } deref_a } */
> diff --git
> a/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C
> b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C
> new file mode 100644
> index 00000000000..a6e991b0f08
> --- /dev/null
> +++
> b/gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C
> @@ -0,0 +1,19 @@
> +/* { dg-additional-options "-fdiagnostics-plain-output
> -fno-analyzer-show-events-in-system-headers" } */
Likewise here.
> +/* { dg-skip-if "no shared_ptr in C++98" { c++98_only } } */
> +
> +#include <memory>
> +
> +struct A {int x; int y;};
> +
> +int main () {
> + std::shared_ptr<A> a; /* { dg-line declare_a } */
> + a->x = 4; /* { dg-line deref_a } */
> + /* { dg-warning "dereference of NULL" "" { target *-*-* } deref_a } */
> +
> + return 0;
> +}
> +
> +/* { dg-note "\\(1\\) 'a\\.std::.+::_M_ptr' is NULL" "" { target c++14_down
> } declare_a } */
> +/* { dg-note "dereference of NULL 'a\\.std.+::operator->\\(\\)'" "" { target
> *-*-* } deref_a } */
> +/* { dg-note "calling 'std::.+::operator->' from 'main'" "" { target *-*-* }
> deref_a } */
> +/* { dg-note "returning to 'main' from 'std::.+::operator->'" "" { target
> *-*-* } deref_a } */
If I'm reading this right, the dg-note directives in the two tests are
identical. We want some kind of automated test coverage for the
property that events within <memory> and below are shown with one
setting, and *not* shown with the other setting.
Is there some substring we can search for that occurs in the untrimmed
output? (preferably one that isn't going to change as cstdlib
changes). Then we can add a directive that requires it to be found for
one, and requires it to *not* be found for the other.
Or maybe for the "trim system headers" case you could put the event
index prefix on the "returning to main" dg-note "\\(3\\)" or "\\(4\\)",
since if there are extra events, that will change the index. Though
given the "c++14_down" for the first event, it looks like trying to
check the numbering would be a pain to maintain.
Thanks again for the patch; hope this is constructive.
Dave
