On Thu, May 14, 2020 at 10:10:55AM -0600, Jeff Law wrote:
> On Tue, 2020-05-12 at 10:12 +0200, Jakub Jelinek wrote:
> > Hi!
> >
> > In the following testcase, store_expr of e.g. 97 bytes long string literal
> > into 1MB long array is implemented by copying the 97 bytes from .rodata
> > section, followed by clearing the remaining bytes. But, as the STRING_CST
> > has type char[1024*1024], we actually allocate whole 1MB in .rodata section
> > for it, even when we only use the first 97 bytes from that.
> >
> > The following patch tweaks it so that if we are going to initialize only the
> > small part from it, we don't emit all the zeros that we never use after it.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > 2020-05-12 Jakub Jelinek <ja...@redhat.com>
> >
> > PR middle-end/95052
> > * expr.c (store_expr): If expr_size is constant and significantly
> > larger than TREE_STRING_LENGTH, set temp to just the
> > TREE_STRING_LENGTH portion of the STRING_CST.
> >
> > * gcc.target/i386/pr95052.c: New test.
> OK. Initially I had a number of concerns, but this only affects the .rodata
> that's used to initialize the real object. The .rodata bits have no other
> purpose and aren't otherwise accessable.
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?
--- gcc/expr.c.jj 2020-05-14 18:43:46.324145031 +0200
+++ gcc/expr.c 2020-05-14 18:47:19.003950500 +0200
@@ -5583,6 +5583,7 @@ store_expr (tree exp, rtx target, int ca
rtx temp;
rtx alt_rtl = NULL_RTX;
location_t loc = curr_insn_location ();
+ bool shortened_string_cst = false;
if (VOID_TYPE_P (TREE_TYPE (exp)))
{
@@ -5771,6 +5772,7 @@ store_expr (tree exp, rtx target, int ca
= build_index_type (size_int (TREE_STRING_LENGTH (exp) - 1));
TREE_TYPE (rexp) = build_array_type (TREE_TYPE (TREE_TYPE (exp)),
index);
+ shortened_string_cst = true;
}
}
temp = expand_expr_real (rexp, tmp_target, GET_MODE (target),
@@ -5787,6 +5789,7 @@ store_expr (tree exp, rtx target, int ca
&& TREE_CODE (exp) != ERROR_MARK
&& GET_MODE (target) != TYPE_MODE (TREE_TYPE (exp)))
{
+ gcc_assert (!shortened_string_cst);
if (GET_MODE_CLASS (GET_MODE (target))
!= GET_MODE_CLASS (TYPE_MODE (TREE_TYPE (exp)))
&& known_eq (GET_MODE_BITSIZE (GET_MODE (target)),
@@ -5839,6 +5842,7 @@ store_expr (tree exp, rtx target, int ca
{
if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != VOIDmode)
{
+ gcc_assert (!shortened_string_cst);
if (GET_MODE (target) == BLKmode)
{
/* Handle calls that return BLKmode values in registers. */
@@ -5924,6 +5928,8 @@ store_expr (tree exp, rtx target, int ca
emit_label (label);
}
}
+ else if (shortened_string_cst)
+ gcc_unreachable ();
/* Handle calls that return values in multiple non-contiguous locations.
The Irix 6 ABI has examples of this. */
else if (GET_CODE (target) == PARALLEL)
@@ -5953,6 +5959,8 @@ store_expr (tree exp, rtx target, int ca
emit_move_insn (target, temp);
}
}
+ else
+ gcc_assert (!shortened_string_cst);
return NULL_RTX;
}
Jakub
