"duanbo (C)" <duan...@huawei.com> writes:
> Hi
>
>> -----Original Message-----
>> From: Richard Sandiford [mailto:richard.sandif...@arm.com]
>> Sent: Friday, April 17, 2020 9:38 PM
>> To: duanbo (C) <duan...@huawei.com>
>> Cc: Wilco Dijkstra <wilco.dijks...@arm.com>; gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH PR94577] [AArch64] ：Add an error message in large
>> code model for ilp32
>>
>> "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.
My quoting made it very unclear, sorry, but here I meant we should remove
the whole "default:" case. Of course, that triggers exactly the kind of
bootstrap failure I mentioned:
error: enumeration value ‘AARCH64_CMODEL_TINY_PIC’ not handled in switch
[-Werror=switch]
error: enumeration value ‘AARCH64_CMODEL_SMALL_PIC’ not handled in switch
[-Werror=switch]
error: enumeration value ‘AARCH64_CMODEL_SMALL_SPIC’ not handled in switch
[-Werror=switch]
I think it would be good to have:
case AARCH64_CMODEL_TINY_PIC:
case AARCH64_CMODEL_SMALL_PIC:
case AARCH64_CMODEL_SMALL_SPIC:
gcc_unreachable ();
}
and no "default:" case.
(When I was reviewing the original patch and existing code, it wasn't
obvious to me why we needed the default: case and gcc_unreachable, but
that was probably just me being dumb. :-) Listing the individual case
statements makes things more explicit. It also means we'll get warnings/
errors if we forget to update the switch statement for a new code model.)
Can you retest and repost with that change? Sorry for the hassle.
Thanks,
Richard
