| Message ID | 20191011082001.GR15914@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix constexpr-dtor3.C FAIL on arm | expand |
On 10/11/19 4:20 AM, Jakub Jelinek wrote: > On Thu, Oct 10, 2019 at 05:38:21PM -0400, Jason Merrill wrote: >>> FAIL: g++.dg/cpp2a/constexpr-dtor3.C -std=c++2a (test for excess errors) >>> Excess errors: >>> /gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C:152:12: in 'constexpr' >>> expansion of '(& w13)->W7::~W7()' >>> >> >> This also seems unrelated, but I'll take a look. > > This is solely a location_t difference, comparing x86_64 and arm output > constexpr-dtor3.C: At global scope: > constexpr-dtor3.C:156:23: in ‘constexpr’ expansion of ‘f4()’ > -constexpr-dtor3.C:156:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > +constexpr-dtor3.C:152:12: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > constexpr-dtor3.C:88:34: error: inline assembly is not a constant expression > 88 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } > | ^~~ > constexpr-dtor3.C:88:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a > > constexpr int > f4 () > { > W7 w13 = 5; > return 0; > } > > constexpr int x4 = f4 (); // { dg-message "in 'constexpr' expansion of" } > > Line 152 is the W7 w13 = 5; line, line 156 the x4 = f4 () line. > From warning POV, the arm locations are nicer, but see below. > > The difference is in cxx_maybe_build_cleanup: > > /* build_delete sets the location of the destructor call to the > current location, even though the destructor is going to be > called later, at the end of the current scope. This can lead to > a "jumpy" behavior for users of debuggers when they step around > the end of the block. So let's unset the location of the > destructor call instead. */ > protected_set_expr_location (cleanup, UNKNOWN_LOCATION); > > On x86_64 and most other targets, cleanup here (if non-NULL) is the > CALL_EXPR, as destructor return type is void, but on arm, as the dtor return > type is some pointer, the CALL_EXPR is wrapped into a NOP_EXPR to void. > protected_set_expr_location then on x86_64 clears the CALL_EXPR location, > but on arm only NOP_EXPR location. > > The following patch (totally untested) should fix that. > > For the warning location, perhaps we could special case destructor calls > in push_cx_call_context (to offset the intentional clearing of location for > debugging purposes), if they don't have location set, don't use > input_location for them, but try to pick DECL_SOURCE_LOCATION for the > variable being destructed? Expanding the CLEANUP_EXPR of a CLEANUP_STMT could use the EXPR_LOCATION of the CLEANUP_STMT. Or the EXPR_LOCATION of *jump_target, if suitable. Jason
On Fri, Oct 11, 2019 at 04:14:16PM -0400, Jason Merrill wrote: > > On x86_64 and most other targets, cleanup here (if non-NULL) is the > > CALL_EXPR, as destructor return type is void, but on arm, as the dtor return > > type is some pointer, the CALL_EXPR is wrapped into a NOP_EXPR to void. > > protected_set_expr_location then on x86_64 clears the CALL_EXPR location, > > but on arm only NOP_EXPR location. > > > > The following patch (totally untested) should fix that. > > > > For the warning location, perhaps we could special case destructor calls > > in push_cx_call_context (to offset the intentional clearing of location for > > debugging purposes), if they don't have location set, don't use > > input_location for them, but try to pick DECL_SOURCE_LOCATION for the > > variable being destructed? > > Expanding the CLEANUP_EXPR of a CLEANUP_STMT could use the EXPR_LOCATION of > the CLEANUP_STMT. Or the EXPR_LOCATION of *jump_target, if suitable. The already previously posted patch (now attached as first) has now been bootstrapped/regtested on x86_64-linux and i686-linux, and regardless if we improve the location or not should fix the arm vs. the rest of the world difference. Is that ok for trunk? As for CLEANUP_STMT, I've tried it (the second patch), but it didn't change anything, the diagnostics was still constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ constexpr-dtor3.C:16:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } | ^~~ constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a as without that change. I've also tried the third patch, tested so far with check-c++-all, which changes that to constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ constexpr-dtor3.C:12:6: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } | ^~~ constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a Jakub 2019-10-16 Jakub Jelinek <jakub@redhat.com> * decl.c (cxx_maybe_build_cleanup): When clearing location of cleanup, if cleanup is a nop, clear location of its operand too. --- gcc/cp/decl.c.jj 2019-10-10 01:33:38.154943945 +0200 +++ gcc/cp/decl.c 2019-10-11 10:09:24.321277942 +0200 @@ -16864,6 +16864,8 @@ cxx_maybe_build_cleanup (tree decl, tsub the end of the block. So let's unset the location of the destructor call instead. */ protected_set_expr_location (cleanup, UNKNOWN_LOCATION); + if (cleanup && CONVERT_EXPR_P (cleanup)) + protected_set_expr_location (TREE_OPERAND (cleanup, 0), UNKNOWN_LOCATION); if (cleanup && DECL_P (decl) 2019-10-16 Jakub Jelinek <jakub@redhat.com> * constexpr.c (cxx_eval_constant_expression) <case CLEANUP_STMT>: Temporarily change input_location to CLEANUP_STMT location. --- gcc/cp/constexpr.c.jj 2019-10-10 01:33:38.185943480 +0200 +++ gcc/cp/constexpr.c 2019-10-11 22:54:32.628051700 +0200 @@ -4980,9 +4980,13 @@ cxx_eval_constant_expression (const cons case CLEANUP_STMT: { tree initial_jump_target = jump_target ? *jump_target : NULL_TREE; + location_t loc = input_location; + if (EXPR_HAS_LOCATION (t)) + input_location = EXPR_LOCATION (t); r = cxx_eval_constant_expression (ctx, CLEANUP_BODY (t), lval, non_constant_p, overflow_p, jump_target); + input_location = loc; if (!CLEANUP_EH_ONLY (t) && !*non_constant_p) /* Also evaluate the cleanup. If we weren't skipping at the start of the CLEANUP_BODY, change jump_target temporarily 2019-10-16 Jakub Jelinek <jakub@redhat.com> * decl.c (cxx_maybe_build_cleanup): When clearing location of cleanup, if cleanup is a nop, clear location of its operand too. * constexpr.c (push_cx_call_context): For calls to destructors, use DECL_SOURCE_LOCATION of destructed variable in preference to input_location. * g++.dg/cpp2a/constexpr-dtor3.C: Expect in 'constexpr' expansion of message on the line with variable declaration. --- gcc/cp/decl.c.jj 2019-10-16 09:30:57.490109872 +0200 +++ gcc/cp/decl.c 2019-10-16 17:45:48.647529567 +0200 @@ -16864,6 +16864,8 @@ cxx_maybe_build_cleanup (tree decl, tsub the end of the block. So let's unset the location of the destructor call instead. */ protected_set_expr_location (cleanup, UNKNOWN_LOCATION); + if (cleanup && CONVERT_EXPR_P (cleanup)) + protected_set_expr_location (TREE_OPERAND (cleanup, 0), UNKNOWN_LOCATION); if (cleanup && DECL_P (decl) --- gcc/cp/constexpr.c.jj 2019-10-16 17:35:37.848752468 +0200 +++ gcc/cp/constexpr.c 2019-10-16 18:07:42.472688066 +0200 @@ -1457,7 +1457,26 @@ push_cx_call_context (tree call) { ++call_stack_tick; if (!EXPR_HAS_LOCATION (call)) - SET_EXPR_LOCATION (call, input_location); + { + tree fun = get_function_named_in_call (call); + location_t loc = input_location; + /* Calls to destructors have UNKNOWN_LOCATION, in order to avoid + problems debugging at the end of scopes. For diagnostic purposes, + try to use location of the variable that needs destruction. */ + if (fun && DECL_DESTRUCTOR_P (fun)) + { + tree arg = CALL_EXPR_ARG (call, 0); + STRIP_NOPS (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + { + tree var = get_base_address (TREE_OPERAND (arg, 0)); + if (DECL_P (var) + && DECL_SOURCE_LOCATION (var) != UNKNOWN_LOCATION) + loc = DECL_SOURCE_LOCATION (var); + } + } + SET_EXPR_LOCATION (call, loc); + } call_stack.safe_push (call); int len = call_stack.length (); if (len > max_constexpr_depth) --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C.jj 2019-10-05 09:36:39.250674497 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C 2019-10-16 18:10:48.603877004 +0200 @@ -149,7 +149,7 @@ constexpr int x3 = f3 (); constexpr int f4 () { - W7 w13 = 5; + W7 w13 = 5; // { dg-message "in 'constexpr' expansion of" } return 0; }
On 10/16/19 12:27 PM, Jakub Jelinek wrote: > On Fri, Oct 11, 2019 at 04:14:16PM -0400, Jason Merrill wrote: >>> On x86_64 and most other targets, cleanup here (if non-NULL) is the >>> CALL_EXPR, as destructor return type is void, but on arm, as the dtor return >>> type is some pointer, the CALL_EXPR is wrapped into a NOP_EXPR to void. >>> protected_set_expr_location then on x86_64 clears the CALL_EXPR location, >>> but on arm only NOP_EXPR location. >>> >>> The following patch (totally untested) should fix that. >>> >>> For the warning location, perhaps we could special case destructor calls >>> in push_cx_call_context (to offset the intentional clearing of location for >>> debugging purposes), if they don't have location set, don't use >>> input_location for them, but try to pick DECL_SOURCE_LOCATION for the >>> variable being destructed? >> >> Expanding the CLEANUP_EXPR of a CLEANUP_STMT could use the EXPR_LOCATION of >> the CLEANUP_STMT. Or the EXPR_LOCATION of *jump_target, if suitable. > > The already previously posted patch (now attached as first) has now been > bootstrapped/regtested on x86_64-linux and i686-linux, and regardless if we > improve the location or not should fix the arm vs. the rest of the world > difference. Is that ok for trunk? OK. > As for CLEANUP_STMT, I've tried it (the second patch), but it didn't change > anything, the diagnostics was still > constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ > constexpr-dtor3.C:16:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression > 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } > | ^~~ > constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a > as without that change. That's because the patch changes EXPR_LOCATION for evaluation of the CLEANUP_BODY, but it should be for evaluation of CLEANUP_EXPR instead. Jason
On 10/16/19 12:27 PM, Jakub Jelinek wrote: > On Fri, Oct 11, 2019 at 04:14:16PM -0400, Jason Merrill wrote: >>> On x86_64 and most other targets, cleanup here (if non-NULL) is the >>> CALL_EXPR, as destructor return type is void, but on arm, as the dtor return >>> type is some pointer, the CALL_EXPR is wrapped into a NOP_EXPR to void. >>> protected_set_expr_location then on x86_64 clears the CALL_EXPR location, >>> but on arm only NOP_EXPR location. >>> >>> The following patch (totally untested) should fix that. >>> >>> For the warning location, perhaps we could special case destructor calls >>> in push_cx_call_context (to offset the intentional clearing of location for >>> debugging purposes), if they don't have location set, don't use >>> input_location for them, but try to pick DECL_SOURCE_LOCATION for the >>> variable being destructed? >> >> Expanding the CLEANUP_EXPR of a CLEANUP_STMT could use the EXPR_LOCATION of >> the CLEANUP_STMT. Or the EXPR_LOCATION of *jump_target, if suitable. > > The already previously posted patch (now attached as first) has now been > bootstrapped/regtested on x86_64-linux and i686-linux, and regardless if we > improve the location or not should fix the arm vs. the rest of the world > difference. Is that ok for trunk? OK. > As for CLEANUP_STMT, I've tried it (the second patch), but it didn't change > anything, the diagnostics was still > constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ > constexpr-dtor3.C:16:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression > 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } > | ^~~ > constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a > as without that change. That's because the patch changes EXPR_LOCATION for evaluation of the CLEANUP_BODY, but it should be for evaluation of CLEANUP_EXPR instead. Jason
On Wed, Oct 16, 2019 at 04:36:07PM -0400, Jason Merrill wrote: > > As for CLEANUP_STMT, I've tried it (the second patch), but it didn't change > > anything, the diagnostics was still > > constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ > > constexpr-dtor3.C:16:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > > constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression > > 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } > > | ^~~ > > constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a > > as without that change. > > That's because the patch changes EXPR_LOCATION for evaluation of the > CLEANUP_BODY, but it should be for evaluation of CLEANUP_EXPR instead. Indeed, that works too. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2019-10-18 Jakub Jelinek <jakub@redhat.com> * constexpr.c (cxx_eval_constant_expression) <case CLEANUP_STMT>: Temporarily change input_location to CLEANUP_STMT location. * g++.dg/cpp2a/constexpr-dtor3.C: Expect in 'constexpr' expansion of message on the line with variable declaration. * g++.dg/ext/constexpr-attr-cleanup1.C: Likewise. --- gcc/cp/constexpr.c.jj 2019-10-17 00:15:50.126726231 +0200 +++ gcc/cp/constexpr.c 2019-10-17 11:21:34.400062565 +0200 @@ -4984,14 +4984,20 @@ cxx_eval_constant_expression (const cons non_constant_p, overflow_p, jump_target); if (!CLEANUP_EH_ONLY (t) && !*non_constant_p) - /* Also evaluate the cleanup. If we weren't skipping at the - start of the CLEANUP_BODY, change jump_target temporarily - to &initial_jump_target, so that even a return or break or - continue in the body doesn't skip the cleanup. */ - cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true, - non_constant_p, overflow_p, - jump_target ? &initial_jump_target - : NULL); + { + location_t loc = input_location; + if (EXPR_HAS_LOCATION (t)) + input_location = EXPR_LOCATION (t); + /* Also evaluate the cleanup. If we weren't skipping at the + start of the CLEANUP_BODY, change jump_target temporarily + to &initial_jump_target, so that even a return or break or + continue in the body doesn't skip the cleanup. */ + cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true, + non_constant_p, overflow_p, + jump_target ? &initial_jump_target + : NULL); + input_location = loc; + } } break; --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C.jj 2019-10-17 00:15:49.425736657 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C 2019-10-17 11:20:13.977290046 +0200 @@ -149,7 +149,7 @@ constexpr int x3 = f3 (); constexpr int f4 () { - W7 w13 = 5; + W7 w13 = 5; // { dg-message "in 'constexpr' expansion of" } return 0; } --- gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C.jj 2019-10-03 00:32:15.604526950 +0200 +++ gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C 2019-10-18 00:18:50.248166117 +0200 @@ -15,7 +15,7 @@ cleanup2 (int *x) constexpr bool foo () { - int a __attribute__((cleanup (cleanup))) = 1; + int a __attribute__((cleanup (cleanup))) = 1; // { dg-message "in 'constexpr' expansion of" } return true; } Jakub
On Thu, Oct 17, 2019 at 6:22 PM Jakub Jelinek <jakub@redhat.com> wrote: > > On Wed, Oct 16, 2019 at 04:36:07PM -0400, Jason Merrill wrote: > > > As for CLEANUP_STMT, I've tried it (the second patch), but it didn't change > > > anything, the diagnostics was still > > > constexpr-dtor3.C:16:23: in ‘constexpr’ expansion of ‘f4()’ > > > constexpr-dtor3.C:16:24: in ‘constexpr’ expansion of ‘(& w13)->W7::~W7()’ > > > constexpr-dtor3.C:5:34: error: inline assembly is not a constant expression > > > 5 | constexpr ~W7 () { if (w == 5) asm (""); w = 3; } // { dg-error "inline assembly is not a constant expression" } > > > | ^~~ > > > constexpr-dtor3.C:5:34: note: only unevaluated inline assembly is allowed in a ‘constexpr’ function in C++2a > > > as without that change. > > > > That's because the patch changes EXPR_LOCATION for evaluation of the > > CLEANUP_BODY, but it should be for evaluation of CLEANUP_EXPR instead. > > Indeed, that works too. Bootstrapped/regtested on x86_64-linux and > i686-linux, ok for trunk? OK, thanks. > 2019-10-18 Jakub Jelinek <jakub@redhat.com> > > * constexpr.c (cxx_eval_constant_expression) <case CLEANUP_STMT>: > Temporarily change input_location to CLEANUP_STMT location. > > * g++.dg/cpp2a/constexpr-dtor3.C: Expect in 'constexpr' expansion of > message on the line with variable declaration. > * g++.dg/ext/constexpr-attr-cleanup1.C: Likewise. > > --- gcc/cp/constexpr.c.jj 2019-10-17 00:15:50.126726231 +0200 > +++ gcc/cp/constexpr.c 2019-10-17 11:21:34.400062565 +0200 > @@ -4984,14 +4984,20 @@ cxx_eval_constant_expression (const cons > non_constant_p, overflow_p, > jump_target); > if (!CLEANUP_EH_ONLY (t) && !*non_constant_p) > - /* Also evaluate the cleanup. If we weren't skipping at the > - start of the CLEANUP_BODY, change jump_target temporarily > - to &initial_jump_target, so that even a return or break or > - continue in the body doesn't skip the cleanup. */ > - cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true, > - non_constant_p, overflow_p, > - jump_target ? &initial_jump_target > - : NULL); > + { > + location_t loc = input_location; > + if (EXPR_HAS_LOCATION (t)) > + input_location = EXPR_LOCATION (t); > + /* Also evaluate the cleanup. If we weren't skipping at the > + start of the CLEANUP_BODY, change jump_target temporarily > + to &initial_jump_target, so that even a return or break or > + continue in the body doesn't skip the cleanup. */ > + cxx_eval_constant_expression (ctx, CLEANUP_EXPR (t), true, > + non_constant_p, overflow_p, > + jump_target ? &initial_jump_target > + : NULL); > + input_location = loc; > + } > } > break; > > --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C.jj 2019-10-17 00:15:49.425736657 +0200 > +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor3.C 2019-10-17 11:20:13.977290046 +0200 > @@ -149,7 +149,7 @@ constexpr int x3 = f3 (); > constexpr int > f4 () > { > - W7 w13 = 5; > + W7 w13 = 5; // { dg-message "in 'constexpr' expansion of" } > return 0; > } > > --- gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C.jj 2019-10-03 00:32:15.604526950 +0200 > +++ gcc/testsuite/g++.dg/ext/constexpr-attr-cleanup1.C 2019-10-18 00:18:50.248166117 +0200 > @@ -15,7 +15,7 @@ cleanup2 (int *x) > constexpr bool > foo () > { > - int a __attribute__((cleanup (cleanup))) = 1; > + int a __attribute__((cleanup (cleanup))) = 1; // { dg-message "in 'constexpr' expansion of" } > return true; > } > > > > Jakub
--- gcc/cp/decl.c.jj 2019-10-10 01:33:38.154943945 +0200 +++ gcc/cp/decl.c 2019-10-11 10:09:24.321277942 +0200 @@ -16864,6 +16864,8 @@ cxx_maybe_build_cleanup (tree decl, tsub the end of the block. So let's unset the location of the destructor call instead. */ protected_set_expr_location (cleanup, UNKNOWN_LOCATION); + if (cleanup && CONVERT_EXPR_P (cleanup)) + protected_set_expr_location (TREE_OPERAND (cleanup, 0), UNKNOWN_LOCATION); if (cleanup && DECL_P (decl)