Fix tree-ssa-strlen handling of partial clobber (PR85814)

Message ID 87po1tmlka.fsf@linaro.org
State New
Headers show
Series
  • Fix tree-ssa-strlen handling of partial clobber (PR85814)
Related show

Commit Message

Richard Sandiford May 18, 2018, 8:09 a.m.
In this PR we have:

  c_5 = c_4(D) + 4;
  c_12 = c_5 + 1;
  *c_5 = 2;
  a = 2;		// A
  c_21 = c_12 + 1;
  *c_12 = 2;
  a = 2;		// B
  c_28 = c_21 + 1;
  *c_21 = 2;
  a = 2;
  c_7 = c_28 + 1;
  *c_28 = 2;

where a is a global int.  We decide that A can't clobber *c_5 == c_4[4]
because the latter implies that c_4 is an object of 5 bytes or more,
whereas a has exactly 4 bytes.

The assumption for B and *c_5 is the same, but when considering B and
*c_12, we only follow the definition of c_12 to c_5 + 1 (for good
reason) and so have *c_12 == c_5[1].  We then don't have the same
size guarantee and so assume that B could clobber *c_12.  This leads
to a situation in which the strinfo for c_5 is still valid but the
next strinfo (c_12) isn't.  We then segfaulted while trying to get
the strinfo for c_21 + 1 == c_5 + 3 because get_stridx_plus_constant
assumed that c_5's next strinfo (c_12) would be valid too.

And of course it should be valid really.  It doesn't make sense for the
string based at c_5 to be valid but a substring of it to be invalid.
I don't think we can guarantee that such weird corner cases never
happen though, even if we tried to avoid this one.

One possibility would be to mark c_12 as valid on the basis that c_5
is valid, but I'm not sure the complication is worth it given that it
seems to trigger very rarely.  A better optimisation would be to get
the unroller to clean up after itself a bit more...

Although this particular instance of the bug relies on r249880, I think
we could have similar problems in GCC 7.  It would be much harder to
trigger though, especially since it relies on unfolded IR like the above.

Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.
OK for trunk and GCC 8?

Richard


2018-05-18  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR tree-optimization/85814
	* tree-ssa-strlen.c (get_stridx_plus_constant): Cope with
	a null return from get_strinfo when unsharing the next
	strinfo in the chain.

gcc/testsuite/
	PR tree-optimization/85814
	* gcc.dg/torture/pr85814.c: New test.

Comments

Jeff Law May 21, 2018, 8:24 p.m. | #1
On 05/18/2018 02:09 AM, Richard Sandiford wrote:
> In this PR we have:

> 

>   c_5 = c_4(D) + 4;

>   c_12 = c_5 + 1;

>   *c_5 = 2;

>   a = 2;		// A

>   c_21 = c_12 + 1;

>   *c_12 = 2;

>   a = 2;		// B

>   c_28 = c_21 + 1;

>   *c_21 = 2;

>   a = 2;

>   c_7 = c_28 + 1;

>   *c_28 = 2;

> 

> where a is a global int.  We decide that A can't clobber *c_5 == c_4[4]

> because the latter implies that c_4 is an object of 5 bytes or more,

> whereas a has exactly 4 bytes.

> 

> The assumption for B and *c_5 is the same, but when considering B and

> *c_12, we only follow the definition of c_12 to c_5 + 1 (for good

> reason) and so have *c_12 == c_5[1].  We then don't have the same

> size guarantee and so assume that B could clobber *c_12.  This leads

> to a situation in which the strinfo for c_5 is still valid but the

> next strinfo (c_12) isn't.  We then segfaulted while trying to get

> the strinfo for c_21 + 1 == c_5 + 3 because get_stridx_plus_constant

> assumed that c_5's next strinfo (c_12) would be valid too.

> 

> And of course it should be valid really.  It doesn't make sense for the

> string based at c_5 to be valid but a substring of it to be invalid.

> I don't think we can guarantee that such weird corner cases never

> happen though, even if we tried to avoid this one.

> 

> One possibility would be to mark c_12 as valid on the basis that c_5

> is valid, but I'm not sure the complication is worth it given that it

> seems to trigger very rarely.  A better optimisation would be to get

> the unroller to clean up after itself a bit more...

> 

> Although this particular instance of the bug relies on r249880, I think

> we could have similar problems in GCC 7.  It would be much harder to

> trigger though, especially since it relies on unfolded IR like the above.

> 

> Tested on aarch64-linux-gnu, aarch64_be-elf and x86_64-linux-gnu.

> OK for trunk and GCC 8?

> 

> Richard

> 

> 

> 2018-05-18  Richard Sandiford  <richard.sandiford@linaro.org>

> 

> gcc/

> 	PR tree-optimization/85814

> 	* tree-ssa-strlen.c (get_stridx_plus_constant): Cope with

> 	a null return from get_strinfo when unsharing the next

> 	strinfo in the chain.

> 

> gcc/testsuite/

> 	PR tree-optimization/85814

> 	* gcc.dg/torture/pr85814.c: New test.

OK, including backports.

jeff

Patch

Index: gcc/tree-ssa-strlen.c
===================================================================
--- gcc/tree-ssa-strlen.c	2018-03-13 15:06:00.657514354 +0000
+++ gcc/tree-ssa-strlen.c	2018-05-18 09:05:44.691476297 +0100
@@ -795,9 +795,9 @@  get_stridx_plus_constant (strinfo *bases
   si = new_strinfo (ptr, idx, build_int_cst (size_type_node, nonzero_chars),
 		    basesi->full_string_p);
   set_strinfo (idx, si);
-  if (chainsi->next)
+  if (strinfo *nextsi = get_strinfo (chainsi->next))
     {
-      strinfo *nextsi = unshare_strinfo (get_strinfo (chainsi->next));
+      nextsi = unshare_strinfo (nextsi);
       si->next = nextsi->idx;
       nextsi->prev = idx;
     }
Index: gcc/testsuite/gcc.dg/torture/pr85814.c
===================================================================
--- /dev/null	2018-04-20 16:19:46.369131350 +0100
+++ gcc/testsuite/gcc.dg/torture/pr85814.c	2018-05-18 09:05:44.689476382 +0100
@@ -0,0 +1,7 @@ 
+int a;
+void b(char *c)
+{
+  c += 4;
+  for (int i = 0; i < 4; i++)
+    a = *c++ = 2;
+}