[PATCH] tree-ssa-strlen: Fix up handle_store [PR113603]

[PATCH] tree-ssa-strlen: Fix up handle_store [PR113603]

[Jakub Jelinek]

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?

2024-01-30  Jakub Jelinek  <jakub@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

Re: [PATCH] tree-ssa-strlen: Fix up handle_store [PR113603]

[Richard Biener]

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  <jakub@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 <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)