| Message ID | 7c782ea8-b832-3e1b-ce83-cca08866f4c8@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 70621 ("[6/7/8 Regression] ICE on invalid code at -O1 and above on x86_64-linux-gnu in record_reference, at cgraphbuild.c:64") | expand |
OK. On Tue, Aug 29, 2017 at 11:33 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote: > Hi, > > in this error recovery regression, we ICE only when optimizing, while > building the cgraph. Avoiding the reported problem seems easy, just check > the return value of duplicate_decls in start_decl and immediately return > back error_mark_node. However, while working on the issue, I noticed > something slightly more interesting, IMO: we have, after the relevant > duplicate_decls call: > > - if (decl_spec_seq_has_spec_p (declspecs, ds_constexpr) > - && !DECL_DECLARED_CONSTEXPR_P (field)) > - error ("%qD declared %<constexpr%> outside its class", > field); > > which I propose to remove. In fact - something I didn't really know - for > well formed user code, duplicate_decls, near the end, does some memcpys > which mean that its second argument (would be 'field' in the start_decl > section we are looking at) is adjusted to have a DECL_DECLARED_CONSTEXPR_P > consistent with its first argument. That's of course because we want to > accept snippets like: > > struct A > { > static const int x; > }; > > constexpr int A::x = 0; > > In turn that means the error above is issued only when something went wrong > in duplicate_decls in the first place. Thus the above error seems at least > verbose. However, here in start_decl we are only handling VAR_P (decl), thus > I don't think the message above even makes sense and could be misleading, > given snippets like the above. Therefore, I propose to remove the diagnostic > entirely, which overall also simplifies a patch dealing with c++/70621. The > below passes testing as-is on x86_64-linux. > > Thanks, Paolo. > > //////////////////////// >
Index: cp/decl.c =================================================================== --- cp/decl.c (revision 251375) +++ cp/decl.c (working copy) @@ -5023,11 +5023,12 @@ start_decl (const cp_declarator *declarator, about this situation, and so we check here. */ if (initialized && DECL_INITIALIZED_IN_CLASS_P (field)) error ("duplicate initialization of %qD", decl); - if (duplicate_decls (decl, field, /*newdecl_is_friend=*/false)) + field = duplicate_decls (decl, field, + /*newdecl_is_friend=*/false); + if (field == error_mark_node) + return error_mark_node; + else if (field) decl = field; - if (decl_spec_seq_has_spec_p (declspecs, ds_constexpr) - && !DECL_DECLARED_CONSTEXPR_P (field)) - error ("%qD declared %<constexpr%> outside its class", field); } } else Index: testsuite/g++.dg/torture/pr70621.C =================================================================== --- testsuite/g++.dg/torture/pr70621.C (revision 0) +++ testsuite/g++.dg/torture/pr70621.C (working copy) @@ -0,0 +1,13 @@ +float foo(); + +struct A +{ + static float x; // { dg-message "previous declaration" } +}; + +double A::x = foo(); // { dg-error "conflicting declaration" } + +void bar() +{ + A::x = 0; +}
Hi, in this error recovery regression, we ICE only when optimizing, while building the cgraph. Avoiding the reported problem seems easy, just check the return value of duplicate_decls in start_decl and immediately return back error_mark_node. However, while working on the issue, I noticed something slightly more interesting, IMO: we have, after the relevant duplicate_decls call: - if (decl_spec_seq_has_spec_p (declspecs, ds_constexpr) - && !DECL_DECLARED_CONSTEXPR_P (field)) - error ("%qD declared %<constexpr%> outside its class", field); which I propose to remove. In fact - something I didn't really know - for well formed user code, duplicate_decls, near the end, does some memcpys which mean that its second argument (would be 'field' in the start_decl section we are looking at) is adjusted to have a DECL_DECLARED_CONSTEXPR_P consistent with its first argument. That's of course because we want to accept snippets like: struct A { static const int x; }; constexpr int A::x = 0; In turn that means the error above is issued only when something went wrong in duplicate_decls in the first place. Thus the above error seems at least verbose. However, here in start_decl we are only handling VAR_P (decl), thus I don't think the message above even makes sense and could be misleading, given snippets like the above. Therefore, I propose to remove the diagnostic entirely, which overall also simplifies a patch dealing with c++/70621. The below passes testing as-is on x86_64-linux. Thanks, Paolo. //////////////////////// /cp 2017-29-08 Paolo Carlini <paolo.carlini@oracle.com> PR c++/70621 * decl.c (start_decl): Early return error_mark_node if duplicate_decls returns it; avoid misleading error message. /testsuite 2017-29-08 Paolo Carlini <paolo.carlini@oracle.com> PR c++/70621 * g++.dg/torture/pr70621.C: New.