> Am 14.12.2024 um 09:20 schrieb Jakub Jelinek <ja...@redhat.com>:
>
> 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?
Ok
Thanks,
Richard
> 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
>
