On Tue, 30 Jan 2024, Jakub Jelinek wrote:
> Hi!
>
> Since r10-2101-gb631bdb3c16e85f35d3 handle_store uses
> count_nonzero_bytes{,_addr} which (more recently limited to statements
> with the same vuse) can walk earlier statements feeding the rhs
> of the store and call get_stridx on it.
> Unlike most of the other functions where get_stridx is called first on
> rhs and only later on lhs, handle_store calls get_stridx on the lhs before
> the count_nonzero_bytes* call and does some si->nonzero_bytes comparison
> on it.
> Now, strinfo structures are refcounted and it is important not to screw
> it up.
> What happens on the following testcase is that we call get_strinfo on the
> destination idx's base (g), which returns a strinfo at that moment
> with refcount of 2, one copy referenced in bb 2 final strinfos, one in bb 3
> (the vector of strinfos was unshared from the dominator there because some
> other strinfo was added) and finally we process a store in bb 6.
> Now, count_nonzero_bytes is called and that sees &g[1] in a PHI and
> calls get_stridx on it, which in turn calls get_stridx_plus_constant
> because &g + 1 address doesn't have stridx yet. This creates a new
> strinfo for it:
> si = new_strinfo (ptr, idx, build_int_cst (size_type_node, nonzero_chars),
> basesi->full_string_p);
> set_strinfo (idx, si);
> and the latter call, because it is the first one in bb 6 that needs it,
> unshares the stridx_to_strinfo vector (so refcount of the g strinfo becomes
> 3).
> Now, get_stridx_plus_constant needs to chain the new strinfo of &g[1] in
> between the related strinfos, so after the g record. Because the strinfo
> is now shared between the current bb and 2 other bbs, it needs to
> unshare_strinfo it (creating a new strinfo which can be modified as a copy
> of the old one, decrementing refcount of the old shared one and setting
> refcount of the new one to 1):
> if (strinfo *nextsi = get_strinfo (chainsi->next))
> {
> nextsi = unshare_strinfo (nextsi);
> si->next = nextsi->idx;
> nextsi->prev = idx;
> }
> chainsi = unshare_strinfo (chainsi);
> if (chainsi->first == 0)
> chainsi->first = chainsi->idx;
> chainsi->next = idx;
> Now, the bug is that the caller of this a couple of frames above,
> handle_store, holds on a pointer to this g strinfo (but doesn't know
> about the unsharing, so the pointer is to the old strinfo with refcount
> of 2), and later needs to update it, so it
> si = unshare_strinfo (si);
> and modifies some fields in it.
> This creates a new strinfo (with refcount of 1 which is stored into
> the vector of the current bb) based on the old strinfo for g and
> decrements refcount of the old one to 1. So, now we are in inconsistent
> state, because the old strinfo for g is referenced in bb 2 and bb 3
> vectors, but has just refcount of 1, and then have one strinfo (the one
> created by unshare_strinfo (chainsi) in get_stridx_plus_constant) which
> has refcount of 1 but isn't referenced from anywhere anymore.
> Later on when we free one of the bb 2 or bb 3 vectors (forgot which)
> that decrements refcount from 1 to 0 and poisons the strinfo/returns it to
> the pool, but then maybe_invalidate when looking at the other bb's pointer
> to it ICEs.
>
> The following patch fixes it by calling get_strinfo again, it is guaranteed
> to return non-NULL, but could be an unshared copy instead of the originally
> fetched shared one.
>
> I believe we only need to do this refetching for the case where get_strinfo
> is called on the lhs before get_stridx is called on other operands, because
> we should be always modifying (apart from the chaining changes) the strinfo
> for the destination of the statements, not other strinfos just consumed in
> there.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK
> 2024-01-30 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/113603
> * tree-ssa-strlen.cc (strlen_pass::handle_store): After
> count_nonzero_bytes call refetch si using get_strinfo in case it
> has been unshared in the meantime.
>
> * gcc.c-torture/compile/pr113603.c: New test.
>
> --- gcc/tree-ssa-strlen.cc.jj 2024-01-29 10:20:25.000000000 +0100
> +++ gcc/tree-ssa-strlen.cc 2024-01-29 15:50:17.056461933 +0100
> @@ -5044,6 +5044,9 @@ strlen_pass::handle_store (bool *zero_wr
>
> if (si != NULL)
> {
> + /* The count_nonzero_bytes call above might have unshared si.
> + Fetch it again from the vector. */
> + si = get_strinfo (idx);
> /* The corresponding element is set to 1 if the first and last
> element, respectively, of the sequence of characters being
> written over the string described by SI ends before
> --- gcc/testsuite/gcc.c-torture/compile/pr113603.c.jj 2024-01-29
> 16:09:54.335227319 +0100
> +++ gcc/testsuite/gcc.c-torture/compile/pr113603.c 2024-01-29
> 16:09:36.010481829 +0100
> @@ -0,0 +1,40 @@
> +/* PR tree-optimization/113603 */
> +
> +int a, e;
> +signed char b;
> +int *c;
> +signed char *d;
> +short f;
> +signed char g[3];
> +
> +int *
> +foo (void)
> +{
> + for (int i = 0; i < 3; i++)
> + g[i] = 2;
> + int j[100][100] = { {}, {4} };
> + signed char *k = &g[1];
> + do
> + {
> + for (;;)
> + {
> + if (c)
> + break;
> + return &a;
> + }
> + for (f = 0;; f++)
> + {
> + for (b = 0; b < 2; b++)
> + *c = j[b][f];
> + if (e)
> + d = k;
> + *k = *d;
> + if (*c)
> + break;
> + if (f)
> + break;
> + }
> + }
> + while (f);
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
