RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[Nick Clifton]

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

Hi Markus, Hi Richard,

  PR 70044 reports a problem with the AArch64 backend.  With LTO enabled
  the function aarch64_override_options_after_change can be called more
  than once for the same function.  If only leaf frame pointers were
  being omitted originally, then the first call will set
  flag_omit_frame_pointer to true.  Then the second call will set
  flag_omit_leaf_frame_pointer to false, thus forcing the frame pointer
  to always be omitted, contrary to the original settings of these
  flags.

  The attached patch fixes this problem by setting
  flag_omit_frame_pointer to true, but using a special value of 2 to do
  so.  Then when the second call occurs we can detect this case and
  ensure that we do not set flag_omit_leaf_frame_pointer to false.

  Tested with no regressions on an aarch64-elf toolchain.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2016-03-04  Nick Clifton  <nickc@redhat.com>

	PR target/7044
	* config/aarch64/aarch64.c
	(aarch64_override_options_after_change_1): When forcing
	flag_omit_frame_pointer to be true, use a special value that can
	be detected if this function is called again, thus preventing
	flag_omit_leaf_frame_pointer from being forced to be false.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64.c.patch --]
[-- Type: text/x-patch, Size: 1326 bytes --]

Index: gcc/config/aarch64/aarch64.c
===================================================================
--- gcc/config/aarch64/aarch64.c	(revision 233960)
+++ gcc/config/aarch64/aarch64.c	(working copy)
@@ -8110,6 +8110,21 @@
 static void
 aarch64_override_options_after_change_1 (struct gcc_options *opts)
 {
+  /* The logic here is that if we are disabling all frame pointer generation
+     then we do not need to disable leaf frame pointer generation as a
+     separate operation.  But if we are *only* disabling leaf frame pointer
+     generation then we set flag_omit_frame_pointer to true, but in
+     aarch64_frame_pointer_required we return false only for leaf functions.
+
+     PR 70044: We have to be careful about being called multiple times for the
+     same function.  Once we have decided to set flag_omit_frame_pointer just
+     so that we can omit leaf frame pointers, we must then not interpret a
+     second call as meaning that all frame pointer generation should be
+     omitted.  We do this by setting flag_omit_frame_pointer to a special,
+     non-zero value.  */
+  if (opts->x_flag_omit_frame_pointer == 2)
+    opts->x_flag_omit_frame_pointer = 0;
+
   if (opts->x_flag_omit_frame_pointer)
     opts->x_flag_omit_leaf_frame_pointer = false;
   else if (opts->x_flag_omit_leaf_frame_pointer)

Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[Kyrill Tkachov]

Hi Nick,

On 04/03/16 13:44, Nick Clifton wrote:
> Hi Markus, Hi Richard,
>
>    PR 70044 reports a problem with the AArch64 backend.  With LTO enabled
>    the function aarch64_override_options_after_change can be called more
>    than once for the same function.  If only leaf frame pointers were
>    being omitted originally, then the first call will set
>    flag_omit_frame_pointer to true.  Then the second call will set
>    flag_omit_leaf_frame_pointer to false, thus forcing the frame pointer
>    to always be omitted, contrary to the original settings of these
>    flags.
>
>    The attached patch fixes this problem by setting
>    flag_omit_frame_pointer to true, but using a special value of 2 to do
>    so.  Then when the second call occurs we can detect this case and
>    ensure that we do not set flag_omit_leaf_frame_pointer to false.
>
>    Tested with no regressions on an aarch64-elf toolchain.
>
>    OK to apply ?

Thanks for looking at this.

> Cheers
>    Nick
>
> gcc/ChangeLog
> 2016-03-04  Nick Clifton  <nickc@redhat.com>
>
> 	PR target/7044
> 	* config/aarch64/aarch64.c
> 	(aarch64_override_options_after_change_1): When forcing
> 	flag_omit_frame_pointer to be true, use a special value that can
> 	be detected if this function is called again, thus preventing
> 	flag_omit_leaf_frame_pointer from being forced to be false.
>

+  /* The logic here is that if we are disabling all frame pointer generation
+     then we do not need to disable leaf frame pointer generation as a
+     separate operation.  But if we are*only*  disabling leaf frame pointer
+     generation then we set flag_omit_frame_pointer to true, but in
+     aarch64_frame_pointer_required we return false only for leaf functions.
+
+     PR 70044: We have to be careful about being called multiple times for the
+     same function.  Once we have decided to set flag_omit_frame_pointer just
+     so that we can omit leaf frame pointers, we must then not interpret a
+     second call as meaning that all frame pointer generation should be
+     omitted.  We do this by setting flag_omit_frame_pointer to a special,
+     non-zero value.  */
+  if (opts->x_flag_omit_frame_pointer == 2)
+    opts->x_flag_omit_frame_pointer = 0;
+
    if (opts->x_flag_omit_frame_pointer)
      opts->x_flag_omit_leaf_frame_pointer = false;
    else if (opts->x_flag_omit_leaf_frame_pointer)

This is missing a second hunk from the patch you attached in the PR that I think is necessary
for this to work (setting to x_flag_omit_frame_pointer)...
I've been testing a very similar patch that just changes that logic to:
   if (opts->x_flag_omit_frame_pointer == 1)
     opts->x_flag_omit_leaf_frame_pointer = 0;
   else if (opts->x_flag_omit_leaf_frame_pointer)
     opts->x_flag_omit_frame_pointer = 2;

Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
That's my bad and I'll propose a patch for it soon.

Also, is there a way to create a testcase for the testuite?
i.e. is there a simple way to scan the assembly generated after the final LTO processing
for the presence of the frame pointer?

Thanks,
Kyrill

Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[Nick Clifton]

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

Hi Kyrill,

> This is missing a second hunk from the patch you attached in the PR that I think is necessary
> for this to work (setting to x_flag_omit_frame_pointer)...

Doh!  Silly me - there was a snafu restoring the patch after I had reverted it in order to
check that the pre- and post- patch gcc test results were the same.

> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> That's my bad and I'll propose a patch for it soon.

OK.

> Also, is there a way to create a testcase for the testuite?
> i.e. is there a simple way to scan the assembly generated after the final LTO processing
> for the presence of the frame pointer?

Originally I thought not.  But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
and that made everything simple.

So attached is a revised patch with the missing second hunk restored and a testcase added.
(Which I have checked and confirmed that it does fail without the patch and it does pass
with the patch applied).

OK to apply ?

Cheers
  Nick

gcc/ChangeLog as before...

gcc/testsuite/ChangeLog
2016-03-07  Nick Clifton  <nickc@redhat.com>

	PR target/70044
	* gcc.target/aarch64/pr70044.c: New test.

[-- Attachment #2: aarch64.c.patch.2 --]
[-- Type: application/x-troff-man, Size: 2021 bytes --]

Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[James Greenhalgh]

On Mon, Mar 07, 2016 at 01:12:16PM +0000, Nick Clifton wrote:
> Hi Kyrill,
> 
> > This is missing a second hunk from the patch you attached in the PR that I think is necessary
> > for this to work (setting to x_flag_omit_frame_pointer)...
> 
> Doh!  Silly me - there was a snafu restoring the patch after I had reverted it in order to
> check that the pre- and post- patch gcc test results were the same.
> 
> > Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> > where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> > That's my bad and I'll propose a patch for it soon.
> 
> OK.
> 
> > Also, is there a way to create a testcase for the testuite?
> > i.e. is there a simple way to scan the assembly generated after the final LTO processing
> > for the presence of the frame pointer?
> 
> Originally I thought not.  But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
> and that made everything simple.
> 
> So attached is a revised patch with the missing second hunk restored and a testcase added.
> (Which I have checked and confirmed that it does fail without the patch and it does pass
> with the patch applied).
> 
> OK to apply ?

OK, thanks.

> > Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
> > where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
> > That's my bad and I'll propose a patch for it soon.

I don't think I've seen this on list yet, it might be worth waiting until
Kyrill has put this patch up before you commit.

Thanks,
James

Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[Kyrill Tkachov]

On 10/03/16 15:23, James Greenhalgh wrote:
> On Mon, Mar 07, 2016 at 01:12:16PM +0000, Nick Clifton wrote:
>> Hi Kyrill,
>>
>>> This is missing a second hunk from the patch you attached in the PR that I think is necessary
>>> for this to work (setting to x_flag_omit_frame_pointer)...
>> Doh!  Silly me - there was a snafu restoring the patch after I had reverted it in order to
>> check that the pre- and post- patch gcc test results were the same.
>>
>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
>> OK.
>>
>>> Also, is there a way to create a testcase for the testuite?
>>> i.e. is there a simple way to scan the assembly generated after the final LTO processing
>>> for the presence of the frame pointer?
>> Originally I thought not.  But then I found scan-lto-assembler in testsuite/lib/scanasm.exp
>> and that made everything simple.
>>
>> So attached is a revised patch with the missing second hunk restored and a testcase added.
>> (Which I have checked and confirmed that it does fail without the patch and it does pass
>> with the patch applied).
>>
>> OK to apply ?
> OK, thanks.
>
>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
> I don't think I've seen this on list yet, it might be worth waiting until
> Kyrill has put this patch up before you commit.

Posted at https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00638.html

Kyrill

> Thanks,
> James
>

Re: RFA: PR 70044: Catch a second call to aarch64_override_options_after_change

[Nick Clifton]

Hi James,

>> OK to apply ?
> 
> OK, thanks.

Thanks - applied.

>>> Note that this patch would expose a bug in common/config/aarch64/aarch64-common.c
>>> where there's a thinko in the handling of OPT_momit_leaf_frame_pointer.
>>> That's my bad and I'll propose a patch for it soon.
> 
> I don't think I've seen this on list yet, it might be worth waiting until
> Kyrill has put this patch up before you commit.

I did this.  Plus I checked to make sure that the patch still works and that the new test passes...

Cheers
  Nick