Re: [PATCH v2] analyzer: Call off a superseding when diagnostics are unrelated [PR110830]

2023-09-11 Thread Andreas Schwab via Gcc-patches
../../gcc/analyzer/diagnostic-manager.cc: In function 'bool 
ana::compatible_epath_p(const exploded_path*, const exploded_path*)':
../../gcc/analyzer/diagnostic-manager.cc:969:1: warning: control reaches end of 
non-void function [-Wreturn-type]

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."


Re: [PATCH v2] analyzer: Call off a superseding when diagnostics are unrelated [PR110830]

2023-09-06 Thread David Malcolm via Gcc-patches
On Wed, 2023-09-06 at 21:16 +0200, priour...@gmail.com wrote:

[...snip...]

> Signed-off-by: benjamin priour 
> Co-authored-by: david malcolm 

Please also add:

  Signed-off-by: David Malcolm 

[...snip...]

> 
> +static bool
> +compatible_epath_p (const exploded_path *lhs_path,
> +   const exploded_path *rhs_path)
> +{
> +  gcc_assert (lhs_path);
> +  gcc_assert (rhs_path);
> +  gcc_assert (rhs_path->length () > 0);
> +  gcc_assert (rhs_path->length () > 0);
> +  int lhs_eedge_idx = lhs_path->length () -1;
> +  int rhs_eedge_idx = rhs_path->length () -1;

Minor formatting nit: there should be a space between the '-' and the
'1' in the above lines, hence:

  int lhs_eedge_idx = lhs_path->length () - 1;
  int rhs_eedge_idx = rhs_path->length () - 1;

[...snip...]

OK for trunk with those changes

Thanks
Dave



[PATCH v2] analyzer: Call off a superseding when diagnostics are unrelated [PR110830]

2023-09-06 Thread Benjamin Priour via Gcc-patches
From: benjamin priour 

Hi,

Second version of this patch after David's suggestions.
Thanks David for pointing out how I could implement it using sedges.
I hadn't thought of them being independent of the exploded path taken,
and unique for a conditional block's outcome. I had mistaken them with
eedges, those that we see in the *exploded*-graph, therefore since the
two saved_diagnostics can have arbitrary different paths I had to
use a nested loop.

It is much more efficient this way.

Regstrapped off trunk a7d052b3200c7928d903a0242b8cfd75d131e374 on
x86_64-linux-gnu.

Is it ready for trunk ?

Thanks,
Benjamin.

Patch below.
---

Before this patch, a saved_diagnostic would supersede another at
the same statement if and only its vfunc supercedes_p returned true
for the other diagnostic's kind.
That both warning were unrelated - i.e. resolving one would not fix
the other - was not considered in making the above choice.

This patch makes it so that two saved_diagnostics taking a different
outcome of at least one common conditional branching cannot supersede
each other.

Signed-off-by: benjamin priour 
Co-authored-by: david malcolm 

gcc/analyzer/ChangeLog:

PR analyzer/110830
* diagnostic-manager.cc
(compatible_epaths_p): New function.
(saved_diagnostic::supercedes_p): Now calls the above
to determine if the diagnostics do overlap and the superseding
may proceed.

gcc/testsuite/ChangeLog:

PR analyzer/110830
* c-c++-common/analyzer/pr110830.c: New test.
---
 gcc/analyzer/diagnostic-manager.cc|  90 +-
 .../c-c++-common/analyzer/pr110830.c  | 111 ++
 2 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/c-c++-common/analyzer/pr110830.c

diff --git a/gcc/analyzer/diagnostic-manager.cc 
b/gcc/analyzer/diagnostic-manager.cc
index 10fea486b8c..90c56a350e7 100644
--- a/gcc/analyzer/diagnostic-manager.cc
+++ b/gcc/analyzer/diagnostic-manager.cc
@@ -887,6 +887,88 @@ saved_diagnostic::add_duplicate (saved_diagnostic *other)
   m_duplicates.safe_push (other);
 }
 
+/* Walk up the sedges of each of the two paths.
+   If the two sequences of sedges do not perfectly correspond,
+   then paths are incompatible.
+   If there is at least one sedge that either cannot be paired up
+   or its counterpart is not equal, then the paths are incompatible
+   and this function returns FALSE.
+   Otherwise return TRUE.
+
+   Incompatible paths:
+
+   
+   /  \
+  /\
+true  false
+ |  |
+......
+ |  |
+...   stmt x
+ |
+   stmt x
+
+   Both LHS_PATH and RHS_PATH final enodes should be
+   over the same gimple statement.  */
+
+static bool
+compatible_epath_p (const exploded_path *lhs_path,
+   const exploded_path *rhs_path)
+{
+  gcc_assert (lhs_path);
+  gcc_assert (rhs_path);
+  gcc_assert (rhs_path->length () > 0);
+  gcc_assert (rhs_path->length () > 0);
+  int lhs_eedge_idx = lhs_path->length () -1;
+  int rhs_eedge_idx = rhs_path->length () -1;
+  const exploded_edge *lhs_eedge;
+  const exploded_edge *rhs_eedge;
+
+  while (lhs_eedge_idx >= 0 && rhs_eedge_idx >= 0)
+{
+  while (lhs_eedge_idx >= 0)
+   {
+ /* Find LHS_PATH's next superedge.  */
+ lhs_eedge = lhs_path->m_edges[lhs_eedge_idx];
+ if (lhs_eedge->m_sedge)
+   break;
+ else
+   lhs_eedge_idx--;
+   }
+  while (rhs_eedge_idx >= 0)
+   {
+ /* Find RHS_PATH's next superedge.  */
+ rhs_eedge = rhs_path->m_edges[rhs_eedge_idx];
+ if (rhs_eedge->m_sedge)
+   break;
+ else
+   rhs_eedge_idx--;
+   }
+
+  if (lhs_eedge->m_sedge && rhs_eedge->m_sedge)
+   {
+ if (lhs_eedge->m_sedge != rhs_eedge->m_sedge)
+   /* Both superedges do not match.
+  Superedges are not dependent on the exploded path, so even
+  different epaths will have similar sedges if they follow
+  the same outcome of a conditional node.  */
+   return false;
+
+ lhs_eedge_idx--;
+ rhs_eedge_idx--;
+ continue;
+   }
+  else if (lhs_eedge->m_sedge == nullptr && rhs_eedge->m_sedge == nullptr)
+   /* Both paths were drained up entirely.
+  No discriminant was found.  */
+   return true;
+
+  /* A superedge was found for only one of the two paths.  */
+  return false;
+}
+}
+
+
 /* Return true if this diagnostic supercedes OTHER, and that OTHER should
therefore not be emitted.  */
 
@@ -896,7 +978,13 @@ saved_diagnostic::supercedes_p (const saved_diagnostic 
) const
   /* They should be at the same stmt.  */
   if (m_stmt != other.m_stmt)
 return false;
-  return m_d->supercedes_p (*other.m_d);
+  /* return early if OTHER won't be superseded anyway.  */
+  if (!m_d->supercedes_p (*other.m_d))
+return false;
+
+  /* If the two