From: Richard Biener <rguenther@suse.de>
To: Martin Jambor <mjambor@suse.cz>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] tree-sra: Fix union handling in build_reconstructed_reference (PR 105860)
Date: Tue, 5 Jul 2022 07:34:19 +0000 (UTC) [thread overview]
Message-ID: <nycvar.YFH.7.77.849.2207050731070.14950@jbgna.fhfr.qr> (raw)
In-Reply-To: <ri6let9hr1x.fsf@suse.cz>
On Mon, 4 Jul 2022, Martin Jambor wrote:
> Hi,
>
> On Mon, Jul 04 2022, Richard Biener wrote:
> > On Fri, 1 Jul 2022, Martin Jambor wrote:
> >
> >> Hi,
> >>
> >> As the testcase in PR 105860 shows, the code that tries to re-use the
> >> handled_component chains in SRA can be horribly confused by unions,
> >> where it thinks it has found a compatible structure under which it can
> >> chain the references, but in fact it found the type it was looking
> >> for elsewhere in a union and generated a write to a completely wrong
> >> part of an aggregate.
> >>
> >> I don't remember whether the plan was to support unions at all in
> >> build_reconstructed_reference but it can work, to an extent, if we
> >> make sure that we start the search only outside the outermost union,
> >> which is what the patch does (and the extra testcase verifies).
> >>
> >> Bootstrapped and tested on x86_64-linux. OK for trunk and then for
> >> release branches?
> >
> > OK, but I'm wondering if the same problem can arise when the
> > handled_component_p includes VIEW_CONVERTs or BIT_FIELD_REFs
> > both of which may pun to a type already seen in a more inner
> > referece.
>
> SRA already refuses to operate at all on any anything that is accessed
> with a reference where a V_C_E is not the outermost handled_component.
> Outermost V_C_Es are skipped and the pass works with the underlying
> expr. BIT_FIELD_REFs have to be outermost and they are treated
> similarly. So that should be safe.
OK.
> > Thus, is the actual problem that build_reconstructed_reference
> > searches for the outermost match of the type rather than the
> > innermost match? So should it search inner-to-outer instead
> > (or for the last match in the current way of searching?)
>
> I don't think so. In the testcase it found a match where there should
> have been none (meaning a crude MEM_REF should be created), any
> certainly correct match must be outside of a union COMPONENT_REF and
> there should never be more than one.
OK, not having looked at the testcase I suspected there would be
two matches. I couldn't come up with a case where a single union
can confuse things enough ..
The patch is OK.
Richard.
> Thanks,
>
> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >> Thanks,
> >>
> >> Martin
> >>
> >>
> >> gcc/ChangeLog:
> >>
> >> 2022-07-01 Martin Jambor <mjambor@suse.cz>
> >>
> >> PR tree-optimization/105860
> >> * tree-sra.cc (build_reconstructed_reference): Start expr
> >> traversal only just below the outermost union.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2022-07-01 Martin Jambor <mjambor@suse.cz>
> >>
> >> PR tree-optimization/105860
> >> * gcc.dg/tree-ssa/alias-access-path-13.c: New test.
> >> * gcc.dg/tree-ssa/pr105860.c: Likewise.
> >> ---
> >> .../gcc.dg/tree-ssa/alias-access-path-13.c | 31 +++++++++
> >> gcc/testsuite/gcc.dg/tree-ssa/pr105860.c | 63 +++++++++++++++++++
> >> gcc/tree-sra.cc | 13 +++-
> >> 3 files changed, 106 insertions(+), 1 deletion(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
> >> new file mode 100644
> >> index 00000000000..e502a97bc75
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-access-path-13.c
> >> @@ -0,0 +1,31 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fdump-tree-fre1" } */
> >> +
> >> +struct inn
> >> +{
> >> + int val;
> >> +};
> >> +
> >> +union foo
> >> +{
> >> + struct inn inn;
> >> + long int baz;
> >> +} *fooptr;
> >> +
> >> +struct bar
> >> +{
> >> + union foo foo;
> >> + int val2;
> >> +} *barptr;
> >> +
> >> +int
> >> +test ()
> >> +{
> >> + union foo foo;
> >> + foo.inn.val = 0;
> >> + barptr->val2 = 123;
> >> + *fooptr = foo;
> >> + return barptr->val2;
> >> +}
> >> +
> >> +/* { dg-final { scan-tree-dump-times "return 123" 1 "fre1"} } */
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
> >> new file mode 100644
> >> index 00000000000..77bcb4a6739
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr105860.c
> >> @@ -0,0 +1,63 @@
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O1" } */
> >> +
> >> +struct S1 {
> >> + unsigned int _0;
> >> + unsigned int _1;
> >> +} ;
> >> +struct S2 {
> >> + struct S1 _s1;
> >> + unsigned long _x2;
> >> +} ;
> >> +
> >> +struct ufld_type1 {
> >> + unsigned int _u1t;
> >> + struct S2 _s2;
> >> +} ;
> >> +
> >> +struct ufld_type2 {
> >> + unsigned int _u2t;
> >> + struct S1 _s1;
> >> +} ;
> >> +struct parm_type {
> >> + union {
> >> + struct ufld_type1 var_1;
> >> + struct ufld_type2 var_2;
> >> + } U;
> >> +};
> >> +
> >> +struct parm_type bad_function( struct parm_type arg0 )
> >> +{
> >> + struct parm_type rv;
> >> + struct S2 var4;
> >> + switch( arg0.U.var_2._u2t ) {
> >> + case 4294967041:
> >> + var4._s1 = arg0.U.var_1._s2._s1;
> >> + rv.U.var_1._u1t = 4294967041;
> >> + rv.U.var_1._s2 = var4;
> >> + break;
> >> + case 4294967043:
> >> + rv.U.var_2._u2t = 4294967043;
> >> + rv.U.var_2._s1 = arg0.U.var_2._s1;
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + return rv;
> >> +}
> >> +
> >> +int main() {
> >> + struct parm_type val;
> >> + struct parm_type out;
> >> + val.U.var_2._u2t = 4294967043;
> >> + val.U.var_2._s1._0 = 0x01010101;
> >> + val.U.var_2._s1._1 = 0x02020202;
> >> + out = bad_function(val);
> >> + if (val.U.var_2._u2t != 4294967043)
> >> + __builtin_abort ();
> >> + if (out.U.var_2._s1._0 != 0x01010101)
> >> + __builtin_abort ();
> >> + if (val.U.var_2._s1._1 != 0x02020202 )
> >> + __builtin_abort ();
> >> + return 0;
> >> +}
> >> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> >> index 081c51b58a4..099e8dbe873 100644
> >> --- a/gcc/tree-sra.cc
> >> +++ b/gcc/tree-sra.cc
> >> @@ -1667,7 +1667,18 @@ build_ref_for_offset (location_t loc, tree base, poly_int64 offset,
> >> static tree
> >> build_reconstructed_reference (location_t, tree base, struct access *model)
> >> {
> >> - tree expr = model->expr, prev_expr = NULL;
> >> + tree expr = model->expr;
> >> + /* We have to make sure to start just below the outermost union. */
> >> + tree start_expr = expr;
> >> + while (handled_component_p (expr))
> >> + {
> >> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (expr, 0))) == UNION_TYPE)
> >> + start_expr = expr;
> >> + expr = TREE_OPERAND (expr, 0);
> >> + }
> >> +
> >> + expr = start_expr;
> >> + tree prev_expr = NULL_TREE;
> >> while (!types_compatible_p (TREE_TYPE (expr), TREE_TYPE (base)))
> >> {
> >> if (!handled_component_p (expr))
> >>
> >
> > --
> > Richard Biener <rguenther@suse.de>
> > SUSE Software Solutions Germany GmbH, Frankenstra
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstra
prev parent reply other threads:[~2022-07-05 7:34 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-01 20:48 Martin Jambor
2022-07-04 6:15 ` Richard Biener
2022-07-04 14:48 ` Martin Jambor
2022-07-05 7:34 ` Richard Biener [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nycvar.YFH.7.77.849.2207050731070.14950@jbgna.fhfr.qr \
--to=rguenther@suse.de \
--cc=gcc-patches@gcc.gnu.org \
--cc=mjambor@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).