expr: Fix fallout from optimize store_expr from STRING_CST [PR95052]

Message ID 20200529200339.GS8462@tucnak
State New
Headers show
Series
  • expr: Fix fallout from optimize store_expr from STRING_CST [PR95052]
Related show

Commit Message

Kees Cook via Gcc-patches May 29, 2020, 8:03 p.m.
On Fri, May 15, 2020 at 10:32:00AM -0600, Jeff Law via Gcc-patches wrote:
> > I wasn't sure if it wouldn't be safer to add some bool flag set to true by

> > the new code and then add gcc_assert in all the other paths, like following

> > incremental patch.  I believe none of the asserts can trigger right now,

> > but the code is still adjusting what it plans to use as source before actually

> > only copying fewer bytes from it, so if somebody changes it later...

> > 

> > Thoughts on that?

> Can't hurt, and debugging the assert tripping is likely a hell of a lot easier

> than debugging the resultant incorrect code.   So if it passes, then I'd say go

> for it.


Testing passed, so I've committed it with those asserts (and thankfully I've
added them!) but it apparently broke Linux kernel build on arm.

The problem is that if the STRING_CST is very short, while the full object
has BLKmode, the short string could very well have
QImode/HImode/SImode/DImode and in that case it wouldn't take the path that
copies the string and then clears the remaining space, but different paths
in which it will ICE because of those asserts and without those it would
just emit wrong-code.

The following patch fixes it by enforcing BLKmode for the string MEM, even
if it is short, so that we copy it and memset the rest.

Ok for trunk if it passes bootstrap/regtest?

2020-05-29  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/95052
	* expr.c (store_expr): For shortedned_string_cst, ensure temp has
	BLKmode.

	* gcc.dg/pr95052.c: New test.



	Jakub

Comments

Kees Cook via Gcc-patches May 30, 2020, 3:34 p.m. | #1
On Fri, 2020-05-29 at 22:03 +0200, Jakub Jelinek wrote:
> On Fri, May 15, 2020 at 10:32:00AM -0600, Jeff Law via Gcc-patches wrote:

> > > I wasn't sure if it wouldn't be safer to add some bool flag set to true by

> > > the new code and then add gcc_assert in all the other paths, like following

> > > incremental patch.  I believe none of the asserts can trigger right now,

> > > but the code is still adjusting what it plans to use as source before

> > > actually

> > > only copying fewer bytes from it, so if somebody changes it later...

> > > 

> > > Thoughts on that?

> > Can't hurt, and debugging the assert tripping is likely a hell of a lot

> > easier

> > than debugging the resultant incorrect code.   So if it passes, then I'd say

> > go

> > for it.

> 

> Testing passed, so I've committed it with those asserts (and thankfully I've

> added them!) but it apparently broke Linux kernel build on arm.

Mips trips over this as well.


> 

> The problem is that if the STRING_CST is very short, while the full object

> has BLKmode, the short string could very well have

> QImode/HImode/SImode/DImode and in that case it wouldn't take the path that

> copies the string and then clears the remaining space, but different paths

> in which it will ICE because of those asserts and without those it would

> just emit wrong-code.

> 

> The following patch fixes it by enforcing BLKmode for the string MEM, even

> if it is short, so that we copy it and memset the rest.

> 

> Ok for trunk if it passes bootstrap/regtest?

> 

> 2020-05-29  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR middle-end/95052

> 	* expr.c (store_expr): For shortedned_string_cst, ensure temp has

> 	BLKmode.

> 

> 	* gcc.dg/pr95052.c: New test.

OK
jeff
>

Patch

--- gcc/expr.c.jj	2020-05-29 10:42:26.000000000 +0200
+++ gcc/expr.c	2020-05-29 21:49:11.421646101 +0200
@@ -5779,6 +5779,11 @@  store_expr (tree exp, rtx target, int ca
 			       (call_param_p
 				? EXPAND_STACK_PARM : EXPAND_NORMAL),
 			       &alt_rtl, false);
+      if (shortened_string_cst)
+	{
+	  gcc_assert (MEM_P (temp));
+	  temp = change_address (temp, BLKmode, NULL_RTX);
+	}
     }
 
   /* If TEMP is a VOIDmode constant and the mode of the type of EXP is not
--- gcc/testsuite/gcc.dg/pr95052.c.jj	2020-05-29 21:56:15.139426809 +0200
+++ gcc/testsuite/gcc.dg/pr95052.c	2020-05-29 21:55:51.919767620 +0200
@@ -0,0 +1,12 @@ 
+/* PR middle-end/95052 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fconserve-stack" } */
+
+void bar (char *);
+
+void
+foo (void)
+{
+  char buf[70] = "";
+  bar (buf);
+}