From: Patrick Palka <ppalka@redhat.com>
To: Marek Polacek <polacek@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Jason Merrill <jason@redhat.com>,
Jakub Jelinek <jakub@redhat.com>,
Patrick Palka <ppalka@redhat.com>
Subject: Re: [PATCH] c++: ICE with temporary of class type in DMI [PR100252]
Date: Wed, 27 Apr 2022 11:00:46 -0400 (EDT) [thread overview]
Message-ID: <8e0bc483-7fba-225c-807b-dbe89efceb5e@idea> (raw)
In-Reply-To: <20220426230226.677300-1-polacek@redhat.com>
On Tue, 26 Apr 2022, Marek Polacek wrote:
> Consider
>
> struct A {
> int x;
> int y = x;
> };
>
> struct B {
> int x = 0;
> int y = A{x}.y; // #1
> };
>
> where for #1 we end up with
>
> {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
>
> that is, two PLACEHOLDER_EXPRs for different types on the same level in
> a {}. This crashes because our CONSTRUCTOR_PLACEHOLDER_BOUNDARY mechanism to
> avoid replacing unrelated PLACEHOLDER_EXPRs cannot deal with it.
>
> Here's why we wound up with those PLACEHOLDER_EXPRs: When we're performing
> cp_parser_late_parsing_nsdmi for "int y = A{x}.y;" we use finish_compound_literal
> on type=A, compound_literal={((struct B *) this)->x}. When digesting this
> initializer, we call get_nsdmi which creates a PLACEHOLDER_EXPR for A -- we don't
> have any object to refer to yet. After digesting, we have
>
> {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
>
> and since we've created a PLACEHOLDER_EXPR inside it, we marked the whole ctor
> CONSTRUCTOR_PLACEHOLDER_BOUNDARY. f_c_l creates a TARGET_EXPR and returns
>
> TARGET_EXPR <D.2384, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>
>
> Then we get to
>
> B b = {};
>
> and call store_init_value, which digest the {}, which produces
>
> {.x=NON_LVALUE_EXPR <0>, .y=(TARGET_EXPR <D.2395, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y}
>
> The call to replace_placeholders in store_init_value will not do anything:
> we've marked the inner { } CONSTRUCTOR_PLACEHOLDER_BOUNDARY, and it's only
> a sub-expression, so replace_placeholders does nothing, so the <P_E struct B>
> stays even though now is the perfect time to replace it because we have an
> object for it: 'b'.
>
> Later, in cp_gimplify_init_expr the *expr_p is
>
> D.2395 = {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}
>
> where D.2395 is of type A, but we crash because we hit <P_E struct B>, which
> has a different type.
>
> My idea was to replace <P_E struct A> with D.2384 in f_c_l after creating the
> TARGET_EXPR because that means we have an object we can refer to. Then clear
> CONSTRUCTOR_PLACEHOLDER_BOUNDARY because we no longer have a PLACEHOLDER_EXPR
> in the {}. Then store_init_value will be able to replace <P_E struct B> with
> 'b', and we should be good to go.
Makes sense to me. It seems all was well until break_out_target_exprs,
called from get_nsdmi for B::y, replaced the 'this' in the initializer
(TARGET_EXPR <D.2131, {.x=((struct B *) this)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y;
with a PLACEHOLDER_EXPR;
(TARGET_EXPR <D.2142, {.x=(&<PLACEHOLDER_EXPR struct B>)->x, .y=(&<PLACEHOLDER_EXPR struct A>)->x}>).y;
This seems to be the wrong thing to do when the 'this' appears inside a
CONSTRUCTOR_PLACEHOLDER_BOUNDARY constructor because the new
PLACEHOLDER_EXPR then can't be resolved correctly.
So in light of this I wonder if we should instead perform this handling
you added to finish_compound_literal in break_out_target_exprs /
bot_manip instead?
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/11.4?
>
> PR c++/100252
>
> gcc/cp/ChangeLog:
>
> * semantics.cc (finish_compound_literal): replace_placeholders after
> creating the TARGET_EXPR.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/cpp1y/nsdmi-aggr14.C: New test.
> ---
> gcc/cp/semantics.cc | 31 +++++++++++++++
> gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C | 46 +++++++++++++++++++++++
> 2 files changed, 77 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
>
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index ab48f11c9be..770369458bb 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -3296,6 +3296,37 @@ finish_compound_literal (tree type, tree compound_literal,
> if (TREE_CODE (compound_literal) == CONSTRUCTOR)
> TREE_HAS_CONSTRUCTOR (compound_literal) = false;
> compound_literal = get_target_expr_sfinae (compound_literal, complain);
> + /* We may have A{} in a NSDMI. */
> + if (parsing_nsdmi ())
> + {
> + /* Digesting the {} could have introduced a PLACEHOLDER_EXPR
> + referring to A. Now that we've built up a TARGET_EXPR, we
> + have an object we can refer to. The reason we bother doing
> + this here is for code like
> +
> + struct A {
> + int x;
> + int y = x;
> + };
> +
> + struct B {
> + int x = 0;
> + int y = A{x}.y; // #1
> + };
> +
> + where in #1 we don't want to end up with two PLACEHOLDER_EXPRs
> + for different types on the same level in a {} as in 100252. */
> + tree init = TARGET_EXPR_INITIAL (compound_literal);
> + if (TREE_CODE (init) == CONSTRUCTOR
> + && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init))
> + {
> + tree obj = TARGET_EXPR_SLOT (compound_literal);
> + replace_placeholders (compound_literal, obj);
> + /* We should have dealt with the PLACEHOLDER_EXPRs. */
> + CONSTRUCTOR_PLACEHOLDER_BOUNDARY (init) = false;
> + gcc_checking_assert (!find_placeholders (init));
> + }
> + }
> }
> else
> /* For e.g. int{42} just make sure it's a prvalue. */
> diff --git a/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
> new file mode 100644
> index 00000000000..7d508f52b48
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp1y/nsdmi-aggr14.C
> @@ -0,0 +1,46 @@
> +// PR c++/100252
> +// { dg-do run { target c++14 } }
> +
> +#define SA(X) static_assert ((X),#X)
> +
> +struct A {
> + int x;
> + int y = x;
> +};
> +
> +struct B {
> + int x = 0;
> + int y = A{x}.y;
> +};
> +
> +constexpr B csb1 = { };
> +SA(csb1.x == 0 && csb1.y == csb1.x);
> +constexpr B csb2 = { 1 };
> +SA(csb2.x == 1 && csb2.y == csb2.x);
> +constexpr B csb3 = { 1, 2 };
> +SA(csb3.x == 1 && csb3.y == 2);
> +
> +B sb1 = { };
> +B sb2 = { 1 };
> +B sb3 = { 1, 2};
> +
> +int
> +main ()
> +{
> + if (sb1.x != 0 || sb1.x != sb1.y)
> + __builtin_abort();
> + if (sb2.x != 1 || sb2.x != sb2.y)
> + __builtin_abort();
> + if (sb3.x != 1 || sb3.y != 2)
> + __builtin_abort();
> +
> + B b1 = { };
> + B b2 = { 1 };
> + B b3 = { 1, 2};
> + if (b1.x != 0 || b1.x != b1.y)
> + __builtin_abort();
> + if (b2.x != 1 || b2.x != b2.y)
> + __builtin_abort();
> + if (b3.x != 1 || b3.y != 2)
> + __builtin_abort();
> +}
>
> base-commit: 9ace5d4dab2ab39072b0f07089621a823580f27c
> --
> 2.35.1
>
>
next prev parent reply other threads:[~2022-04-27 15:00 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-26 23:02 Marek Polacek
2022-04-27 15:00 ` Patrick Palka [this message]
2022-04-27 20:30 ` [PATCH v2] " Marek Polacek
2022-04-28 14:12 ` Patrick Palka
2022-04-28 14:39 ` Marek Polacek
2022-05-03 20:59 ` Jason Merrill
2022-05-07 19:11 ` [PATCH v3] " Marek Polacek
2022-05-07 22:02 ` Jason Merrill
2022-05-13 23:41 ` [PATCH v4] " Marek Polacek
2022-05-15 3:13 ` Jason Merrill
2022-05-16 15:36 ` [PATCH v5] " Marek Polacek
2022-05-24 11:40 ` Marek Polacek
2022-05-24 12:36 ` Jason Merrill
2022-05-24 13:55 ` Marek Polacek
2022-05-24 20:01 ` Jason Merrill
2022-05-24 20:21 ` Marek Polacek
2022-05-25 12:22 ` Jason Merrill
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=8e0bc483-7fba-225c-807b-dbe89efceb5e@idea \
--to=ppalka@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=jason@redhat.com \
--cc=polacek@redhat.com \
/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).