[PATCH] Fix nrv-1.c false failure on aarch64.

[PATCH] Fix nrv-1.c false failure on aarch64.

[Egeyar Bagcioglu]

[-- Attachment #1: Type: text/plain, Size: 410 bytes --]

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.

Egeyar


[-- Attachment #2: 0001-Fix-nrv-1.c-false-failure-on-aarch64.patch --]
[-- Type: text/x-patch, Size: 1114 bytes --]

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

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Richard Biener]

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
>

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Richard Earnshaw (lists)]

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
>>

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Richard Biener]

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
>>>
>
>

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Alexandre Oliva]

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.

-- 
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

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Richard Earnshaw (lists)]

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

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Egeyar Bagcioglu]

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

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Jeff Law]

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

Re: [PATCH] Fix nrv-1.c false failure on aarch64.

[Egeyar Bagcioglu]

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