[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #29 from CVS Commits --- The master branch has been updated by Jakub Jelinek : https://gcc.gnu.org/g:eb03e424598d30fed68801af6d6ef6236d32e32e commit r12-8196-geb03e424598d30fed68801af6d6ef6236d32e32e Author: Jakub Jelinek Date: Tue Apr 19 18:27:41 2022 +0200 c++: Fix up CONSTRUCTOR_PLACEHOLDER_BOUNDARY handling [PR105256] The CONSTRUCTOR_PLACEHOLDER_BOUNDARY bit is supposed to separate PLACEHOLDER_EXPRs that should be replaced by one object or subobjects of it (variable, TARGET_EXPR slot, ...) from other PLACEHOLDER_EXPRs that should be replaced by different objects or subobjects. The bit is set when finding PLACEHOLDER_EXPRs inside of a CONSTRUCTOR, not looking into nested CONSTRUCTOR_PLACEHOLDER_BOUNDARY ctors, and we prevent elision of TARGET_EXPRs (through TARGET_EXPR_NO_ELIDE) whose initializer is a CONSTRUCTOR_PLACEHOLDER_BOUNDARY ctor. The following testcase ICEs though, we don't replace the placeholders in there at all, because CONSTRUCTOR_PLACEHOLDER_BOUNDARY isn't set on the TARGET_EXPR_INITIAL ctor, but on a ctor nested in such a ctor. replace_placeholders should be run on the whole TARGET_EXPR slot. So, the following patch fixes it by moving the CONSTRUCTOR_PLACEHOLDER_BOUNDARY bit from nested CONSTRUCTORs to the CONSTRUCTOR containing those (but only if it is closely nested, if there is some other tree sandwiched in between, it doesn't do it). 2022-04-19 Jakub Jelinek PR c++/105256 * typeck2.cc (process_init_constructor_array, process_init_constructor_record, process_init_constructor_union): Move CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag from CONSTRUCTOR elements to the containing CONSTRUCTOR. * g++.dg/cpp0x/pr105256.C: New test.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
Richard Biener changed:
What|Removed |Added
Blocks||102990
--- Comment #28 from Richard Biener ---
(In reply to Martin Liška from comment #27)
> So it started on gcc-11 branch with r11-9711-g6ba2a7e7474135.
For
struct S {
struct Prefs {
struct P {
int i = 42;
int j = i;
} p;
void Load();
};
};
void S::Prefs::Load() {
*this = {};
};
that is, which probably means that the assumtion CTORs are already folded
is false and with the PR102990 change we no longer constant fold the
initializer. Before:
;; Function void S::Prefs::Load() (null)
;; enabled by -tree-original
<) >;
After:
;; Function void S::Prefs::Load() (null)
;; enabled by -tree-original
<)->i}}>) >;
which explains why the PR102990 change makes a difference here (and why it
is a recent regression for compiling Firefox on the branch).
Referenced Bugs:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102990
[Bug 102990] [9/10 Regression] ICE in tsubst_copy_and_build with NSDMI and
double to int conversion
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #27 from Martin Liška --- So it started on gcc-11 branch with r11-9711-g6ba2a7e7474135.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #26 from Richard Biener --- What's odd is that we could successfully build 99.0.1 with r11-9662-g6a1150d1524aed but it now fails this reported way with the release candidate.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #25 from Chris Clayton --- I went ahead and patched gcc-11-0220409 and with the resultant compiler have had two successful builds of firefox-99. I then reverted to the unpatched gcc and a build of firefox-99 failed with the same ICE that I reported in comment 1.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #24 from Chris Clayton --- I see the patch is for gcc-12. As I said in comment, I don't get the ICE with the latest gcc-12 snapshot, but is it worth me applying the patch to gcc-11 (with which I do get the ICE) and testing a build with that?
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #23 from Jakub Jelinek --- Created attachment 52811 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52811&action=edit gcc12-pr105256.patch Untested patch to do that.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
--- Comment #22 from Jakub Jelinek ---
So, wonder if for a CONSTRUCTOR containing elements which are CONSTRUCTORs with
CONSTRUCTOR_PLACEHOLDER_BOUNDARY set we shouldn't move the
CONSTRUCTOR_PLACEHOLDER_BOUNDARY flag to the outer CONSTRUCTOR (if not set
there already).
The reason why the flag was introduced was to catch e.g. those
X x { 1, bar (X{2}).n }; etc. cases where the X{2} contains an unrelated
PLACEHOLDER_EXPR to the outer one.
But when we have directly nested CONSTRUCTORs, we can replace_placeholders all
of those PLACEHOLDER_EXPRs from in there, the object will be simply some member
of the var or TARGET_EXPR slot etc.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #21 from Jakub Jelinek --- Ah, we have: case TARGET_EXPR: if (TARGET_EXPR_INITIAL (stmt) && TREE_CODE (TARGET_EXPR_INITIAL (stmt)) == CONSTRUCTOR && CONSTRUCTOR_PLACEHOLDER_BOUNDARY (TARGET_EXPR_INITIAL (stmt))) TARGET_EXPR_NO_ELIDE (stmt) = 1; break; but that doesn't trigger in this case because the TARGET_EXPR's CONSTRUCTOR is not CONSTRUCTOR_PLACEHOLDER_BOUNDARY, instead it is a CONSTRUCTOR that contains a CONSTRUCTOR_PLACEHOLDER_BOUNDARY CONSTRUCTOR as one of its elements.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
--- Comment #20 from Marek Polacek ---
(In reply to Jakub Jelinek from comment #16)
> Don't both of those tests have UB (sure, we shouldn't ICE), using
> uninitialized non-static data member?
NSDMIs are parsed at the end of the class, but I think I should have switched
the order of the initialization:
struct S {
struct Prefs {
struct P {
int i = 42;
int j = i;
} p;
void Load();
};
};
void S::Prefs::Load() {
*this = {};
};
but your test is good too.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
--- Comment #19 from Jakub Jelinek ---
The TARGET_EXPR is elided in gimplify_modify_expr_rhs:
case TARGET_EXPR:
{
/* If we are initializing something from a TARGET_EXPR, strip the
TARGET_EXPR and initialize it directly, if possible. This can't
be done if the initializer is void, since that implies that the
temporary is set in some non-trivial way.
??? What about code that pulls out the temp and uses it
elsewhere? I think that such code never uses the TARGET_EXPR as
an initializer. If I'm wrong, we'll die because the temp won't
have any RTL. In that case, I guess we'll need to replace
references somehow. */
tree init = TARGET_EXPR_INITIAL (*from_p);
if (init
&& (TREE_CODE (*expr_p) != MODIFY_EXPR
|| !TARGET_EXPR_NO_ELIDE (*from_p))
&& !VOID_TYPE_P (TREE_TYPE (init)))
{
*from_p = init;
ret = GS_OK;
changed = true;
}
}
break;
Though, can the TARGET_EXPR be elided if there are PLACEHOLDER_EXPRs in it?
If not, something should probably set TARGET_EXPR_NO_ELIDE somewhere.
If yes, perhaps we need to replace_placeholders perhaps through a language hook
at the
above point. But it is unclear to what it should be replaced...
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
--- Comment #18 from Jakub Jelinek ---
The rationale for introduction of CONSTRUCTOR_PLACEHOLDER_BOUNDARY was given in
the
https://gcc.gnu.org/legacy-ml/gcc-patches/2018-03/threads.html#00681
thread, not all PLACEHOLDER_EXPRs nested in an expression should be replaced by
the same object, if there are e.g. nested TARGET_EXPRs with PLACEHOLDER_EXPRs
in their initializers, those nested PLACEHOLDER_EXPRs should be replaced by the
TARGET_EXPR's slot rather than the outermost expression.
Before the r8-7334-g570f86f94eca76fbf, the PLACEHOLDER_EXPR was replaced from
build_over_call, but that has removed. The expectation was that we'd always
have some object (var being constructed, TARGET_EXPR slot, etc. and we'd
replace_placeholders when gimplifying INIT_EXPR for it).
But on:
int bar (int &);
struct S {
struct T {
struct U {
int i = bar (i);
} u;
};
};
void
foo (S::T *p)
{
*p = {};
};
cp_gimplify_init_expr is never called, gimplify_target_expr neither, because we
ICE earlier than that.
While we have there a TARGET_EXPR when we start gimplification:
*NON_LVALUE_EXPR = *(struct T &) &TARGET_EXPR )->i)}}>, seems something has elided it.
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 --- Comment #17 from Chris Clayton --- Created attachment 52810 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52810&action=edit Compiler commands Finally got them by running running "ps ax" in a while true loop, grepping the output for the file name and writing any matches to a file. There must be an easier way!
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
--- Comment #16 from Jakub Jelinek ---
Don't both of those tests have UB (sure, we shouldn't ICE), using uninitialized
non-static data member?
Perhaps:
int
bar (int &)
{
return 1;
}
struct S {
struct T {
struct U {
int i = bar (i);
} p;
void foo ();
};
};
void
S::T::foo ()
{
*this = {};
};
is better?
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256
Marek Polacek changed:
What|Removed |Added
Keywords||ice-on-valid-code
--- Comment #15 from Marek Polacek ---
Better test:
struct S {
struct Prefs {
struct {
int i = j;
int j = 42;
} p;
void Load();
};
};
void S::Prefs::Load() {
*this = {};
};
[Bug c++/105256] [9/10/11/12 Regression] ICE compiling firefox-99
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105256 Marek Polacek changed: What|Removed |Added Target Milestone|--- |9.5 Summary|ICE compiling firefox-99|[9/10/11/12 Regression] ICE ||compiling firefox-99 Status|WAITING |NEW Priority|P3 |P2 --- Comment #14 from Marek Polacek --- The testcase in the previous comment started to ICE with r258593: commit 570f86f94eca76fbfab919dcbfe639a5ba69f20e Author: Jakub Jelinek Date: Fri Mar 16 13:46:12 2018 +0100 re PR c++/79937 (ICE in replace_placeholders_r) So confirmed. I'm not sure this is the same ICE Chris saw though...
