"duanbo (C)" <duan...@huawei.com> writes:
> Thank you for your suggestions.
> I have modified accordingly and a full test has been carried, no new failure
> witnessed.
> Attached please find the new patch which has been adjusted to be suitable for
> git am.
> Does the v2 patch look better ?
>
> Thanks,
> Duan bo
>
> -----Original Message-----
> From: Wilco Dijkstra [mailto:wilco.dijks...@arm.com]
> Sent: Tuesday, April 14, 2020 4:40 AM
> To: GCC Patches <gcc-patches@gcc.gnu.org>; duanbo (C) <duan...@huawei.com>
> Subject: Re: [PATCH PR00002] aarch64：Add an error message in large code model
> for ilp32
>
> Hi Duanbo,
>
>> This is a simple fix for pr94577.
>> The option -mabi=ilp32 should not be used in large code model. Like x86,
>> using -mx32 and -mcmodel=large together will result in an error message.
>> On aarch64, there is no error message for this option conflict.
>> A solution to this problem can be found in the attached patch.
>> Bootstrap and tested on aarch64 Linux platform. No new regression witnessed.
>> Any suggestion?
>
> Thanks for your patch, more than 4GB doesn't make any sense with ILP32 indeed.
> A few suggestions:
>
> It would be good to also update the documentation for -mcmodel=large to state
> it is incompatible with -fpic, -fPIC and -mabi=ilp32.
>
> The patch adds a another switch statement on mcmodel that ignores the
> previous processing done (which may changes the selected mcmodel). It would
> be safer and more concise to use one switch at the top level and in each case
> use an if statement to handle the special cases.
>
> A few minor nitpics:
>
> + PR target/94577
> + * gcc.target/aarch64/pr94577.c : New test
>
> Just like comments, there should be a '.' at the end of changelog entries.
>
> AFAICT the format isn't exactly specified, but the email header should be
> like:
>
> [PATCH][AArch64] PR94577: Add an error message in large code model for ilp32
>
> Sometimes the PR number is also placed in brackets.
>
> Cheers,
> Wilco
>
>
> From feb16a5e5d35d4f632e1be10ce0ac4f4c3505d22 Mon Sep 17 00:00:00 2001
> From: Duan bo <duan...@huawei.com>
> Date: Wed, 15 Apr 2020 05:19:31 -0400
> Subject: [PATCH] aarch64: Add an error message in large code model for ilp32
> [PR94577]
>
> The option -mabi=ilp32 should not be used in large code model. An error
> message is added for the option conflict.
>
> 2020-04-15 Duan bo <duan...@huawei.com>
>
> PR target/94577
> * config/aarch64/aarch64.c: Add an error message for option conflict.
> * doc/invoke.texi (-mcmodel=large): Mention that -mcmodel=large is
> incompatible with -fpic, -fPIC and -mabi=ilp32.
>
> 2020-04-15 Duan bo <duan...@huawei.com>
>
> PR target/94577
> * gcc.target/aarch64/pr94577.c: New test.
> ---
> gcc/ChangeLog | 7 ++++
> gcc/config/aarch64/aarch64.c | 41 ++++++++++++----------
> gcc/doc/invoke.texi | 4 ++-
> gcc/testsuite/ChangeLog | 5 +++
> gcc/testsuite/gcc.target/aarch64/pr94577.c | 10 ++++++
> 5 files changed, 47 insertions(+), 20 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/pr94577.c
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 3c6a45e8fe7..c2f1fcb7bff 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,10 @@
> +2020-04-15 Duan bo <duan...@huawei.com>
> +
> + PR target/94577
> + * config/aarch64/aarch64.c: Add an error message for option conflict.
> + * doc/invoke.texi (-mcmodel=large): Mention that -mcmodel=large is
> + incompatible with -fpic, -fPIC and -mabi=ilp32.
> +
> 2020-04-14 Max Filippov <jcmvb...@gmail.com>
>
> PR target/94584
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 4af562a81ea..f8a38bd899a 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -14707,32 +14707,35 @@ aarch64_init_expanders (void)
> static void
> initialize_aarch64_code_model (struct gcc_options *opts)
> {
> - if (opts->x_flag_pic)
> + aarch64_cmodel = opts->x_aarch64_cmodel_var;
> + switch (opts->x_aarch64_cmodel_var)
> {
> - switch (opts->x_aarch64_cmodel_var)
> - {
> - case AARCH64_CMODEL_TINY:
> + case AARCH64_CMODEL_TINY:
> + if (opts->x_flag_pic)
Minor formatting nit, but: the case statement should be indented
by the same amount as the "{" for the switch statement. The code
after the case statement should be indented by two further columns.
(The formatting is wrong in the existing code too, which is probably
what confused things.)
> aarch64_cmodel = AARCH64_CMODEL_TINY_PIC;
> - break;
> - case AARCH64_CMODEL_SMALL:
> + break;
> + case AARCH64_CMODEL_SMALL:
> + if (opts->x_flag_pic)
> + {
> #ifdef HAVE_AS_SMALL_PIC_RELOCS
> - aarch64_cmodel = (flag_pic == 2
> - ? AARCH64_CMODEL_SMALL_PIC
> - : AARCH64_CMODEL_SMALL_SPIC);
> + aarch64_cmodel = (flag_pic == 2
> + ? AARCH64_CMODEL_SMALL_PIC
> + : AARCH64_CMODEL_SMALL_SPIC);
> #else
> - aarch64_cmodel = AARCH64_CMODEL_SMALL_PIC;
> + aarch64_cmodel = AARCH64_CMODEL_SMALL_PIC;
> #endif
> - break;
> - case AARCH64_CMODEL_LARGE:
> + }
> + break;
> + case AARCH64_CMODEL_LARGE:
> + if (opts->x_flag_pic)
> sorry ("code model %qs with %<-f%s%>", "large",
> opts->x_flag_pic > 1 ? "PIC" : "pic");
> - break;
> - default:
> - gcc_unreachable ();
> - }
> - }
> - else
> - aarch64_cmodel = opts->x_aarch64_cmodel_var;
> + if (opts->x_aarch64_abi == AARCH64_ABI_ILP32)
> + sorry ("code model large not supported in ilp32 mode");
I think "large" should be quoted here, like it is in the pic/PIC message:
sorry ("code model %<large%> not supported in ilp32 mode");
or:
sorry ("code model %qs not supported in ilp32 mode", "large");
The second's probably better. Each message format string creates more
work for translators, and with the second form, there's more chance that
the format can be reused elsewhere.
> + break;
> + default:
> + gcc_unreachable ();
This is pre-existing, but in cases like this, it's probably better
to leave out the default case. That way bootstrap will fail if a
new code model is added.
LGTM otherwise.
Thanks,
Richard
