"duanbo (C)" <duan...@huawei.com> writes: > Hi > >> -----Original Message----- >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] >> Sent: Monday, April 20, 2020 10:42 PM >> To: duanbo (C) <duan...@huawei.com> >> Cc: GCC Patches <gcc-patches@gcc.gnu.org> >> Subject: Re: [PATCH] aarch64：Add an error message in large code model for >> ilp32 [PR94577] >> >> "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 > > Sorry for misunderstanding what you mean. > I have made a new patch and carried a full test, no new failure witnessed. > Attached please find the v4 patch.
Thanks, pushed to master. Richard
