| Message ID | 87a8metl1a.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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 >
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
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)