| Message ID | 22d70b27-ddad-6410-3c59-1e630d3d7d55@suse.cz |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 23, 2016 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote: > Following patch fixes situation where we do a store to a bitfield which > is at boundary of a record. This leads to usage of wider store, leading > to overwriting a following memory location. > > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > Apart from that, the reported test-case in PR works on x86_64-linux-gnu. > > Ready to be installed? + HOST_WIDE_INT bitregion_end + = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1; I don't think looking at the CONSTRUCTOR to determine bitregion_end is a good idea. The function gets 'size' as argument which is documented as "number of bytes we are allowed to modify" - so better use that. @@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size, MEM_KEEP_ALIAS_SET_P (to_rtx) = 1; } - store_constructor_field (to_rtx, bitsize, bitpos, mode, + store_constructor_field (to_rtx, bitsize, bitpos, + 0, bitregion_end, mode, value, cleared, get_alias_set (TREE_TYPE (field)), reverse); this stores to to_rtx which may be offsetted from target this means in this case bitregion_end is not conservative enough - you'd need to resort to the field width in that case I guess (and for variable field size not specify any end -- I suppose the 'size' store_constructor gets might also be "unknown"?). But maybe all the non-constant offset / size cases are "dead code" now that we are in GIMPLE? Note they likely can only appear from Ada code anyway -- CCing Eric. I suppose a "safe" thing to do would be to give up on the first variable offset/size and re-set bitregion_end to zero for this and all following fields. The other cases look fine to me. Thanks, Richard. > Martin
Pinging Eric. On 11/23/2016 11:49 AM, Richard Biener wrote: > On Wed, Nov 23, 2016 at 11:26 AM, Martin Liška <mliska@suse.cz> wrote: >> Following patch fixes situation where we do a store to a bitfield which >> is at boundary of a record. This leads to usage of wider store, leading >> to overwriting a following memory location. >> >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> Apart from that, the reported test-case in PR works on x86_64-linux-gnu. >> >> Ready to be installed? > > + HOST_WIDE_INT bitregion_end > + = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1; > > I don't think looking at the CONSTRUCTOR to determine bitregion_end is > a good idea. > The function gets 'size' as argument which is documented as "number of > bytes we are > allowed to modify" - so better use that. > > @@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int > cleared, HOST_WIDE_INT size, > MEM_KEEP_ALIAS_SET_P (to_rtx) = 1; > } > > - store_constructor_field (to_rtx, bitsize, bitpos, mode, > + store_constructor_field (to_rtx, bitsize, bitpos, > + 0, bitregion_end, mode, > value, cleared, > get_alias_set (TREE_TYPE (field)), > reverse); > > this stores to to_rtx which may be offsetted from target this means in this case > bitregion_end is not conservative enough - you'd need to resort to the > field width > in that case I guess (and for variable field size not specify any end > -- I suppose > the 'size' store_constructor gets might also be "unknown"?). But maybe all > the non-constant offset / size cases are "dead code" now that we are in GIMPLE? > Note they likely can only appear from Ada code anyway -- CCing Eric. > > I suppose a "safe" thing to do would be to give up on the first > variable offset/size > and re-set bitregion_end to zero for this and all following fields. > > The other cases look fine to me. > > Thanks, > Richard. > >> Martin
> But maybe all the non-constant offset / size cases are "dead code" now that > we are in GIMPLE? Note they likely can only appear from Ada code anyway Possibly, but to be sure put a call to gcc_unreachable where appropriate and run the Ada testsuite; if it is clean, then send me the patch and I'll run it on a more thorough testsuite.
From 69961d69e551d9abe31eb7946d5211b99bbe1479 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 22 Nov 2016 10:15:42 +0100
Subject: [PATCH] Fill bitregion_{start,end} in store_constructor (PR
tree-optimization/78428).
gcc/testsuite/ChangeLog:
2016-11-22 Martin Liska <mliska@suse.cz>
Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/78428
* gcc.dg/tree-ssa/pr78428.c: New test.
gcc/ChangeLog:
2016-11-22 Martin Liska <mliska@suse.cz>
Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/78428
* expr.c (store_constructor_field): Add new arguments.
(store_constructor): Fill up store_constructor_field for
FIELD_DECL of a store_constructor_fielductor.
---
gcc/expr.c | 26 ++++++++++++++++++--------
gcc/testsuite/gcc.dg/tree-ssa/pr78428.c | 27 +++++++++++++++++++++++++++
2 files changed, 45 insertions(+), 8 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr78428.c
diff --git a/gcc/expr.c b/gcc/expr.c
index fe752fb..50bc7c5 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -80,7 +80,8 @@ static void clear_by_pieces (rtx, unsigned HOST_WIDE_INT, unsigned int);
static rtx_insn *compress_float_constant (rtx, rtx);
static rtx get_subtarget (rtx);
static void store_constructor_field (rtx, unsigned HOST_WIDE_INT,
- HOST_WIDE_INT, machine_mode,
+ HOST_WIDE_INT, unsigned HOST_WIDE_INT,
+ unsigned HOST_WIDE_INT, machine_mode,
tree, int, alias_set_type, bool);
static void store_constructor (tree, rtx, int, HOST_WIDE_INT, bool);
static rtx store_field (rtx, HOST_WIDE_INT, HOST_WIDE_INT,
@@ -6077,7 +6078,10 @@ all_zeros_p (const_tree exp)
static void
store_constructor_field (rtx target, unsigned HOST_WIDE_INT bitsize,
- HOST_WIDE_INT bitpos, machine_mode mode,
+ HOST_WIDE_INT bitpos,
+ unsigned HOST_WIDE_INT bitregion_start,
+ unsigned HOST_WIDE_INT bitregion_end,
+ machine_mode mode,
tree exp, int cleared,
alias_set_type alias_set, bool reverse)
{
@@ -6112,8 +6116,8 @@ store_constructor_field (rtx target, unsigned HOST_WIDE_INT bitsize,
reverse);
}
else
- store_field (target, bitsize, bitpos, 0, 0, mode, exp, alias_set, false,
- reverse);
+ store_field (target, bitsize, bitpos, bitregion_start, bitregion_end, mode,
+ exp, alias_set, false, reverse);
}
@@ -6148,6 +6152,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
{
tree type = TREE_TYPE (exp);
HOST_WIDE_INT exp_size = int_size_in_bytes (type);
+ HOST_WIDE_INT bitregion_end
+ = exp_size == -1 ? 0 : exp_size * BITS_PER_UNIT - 1;
switch (TREE_CODE (type))
{
@@ -6308,7 +6314,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
MEM_KEEP_ALIAS_SET_P (to_rtx) = 1;
}
- store_constructor_field (to_rtx, bitsize, bitpos, mode,
+ store_constructor_field (to_rtx, bitsize, bitpos,
+ 0, bitregion_end, mode,
value, cleared,
get_alias_set (TREE_TYPE (field)),
reverse);
@@ -6468,7 +6475,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
}
store_constructor_field
- (target, bitsize, bitpos, mode, value, cleared,
+ (target, bitsize, bitpos, 0, bitregion_end,
+ mode, value, cleared,
get_alias_set (elttype), reverse);
}
}
@@ -6571,7 +6579,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
target = copy_rtx (target);
MEM_KEEP_ALIAS_SET_P (target) = 1;
}
- store_constructor_field (target, bitsize, bitpos, mode, value,
+ store_constructor_field (target, bitsize, bitpos, 0,
+ bitregion_end, mode, value,
cleared, get_alias_set (elttype),
reverse);
}
@@ -6705,7 +6714,8 @@ store_constructor (tree exp, rtx target, int cleared, HOST_WIDE_INT size,
? TYPE_MODE (TREE_TYPE (value))
: eltmode;
bitpos = eltpos * elt_size;
- store_constructor_field (target, bitsize, bitpos, value_mode,
+ store_constructor_field (target, bitsize, bitpos, 0,
+ bitregion_end, value_mode,
value, cleared, alias, reverse);
}
}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c b/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c
new file mode 100644
index 0000000..3a9b99c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr78428.c
@@ -0,0 +1,27 @@
+/* PR tree-optimization/78428. */
+/* { dg-options "-O2" } */
+/* { dg-do run } */
+
+struct S0
+{
+ int f2;
+ int f3:16;
+ int f4:18;
+} ;
+
+int a = 5;
+struct S0 b = { 3, 0, 0 };
+static struct S0 global[2] = { { 77, 0, 78 }, { 77, 0, 78 } };
+
+int main ()
+{
+ volatile struct S0 *j;
+ for (; a;)
+ {
+ __builtin_printf ("", b.f2);
+ j = &b;
+ *j = global[1];
+ a--;
+ }
+ return 0;
+}
--
2.10.2