Re: [PATCH] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]

[David Malcolm via Gcc-patches]

On Fri, 2023-08-11 at 13:51 +0200, priour...@gmail.com wrote:
> From: benjamin priour 

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 
> 

[...]

> 
> 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,
> + 

Re: [PATCH] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]

[Benjamin Priour via Gcc-patches]

I forgot to mention that this has been successfully regstrapped off trunk
54be338589ea93ad4ff53d22adde476a0582537b on x86_64-linux-gnu.

Is it OK for trunk ?

Thanks,
Benjamin.

[PATCH] analyzer: New option fanalyzer-show-events-in-system-headers [PR110543]

[Benjamin Priour via Gcc-patches]

From: benjamin priour 

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 

gcc/analyzer/ChangeLog:

PR analyzer/110543
* analyzer.cc (is_std_function_p): No longer static.
* analyzer.h (is_std_function_p): Add declaration.
* analyzer.opt: Add new option.
* diagnostic-manager.cc
(INCLUDE_VECTOR): Include vector from system.h
(diagnostic_manager::prune_path): Call prune_system_headers.
(prune_frame): New function that deletes all events in a frame.
(diagnostic_manager::prune_system_headers): New function.
* diagnostic-manager.h: Add prune_system_headers declaration.

gcc/testsuite/ChangeLog:

PR analyzer/110543
* g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C:
New test.
* g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.C:
New test.
---
 gcc/analyzer/analyzer.cc  |  2 +-
 gcc/analyzer/analyzer.h   |  1 +
 gcc/analyzer/analyzer.opt |  4 ++
 gcc/analyzer/diagnostic-manager.cc| 65 +++
 gcc/analyzer/diagnostic-manager.h |  1 +
 ...er-show-events-in-system-headers-default.C | 19 ++
 ...nalyzer-show-events-in-system-headers-no.C | 19 ++
 7 files changed, 110 insertions(+), 1 deletion(-)
 create mode 100644 
gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-default.C
 create mode 100644 
gcc/testsuite/g++.dg/analyzer/fanalyzer-show-events-in-system-headers-no.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..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.
+
 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
 #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