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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

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