Hi!
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.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-12-14 Jakub Jelinek <ja...@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.
* gcc.dg/pr118024.c: New test.
--- gcc/gimple-ssa-warn-access.cc.jj 2024-12-07 11:35:49.467439817 +0100
+++ gcc/gimple-ssa-warn-access.cc 2024-12-13 13:03:13.626700818 +0100
@@ -1935,52 +1935,49 @@ matching_alloc_calls_p (tree alloc_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;
- }
+ 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;
+ }
- common_deallocs.add (adealloc);
- }
+ 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 (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. */
--- gcc/testsuite/gcc.dg/pr118024.c.jj 2024-12-13 13:58:41.022588554 +0100
+++ gcc/testsuite/gcc.dg/pr118024.c 2024-12-13 13:58:17.189925679 +0100
@@ -0,0 +1,15 @@
+/* PR middle-end/118024 */
+/* { dg-do compile } */
+/* { dg-options "-fstrub=all" } */
+/* { dg-require-effective-target strub } */
+
+void *realloc (void *, __SIZE_TYPE__);
+void *reallocarray (void *);
+void *reallocarray (void *) __attribute__((__malloc__(reallocarray)));
+
+void *
+foo (void)
+{
+ char *buf = reallocarray (0);
+ return realloc (buf, 1);
+}
Jakub
