Hi,
After this patch, a few tests are failing when running the testsuite
with -mabi=ilp32:
gcc.target/aarch64/pr63304_1.c (test for excess errors)
gcc.target/aarch64/pr63304_1.c scan-assembler-times adrp 6
gcc.target/aarch64/pr70120-2.c (test for excess errors)
gcc.target/aarch64/pr94530.c (test for excess errors)
gcc.target/aarch64/reload-valid-spoff.c (test for excess errors)
All of them fail because of the new error message: would you mind
adjusting the tests?
Thanks
Christophe
On Tue, 21 Apr 2020 at 16:10, Richard Sandiford
<[email protected]> wrote:
>
> "duanbo (C)" <[email protected]> writes:
> > Hi
> >
> >> -----Original Message-----
> >> From: Richard Sandiford [mailto:[email protected]]
> >> Sent: Monday, April 20, 2020 10:42 PM
> >> To: duanbo (C) <[email protected]>
> >> Cc: GCC Patches <[email protected]>
> >> Subject: Re: [PATCH] aarch64：Add an error message in large code model for
> >> ilp32 [PR94577]
> >>
> >> "duanbo (C)" <[email protected]> writes:
> >> > Hi
> >> >
> >> >> -----Original Message-----
> >> >> From: Richard Sandiford [mailto:[email protected]]
> >> >> Sent: Friday, April 17, 2020 9:38 PM
> >> >> To: duanbo (C) <[email protected]>
> >> >> Cc: Wilco Dijkstra <[email protected]>; [email protected]
> >> >> Subject: Re: [PATCH PR94577] [AArch64] ：Add an error message in large
> >> >> code model for ilp32
> >> >>
> >> >> "duanbo (C)" <[email protected]> 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:[email protected]]
> >> >> > Sent: Tuesday, April 14, 2020 4:40 AM
> >> >> > To: GCC Patches <[email protected]>; duanbo (C)
> >> >> > <[email protected]>
> >> >> > 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 <[email protected]>
> >> >> > 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 <[email protected]>
> >> >> >
> >> >> > 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 <[email protected]>
> >> >> >
> >> >> > 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 <[email protected]>
> >> >> > +
> >> >> > + 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 <[email protected]>
> >> >> >
> >> >> > 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
