warn-access: Fix up matching_alloc_calls_p [PR118024]

The following testcase ICEs because of a bug in matching_alloc_calls_p.
The loop was apparently meant to be walking the two attribute chains
in lock-step, but doesn't really do that.  If the first lookup_attribute
returns non-NULL, the second one is not done, so rmats in that case can
be some random unrelated attribute rather than "malloc" attribute; the
body assumes even rmats if non-NULL is "malloc" attribute and relies
on its argument to be a "malloc" argument and if it is some other
attribute with incompatible attribute, it just crashes.

Now, fixing that in the obvious way, instead of doing
(amats = lookup_attribute ("malloc", amats))
 || (rmats = lookup_attribute ("malloc", rmats))
in the condition do
((amats = lookup_attribute ("malloc", amats)),
 (rmats = lookup_attribute ("malloc", rmats)),
 (amats || rmats))
fixes the testcase but regresses Wmismatched-dealloc-{2,3}.c tests.
The problem is that walking the attribute lists in a lock-step is obviously
a very bad idea, there is no requirement that the same deallocators are
present in the same order on both decls, e.g. there could be an extra malloc
attribute without argument in just one of the lists, or the order of say
free/realloc could be swapped, etc.  We don't generally document nor enforce
any particular ordering of attributes (even when for some attributes we just
handle the first one rather than all).

So, this patch instead simply splits it into two loops, the first one walks
alloc_decl attributes, the second one walks dealloc_decl attributes.
If the malloc attribute argument is a built-in, that doesn't change
anything, and otherwise we have the chance to populate the whole
common_deallocs hash_set in the first loop and then can check it in the
second one (and don't need to use more expensive add method on it, can just
check contains there).  Not to mention that it also fixes the case when
the function would incorrectly return true if there wasn't a common
deallocator between the two, but dealloc_decl had 2 malloc attributes with
the same deallocator.

2024-12-14  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/118024
	* gimple-ssa-warn-access.cc (matching_alloc_calls_p): Walk malloc
	attributes of alloc_decl and dealloc_decl in separate loops rather
	than in lock-step.  Use common_deallocs.contains rather than
	common_deallocs.add in the second loop.

(cherry picked from commit 9537ca5ad9bc23d7e9c446b4a7cbb98f63bddb6a)

diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 8e12e49816f9d8020a2663e8e47e332a858520a7..6edb754f66546762ef864cda27f3330917d9c505 100644 (file)
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -1908,52 +1908,49 @@ matching_alloc_calls_p (tree alloc_decl, tree dealloc_decl)
      headers.
      With AMATS set to the Allocator's Malloc ATtributes,
      and  RMATS set to Reallocator's Malloc ATtributes...  */
-  for (tree amats = DECL_ATTRIBUTES (alloc_decl),
-        rmats = DECL_ATTRIBUTES (dealloc_decl);
-       (amats = lookup_attribute ("malloc", amats))
-        || (rmats = lookup_attribute ("malloc", rmats));
-       amats = amats ? TREE_CHAIN (amats) : NULL_TREE,
-        rmats = rmats ? TREE_CHAIN (rmats) : NULL_TREE)
-    {
-      if (tree args = amats ? TREE_VALUE (amats) : NULL_TREE)
-       if (tree adealloc = TREE_VALUE (args))
-         {
-           if (DECL_P (adealloc)
-               && fndecl_built_in_p (adealloc, BUILT_IN_NORMAL))
-             {
-               built_in_function fncode = DECL_FUNCTION_CODE (adealloc);
-               if (fncode == BUILT_IN_FREE || fncode == BUILT_IN_REALLOC)
-                 {
-                   if (realloc_kind == alloc_kind_t::builtin)
-                     return true;
-                   alloc_dealloc_kind = alloc_kind_t::builtin;
-                 }
-               continue;
-             }
-
-           common_deallocs.add (adealloc);
-         }
+  for (tree amats = DECL_ATTRIBUTES (alloc_decl);
+       (amats = lookup_attribute ("malloc", amats));
+       amats = amats ? TREE_CHAIN (amats) : NULL_TREE)
+    if (tree args = amats ? TREE_VALUE (amats) : NULL_TREE)
+      if (tree adealloc = TREE_VALUE (args))
+       {
+         if (DECL_P (adealloc)
+             && fndecl_built_in_p (adealloc, BUILT_IN_NORMAL))
+           {
+             built_in_function fncode = DECL_FUNCTION_CODE (adealloc);
+             if (fncode == BUILT_IN_FREE || fncode == BUILT_IN_REALLOC)
+               {
+                 if (realloc_kind == alloc_kind_t::builtin)
+                   return true;
+                 alloc_dealloc_kind = alloc_kind_t::builtin;
+               }
+             continue;
+           }
 
-      if (tree args = rmats ? TREE_VALUE (rmats) : NULL_TREE)
-       if (tree ddealloc = TREE_VALUE (args))
-         {
-           if (DECL_P (ddealloc)
-               && fndecl_built_in_p (ddealloc, BUILT_IN_NORMAL))
-             {
-               built_in_function fncode = DECL_FUNCTION_CODE (ddealloc);
-               if (fncode == BUILT_IN_FREE || fncode == BUILT_IN_REALLOC)
-                 {
-                   if (alloc_dealloc_kind == alloc_kind_t::builtin)
-                     return true;
-                   realloc_dealloc_kind = alloc_kind_t::builtin;
-                 }
-               continue;
-             }
+         common_deallocs.add (adealloc);
+       }
+  for (tree rmats = DECL_ATTRIBUTES (dealloc_decl);
+       (rmats = lookup_attribute ("malloc", rmats));
+       rmats = rmats ? TREE_CHAIN (rmats) : NULL_TREE)
+    if (tree args = rmats ? TREE_VALUE (rmats) : NULL_TREE)
+      if (tree ddealloc = TREE_VALUE (args))
+       {
+         if (DECL_P (ddealloc)
+             && fndecl_built_in_p (ddealloc, BUILT_IN_NORMAL))
+           {
+             built_in_function fncode = DECL_FUNCTION_CODE (ddealloc);
+             if (fncode == BUILT_IN_FREE || fncode == BUILT_IN_REALLOC)
+               {
+                 if (alloc_dealloc_kind == alloc_kind_t::builtin)
+                   return true;
+                 realloc_dealloc_kind = alloc_kind_t::builtin;
+               }
+             continue;
+           }
 
-           if (common_deallocs.add (ddealloc))
-             return true;
-         }
-    }
+         if (common_deallocs.contains (ddealloc))
+           return true;
+       }
 
   /* Succeed only if ALLOC_DECL and the reallocator DEALLOC_DECL share
      a built-in deallocator.  */