| Message ID | 583990e4-7340-f9ac-686d-ccc5b4fb7a7d@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix nrv-1.c false failure on aarch64. | expand |
On Wed, Oct 18, 2017 at 6:59 PM, Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> wrote: > Hello, > > Test case "guality.exp=nrv-1.c" fails on aarch64. Optimizations reorder the > instructions and cause the value of a variable to be checked before its > first assignment. The following patch is moving the > break point to the end of the function. Therefore, it ensures that the break > point is reached after the assignment instruction is executed. > > Please review the patch and apply if legitimate. guality testcases are mostly user-experience tests but they are indeed prone to the usual jumpiness. As a user we'd expect a breakpoint on this line to trigger only if previous stmts have been committed. I guess Alex work on stmt frontiers will fix this instance? Richard. > Egeyar >
On 19/10/17 09:14, Richard Biener wrote: > On Wed, Oct 18, 2017 at 6:59 PM, Egeyar Bagcioglu > <egeyar.bagcioglu@oracle.com> wrote: >> Hello, >> >> Test case "guality.exp=nrv-1.c" fails on aarch64. Optimizations reorder the >> instructions and cause the value of a variable to be checked before its >> first assignment. The following patch is moving the >> break point to the end of the function. Therefore, it ensures that the break >> point is reached after the assignment instruction is executed. >> >> Please review the patch and apply if legitimate. > > guality testcases are mostly user-experience tests but they are indeed > prone to the usual jumpiness. As a user we'd expect a breakpoint > on this line to trigger only if previous stmts have been committed. > If all the side effects of the later expression occur before any of the side effects of the earlier one then this would never happen. You might be able to concoct dwarf expressions that gave the appearance of the expression having been evaluated, but memory dumping would almost certainly reveal that memory hadn't really been updated at that point. > I guess Alex work on stmt frontiers will fix this instance? Don't stmt frontiers just enable you to identify exactly one stopping point with each statement, so that you don't keep repeatedly stepping to the same line? R. > > Richard. > >> Egeyar >>
On Thu, Oct 19, 2017 at 2:47 PM, Richard Earnshaw (lists) <Richard.Earnshaw@arm.com> wrote: > On 19/10/17 09:14, Richard Biener wrote: >> On Wed, Oct 18, 2017 at 6:59 PM, Egeyar Bagcioglu >> <egeyar.bagcioglu@oracle.com> wrote: >>> Hello, >>> >>> Test case "guality.exp=nrv-1.c" fails on aarch64. Optimizations reorder the >>> instructions and cause the value of a variable to be checked before its >>> first assignment. The following patch is moving the >>> break point to the end of the function. Therefore, it ensures that the break >>> point is reached after the assignment instruction is executed. >>> >>> Please review the patch and apply if legitimate. >> >> guality testcases are mostly user-experience tests but they are indeed >> prone to the usual jumpiness. As a user we'd expect a breakpoint >> on this line to trigger only if previous stmts have been committed. >> > > If all the side effects of the later expression occur before any of the > side effects of the earlier one then this would never happen. > > You might be able to concoct dwarf expressions that gave the appearance > of the expression having been evaluated, but memory dumping would almost > certainly reveal that memory hadn't really been updated at that point. > >> I guess Alex work on stmt frontiers will fix this instance? > > Don't stmt frontiers just enable you to identify exactly one stopping > point with each statement, so that you don't keep repeatedly stepping to > the same line? I thought they made sure the point it comes up with is at least somewhat correlated with the intended point in the excution flow? If we're allowed to move all those notes to the beginning of the function with just keeping their order I would miss the point of having them at all. But I don't remember anything "invalidating" such notes in the patch so maybe this is indeed what they are... :/ Richard. > R. > >> >> Richard. >> >>> Egeyar >>> > >
On Oct 19, 2017, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> wrote: > On 19/10/17 09:14, Richard Biener wrote: >> I guess Alex work on stmt frontiers will fix this instance? > Don't stmt frontiers just enable you to identify exactly one stopping > point with each statement, so that you don't keep repeatedly stepping to > the same line? There's that, but such stopping points are also ordered WRT debug bind stmts, so that when you stop at such a recommended point, you observe the expected side effects.
On 20/10/17 13:45, Alexandre Oliva wrote: > On Oct 19, 2017, "Richard Earnshaw (lists)" <Richard.Earnshaw@arm.com> > wrote: > >> On 19/10/17 09:14, Richard Biener wrote: >>> I guess Alex work on stmt frontiers will fix this instance? > >> Don't stmt frontiers just enable you to identify exactly one stopping >> point with each statement, so that you don't keep repeatedly stepping to >> the same line? > > There's that, but such stopping points are also ordered WRT debug bind > stmts, so that when you stop at such a recommended point, you observe > the expected side effects. > How do you ensure that if all the instructions from statement2 are scheduled before any of the instructions from statement1? R. > -- > Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/ > You must be the change you wish to see in the world. -- Gandhi > Be Free! -- http://FSFLA.org/ FSF Latin America board member > Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer
On 10/20/2017 03:13 PM, Richard Earnshaw (lists) wrote: >>> On 19/10/17 09:14, Richard Biener wrote: >>>> I guess Alex work on stmt frontiers will fix this instance? Thank you all for the reviews. I fetched Alex's branches in gcc (aoliva/SFN) and in binutils (users/aoliva/SFN). Using gcc and binutils built from these sources, nrv-1.c test case passes. I am following the progress of his patches. Thanks, Egeyar
On 10/18/2017 10:59 AM, Egeyar Bagcioglu wrote: > Hello, > > Test case "guality.exp=nrv-1.c" fails on aarch64. Optimizations reorder > the instructions and cause the value of a variable to be checked before > its first assignment. The following patch is moving the > break point to the end of the function. Therefore, it ensures that the > break point is reached after the assignment instruction is executed. > > Please review the patch and apply if legitimate. This seems wrong. If I understand the test correctly, we want to break on the line with the assignment to a2.i[4] = 7 and verify that before that line executes that a2.i[0] == 42. Moving the test point to the end of the function seems to defeat the purpose of the test. A breakpoint at the end of the function to test state is pointless as it doesn't reflect what a user is likely to want to do. I'm guessing based on your description that optimization has sunk the assignment to a2.i[0] down past the assignment to a2.i[4]? What optimization did this and what do the dwarf records look like? Jeff
On 10/26/2017 05:03 PM, Jeff Law wrote: > On 10/18/2017 10:59 AM, Egeyar Bagcioglu wrote: >> Hello, >> >> Test case "guality.exp=nrv-1.c" fails on aarch64. Optimizations reorder >> the instructions and cause the value of a variable to be checked before >> its first assignment. The following patch is moving the >> break point to the end of the function. Therefore, it ensures that the >> break point is reached after the assignment instruction is executed. >> >> Please review the patch and apply if legitimate. > This seems wrong. > > If I understand the test correctly, we want to break on the line with > the assignment to a2.i[4] = 7 and verify that before that line executes > that a2.i[0] == 42. > > Moving the test point to the end of the function seems to defeat the > purpose of the test. A breakpoint at the end of the function to test > state is pointless as it doesn't reflect what a user is likely to want > to do. > > I'm guessing based on your description that optimization has sunk the > assignment to a2.i[0] down past the assignment to a2.i[4]? What > optimization did this and what do the dwarf records look like? > > > Jeff Hi Jeff, Thank you for the review. Your analysis of the issue is correct. Indeed, I realized after the first reviews that the test was aiming the debugging experience rather than the functionality of the executable. As a result, I withdraw this patch. Earlier reviewers mentioned Alex Oliva's SFN work could fix this test case as well. I verified that. His patches are currently in review. I am following and waiting for their progress before any further action. Egeyar
From a11fe0b1fcf1867a0fa8c4627e347bda07a4c61b Mon Sep 17 00:00:00 2001 From: Egeyar Bagcioglu <egeyar.bagcioglu@oracle.com> Date: Thu, 12 Oct 2017 06:16:30 -0700 Subject: [PATCH] Fix nrv-1.c false failure on aarch64. Reordering instructions due to optimization flags is causing nrv-1.c to fail on aarch64. The value of the variable is checked before the assignment instruction is reached. This patch is moving the breakpoint to the end of the function to prevent such false failures. * gcc.dg/guality/nrv-1.c: Move breakpoint from line 20 to 22. --- gcc/testsuite/gcc.dg/guality/nrv-1.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gcc/testsuite/gcc.dg/guality/nrv-1.c b/gcc/testsuite/gcc.dg/guality/nrv-1.c index 2f4e654..77ad462 100644 --- a/gcc/testsuite/gcc.dg/guality/nrv-1.c +++ b/gcc/testsuite/gcc.dg/guality/nrv-1.c @@ -17,9 +17,9 @@ f () a2.i[0] = 42; if (a3.i[0] != 0) abort (); - a2.i[4] = 7; /* { dg-final { gdb-test 20 "a2.i\[0\]" "42" } } */ + a2.i[4] = 7; return a2; -} +} /* { dg-final { gdb-test 22 "a2.i\[0\]" "42" } } */ int main () -- 1.9.1