| Message ID | 012a01d9f710$db84ef40$928ecdc0$@nextmovesoftware.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support g++ 4.8 as a host compiler. | expand |
On Wed, 2023-10-04 at 23:19 +0100, Roger Sayle wrote: > > The recent patch to remove poly_int_pod triggers a bug in g++ 4.8.5's > C++ 11 support which mistakenly believes poly_uint16 has a non-trivial > constructor. This in turn prohibits it from being used as a member in > a union (rtxunion) that constructed statically, resulting in a (fatal) > error during stage 1. A workaround is to add an explicit constructor > to the problematic union, which allows mainline to be bootstrapped with > the system compiler on older RedHat 7 systems. > > This patch has been tested on x86_64-pc-linux-gnu where it allows a > bootstrap to complete when using g++ 4.8.5 as the host compiler. > Ok for mainline? > > > 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> > > gcc/ChangeLog > * rtl.h (rtx_def::u): Add explicit constructor to workaround > issue using g++ 4.8 as a host compiler. AFAIK G++ 5.1 also has a bug (https://gcc.gnu.org/PR65801) breaking building recent GCC. I don't think it's really "maintainable" to ensure current GCC able to be built with a buggy host compiler.
On 10/4/23 16:19, Roger Sayle wrote: > > The recent patch to remove poly_int_pod triggers a bug in g++ 4.8.5's > C++ 11 support which mistakenly believes poly_uint16 has a non-trivial > constructor. This in turn prohibits it from being used as a member in > a union (rtxunion) that constructed statically, resulting in a (fatal) > error during stage 1. A workaround is to add an explicit constructor > to the problematic union, which allows mainline to be bootstrapped with > the system compiler on older RedHat 7 systems. > > This patch has been tested on x86_64-pc-linux-gnu where it allows a > bootstrap to complete when using g++ 4.8.5 as the host compiler. > Ok for mainline? > > > 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> > > gcc/ChangeLog > * rtl.h (rtx_def::u): Add explicit constructor to workaround > issue using g++ 4.8 as a host compiler. I think the bigger question is whether or not we're going to step forward on the minimum build requirements. My recollection was we settled on gcc-4.8 for the benefit of RHEL 7 and Centos 7 which are rapidly approaching EOL (June 2024). I would certainly support stepping forward to a more modern compiler for the build requirements, which might make this patch obsolete. Jeff
Jeff Law <jeffreyalaw@gmail.com> writes: > On 10/4/23 16:19, Roger Sayle wrote: >> The recent patch to remove poly_int_pod triggers a bug in g++ >> 4.8.5's >> C++ 11 support which mistakenly believes poly_uint16 has a non-trivial >> constructor. This in turn prohibits it from being used as a member in >> a union (rtxunion) that constructed statically, resulting in a (fatal) >> error during stage 1. A workaround is to add an explicit constructor >> to the problematic union, which allows mainline to be bootstrapped with >> the system compiler on older RedHat 7 systems. >> This patch has been tested on x86_64-pc-linux-gnu where it allows a >> bootstrap to complete when using g++ 4.8.5 as the host compiler. >> Ok for mainline? >> 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> >> gcc/ChangeLog >> * rtl.h (rtx_def::u): Add explicit constructor to workaround >> issue using g++ 4.8 as a host compiler. > I think the bigger question is whether or not we're going to step > forward on the minimum build requirements. > > My recollection was we settled on gcc-4.8 for the benefit of RHEL 7 > and Centos 7 which are rapidly approaching EOL (June 2024). > > I would certainly support stepping forward to a more modern compiler > for the build requirements, which might make this patch obsolete. See also richi and jakub's comments at https://inbox.sourceware.org/gcc-patches/mpt5y3ppio0.fsf@arm.com/T/#m985295bedaadb47aa0b9ba63b7cb69a660a108bb. > > Jeff
On 10/7/23 15:30, Sam James wrote: > > Jeff Law <jeffreyalaw@gmail.com> writes: > >> On 10/4/23 16:19, Roger Sayle wrote: >>> The recent patch to remove poly_int_pod triggers a bug in g++ >>> 4.8.5's >>> C++ 11 support which mistakenly believes poly_uint16 has a non-trivial >>> constructor. This in turn prohibits it from being used as a member in >>> a union (rtxunion) that constructed statically, resulting in a (fatal) >>> error during stage 1. A workaround is to add an explicit constructor >>> to the problematic union, which allows mainline to be bootstrapped with >>> the system compiler on older RedHat 7 systems. >>> This patch has been tested on x86_64-pc-linux-gnu where it allows a >>> bootstrap to complete when using g++ 4.8.5 as the host compiler. >>> Ok for mainline? >>> 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> >>> gcc/ChangeLog >>> * rtl.h (rtx_def::u): Add explicit constructor to workaround >>> issue using g++ 4.8 as a host compiler. >> I think the bigger question is whether or not we're going to step >> forward on the minimum build requirements. >> >> My recollection was we settled on gcc-4.8 for the benefit of RHEL 7 >> and Centos 7 which are rapidly approaching EOL (June 2024). >> >> I would certainly support stepping forward to a more modern compiler >> for the build requirements, which might make this patch obsolete. > > See also richi and jakub's comments at https://inbox.sourceware.org/gcc-patches/mpt5y3ppio0.fsf@arm.com/T/#m985295bedaadb47aa0b9ba63b7cb69a660a108bb. Yea. As Jakub notes, there's the cfarm situation, but I've had good success with DTS on Centos 7 systems (I have to support some of those internally within Ventana). It quite literally "just works" though users would have to enable it. Alternately, update the cfarm hosts? Jeff
> On 8 Oct 2023, at 05:40, Jeff Law <jeffreyalaw@gmail.com> wrote: > On 10/7/23 15:30, Sam James wrote: >> Jeff Law <jeffreyalaw@gmail.com> writes: >>> On 10/4/23 16:19, Roger Sayle wrote: >>>> The recent patch to remove poly_int_pod triggers a bug in g++ >>>> 4.8.5's >>>> C++ 11 support which mistakenly believes poly_uint16 has a non-trivial >>>> constructor. This in turn prohibits it from being used as a member in >>>> a union (rtxunion) that constructed statically, resulting in a (fatal) >>>> error during stage 1. A workaround is to add an explicit constructor >>>> to the problematic union, which allows mainline to be bootstrapped with >>>> the system compiler on older RedHat 7 systems. >>>> This patch has been tested on x86_64-pc-linux-gnu where it allows a >>>> bootstrap to complete when using g++ 4.8.5 as the host compiler. >>>> Ok for mainline? >>>> 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> >>>> gcc/ChangeLog >>>> * rtl.h (rtx_def::u): Add explicit constructor to workaround >>>> issue using g++ 4.8 as a host compiler. >>> I think the bigger question is whether or not we're going to step >>> forward on the minimum build requirements. >>> >>> My recollection was we settled on gcc-4.8 for the benefit of RHEL 7 >>> and Centos 7 which are rapidly approaching EOL (June 2024). >>> >>> I would certainly support stepping forward to a more modern compiler >>> for the build requirements, which might make this patch obsolete. >> See also richi and jakub's comments at https://inbox.sourceware.org/gcc-patches/mpt5y3ppio0.fsf@arm.com/T/#m985295bedaadb47aa0b9ba63b7cb69a660a108bb. > Yea. As Jakub notes, there's the cfarm situation, but I've had good success with DTS on Centos 7 systems (I have to support some of those internally within Ventana). It quite literally "just works" though users would have to enable it. > > Alternately, update the cfarm hosts? In practice, if one wants to test Ada and D, a newer toolchain is needed anyway - so at least some of us are already using self-built bootstrap toolchains. Is there some blocker to installing a project-built toolchain on /opt, for example? (admittedly it then becomes a point that someone has to take responsibility for providing it). Iain
I'd like to ping my patch for restoring bootstrap using g++ 4.8.5 (the system compiler on RHEL 7 and later systems). https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632008.html Note the preprocessor #ifs can be removed; they are only there to document why the union u must have an explicit, empty (but not default) constructor. I completely agree with the various opinions that we might consider upgrading the minimum host compiler for many good reasons (Ada, D, newer C++ features etc.). It's inevitable that older compilers and systems can't be supported indefinitely. Having said that I don't think that this unintentional trivial breakage, that has a safe one-line work around is sufficient cause (or non-neglible risk or support burden), to inconvenice a large number of GCC users (the impact/disruption to cfarm has already been mentioned). Interestingly, "scl enable devtoolset-XX" to use a newer host compiler, v10 or v11, results in a significant increase (100+) in unexpected failures I see during mainline regression testing using "make -k check" (on RedHat 7.9). (Older) system compilers, despite their flaws, are selected for their (overall) stability and maturity. If another patch/change hits the compiler next week that reasonably means that 4.8.5 can no longer be supported, so be it, but its an annoying (and unnecessary?) inconvenience in the meantime. Perhaps we should file a Bugzilla PR indicating that the documentation and release notes need updating, if my fix isn't considered acceptable? Why this patch is an trigger issue (that requires significant discussion and deliberation) is somewhat of a mystery. Thanks in advance. Roger > -----Original Message----- > From: Jeff Law <jeffreyalaw@gmail.com> > Sent: 07 October 2023 17:20 > To: Roger Sayle <roger@nextmovesoftware.com>; gcc-patches@gcc.gnu.org > Cc: 'Richard Sandiford' <richard.sandiford@arm.com> > Subject: Re: [PATCH] Support g++ 4.8 as a host compiler. > > > > On 10/4/23 16:19, Roger Sayle wrote: > > > > The recent patch to remove poly_int_pod triggers a bug in g++ 4.8.5's > > C++ 11 support which mistakenly believes poly_uint16 has a non-trivial > > constructor. This in turn prohibits it from being used as a member in > > a union (rtxunion) that constructed statically, resulting in a (fatal) > > error during stage 1. A workaround is to add an explicit constructor > > to the problematic union, which allows mainline to be bootstrapped > > with the system compiler on older RedHat 7 systems. > > > > This patch has been tested on x86_64-pc-linux-gnu where it allows a > > bootstrap to complete when using g++ 4.8.5 as the host compiler. > > Ok for mainline? > > > > > > 2023-10-04 Roger Sayle <roger@nextmovesoftware.com> > > > > gcc/ChangeLog > > * rtl.h (rtx_def::u): Add explicit constructor to workaround > > issue using g++ 4.8 as a host compiler. > I think the bigger question is whether or not we're going to step forward on the > minimum build requirements. > > My recollection was we settled on gcc-4.8 for the benefit of RHEL 7 and Centos 7 > which are rapidly approaching EOL (June 2024). > > I would certainly support stepping forward to a more modern compiler for the > build requirements, which might make this patch obsolete. > > Jeff
"Roger Sayle" <roger@nextmovesoftware.com> writes: > I'd like to ping my patch for restoring bootstrap using g++ 4.8.5 > (the system compiler on RHEL 7 and later systems). > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632008.html > > Note the preprocessor #ifs can be removed; they are only there to document > why the union u must have an explicit, empty (but not default) constructor. > > I completely agree with the various opinions that we might consider > upgrading the minimum host compiler for many good reasons (Ada, > D, newer C++ features etc.). It's inevitable that older compilers and > systems can't be supported indefinitely. > > Having said that I don't think that this unintentional trivial breakage, > that has a safe one-line work around is sufficient cause (or non-neglible > risk or support burden), to inconvenice a large number of GCC users > (the impact/disruption to cfarm has already been mentioned). > > Interestingly, "scl enable devtoolset-XX" to use a newer host compiler, > v10 or v11, results in a significant increase (100+) in unexpected failures I see > during mainline regression testing using "make -k check" (on RedHat 7.9). > (Older) system compilers, despite their flaws, are selected for their > (overall) stability and maturity. > > If another patch/change hits the compiler next week that reasonably > means that 4.8.5 can no longer be supported, so be it, but its an > annoying (and unnecessary?) inconvenience in the meantime. > > Perhaps we should file a Bugzilla PR indicating that the documentation > and release notes need updating, if my fix isn't considered acceptable? > > Why this patch is an trigger issue (that requires significant discussion > and deliberation) is somewhat of a mystery. It seemed like there was considerable support for bumping the minimum to beyond 4.8. I think we should wait until a decision has been made before adding more 4.8 workarounds. Having a conditional explicit constructor is dangerous because it changes semantics. E.g. consider: #include <new> union u { int x; }; void f(u *ptr) { new(ptr) u; } void g(u *ptr) { new(ptr) u(); } g(ptr) zeros ptr->x whereas f(ptr) doesn't. If we add "u() {}" then g() does not zero ptr->x. So if we did add the workaround, it would need to be unconditional, like you say. Thanks, Richard
On Sun, Oct 15, 2023 at 7:43 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > "Roger Sayle" <roger@nextmovesoftware.com> writes: > > I'd like to ping my patch for restoring bootstrap using g++ 4.8.5 > > (the system compiler on RHEL 7 and later systems). > > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632008.html > > > > Note the preprocessor #ifs can be removed; they are only there to document > > why the union u must have an explicit, empty (but not default) constructor. > > > > I completely agree with the various opinions that we might consider > > upgrading the minimum host compiler for many good reasons (Ada, > > D, newer C++ features etc.). It's inevitable that older compilers and > > systems can't be supported indefinitely. > > > > Having said that I don't think that this unintentional trivial breakage, > > that has a safe one-line work around is sufficient cause (or non-neglible > > risk or support burden), to inconvenice a large number of GCC users > > (the impact/disruption to cfarm has already been mentioned). > > > > Interestingly, "scl enable devtoolset-XX" to use a newer host compiler, > > v10 or v11, results in a significant increase (100+) in unexpected failures I see > > during mainline regression testing using "make -k check" (on RedHat 7.9). > > (Older) system compilers, despite their flaws, are selected for their > > (overall) stability and maturity. > > > > If another patch/change hits the compiler next week that reasonably > > means that 4.8.5 can no longer be supported, so be it, but its an > > annoying (and unnecessary?) inconvenience in the meantime. > > > > Perhaps we should file a Bugzilla PR indicating that the documentation > > and release notes need updating, if my fix isn't considered acceptable? > > > > Why this patch is an trigger issue (that requires significant discussion > > and deliberation) is somewhat of a mystery. > > It seemed like there was considerable support for bumping the minimum > to beyond 4.8. I think we should wait until a decision has been made > before adding more 4.8 workarounds. > > Having a conditional explicit constructor is dangerous because it changes > semantics. E.g. consider: > > #include <new> > > union u { int x; }; > void f(u *ptr) { new(ptr) u; } > void g(u *ptr) { new(ptr) u(); } > > g(ptr) zeros ptr->x whereas f(ptr) doesn't. If we add "u() {}" then g() > does not zero ptr->x. > > So if we did add the workaround, it would need to be unconditional, > like you say. > > Thanks, > Richard I personally would prefer it if GCC would continue to maintain support for gcc 4.8 for a host compiler. One of the things I like about GCC is how it's generally tried to keep support for older host tools for longer than other projects do, meaning that with GCC, you don't get stuck on the upgrade treadmill of having to compile a whole string of consecutive compilers just to be able to compile the latest one like you have to do with clang. Please just apply Roger's patch; it already exists and is relatively simple.
On Sun, Oct 15, 2023 at 12:43:10PM +0100, Richard Sandiford wrote: > It seemed like there was considerable support for bumping the minimum > to beyond 4.8. I think we should wait until a decision has been made > before adding more 4.8 workarounds. I think adding a workaround until that decision is made and perhaps removing it afterwards will make life easier for people still using gcc 4.8. > Having a conditional explicit constructor is dangerous because it changes > semantics. E.g. consider: > > #include <new> > > union u { int x; }; > void f(u *ptr) { new(ptr) u; } > void g(u *ptr) { new(ptr) u(); } > > g(ptr) zeros ptr->x whereas f(ptr) doesn't. If we add "u() {}" then g() > does not zero ptr->x. > > So if we did add the workaround, it would need to be unconditional, > like you say. What about using more directed workaround then? Like (just stage1 build tested, perhaps with comment why we do that) below? Seems at least in stage1 it is the only problematic spot. --- a/gcc/cse.cc +++ b/gcc/cse.cc @@ -4951,8 +4951,14 @@ cse_insn (rtx_insn *insn) && is_a <scalar_int_mode> (mode, &int_mode) && (extend_op = load_extend_op (int_mode)) != UNKNOWN) { +#if GCC_VERSION >= 5000 struct rtx_def memory_extend_buf; rtx memory_extend_rtx = &memory_extend_buf; +#else + alignas (alignof (rtx_def)) unsigned char + memory_extended_buf[sizeof (rtx_def)]; + rtx memory_extend_rtx = (rtx) &memory_extended_buf[0]; +#endif /* Set what we are trying to extend and the operation it might have been extended with. */ Jakub
Jakub Jelinek <jakub@redhat.com> writes: > On Sun, Oct 15, 2023 at 12:43:10PM +0100, Richard Sandiford wrote: >> It seemed like there was considerable support for bumping the minimum >> to beyond 4.8. I think we should wait until a decision has been made >> before adding more 4.8 workarounds. > > I think adding a workaround until that decision is made and perhaps > removing it afterwards will make life easier for people still using gcc 4.8. > >> Having a conditional explicit constructor is dangerous because it changes >> semantics. E.g. consider: >> >> #include <new> >> >> union u { int x; }; >> void f(u *ptr) { new(ptr) u; } >> void g(u *ptr) { new(ptr) u(); } >> >> g(ptr) zeros ptr->x whereas f(ptr) doesn't. If we add "u() {}" then g() >> does not zero ptr->x. >> >> So if we did add the workaround, it would need to be unconditional, >> like you say. > > What about using more directed workaround then? > > Like (just stage1 build tested, perhaps with comment why we do that) > below? Seems at least in stage1 it is the only problematic spot. > > --- a/gcc/cse.cc > +++ b/gcc/cse.cc > @@ -4951,8 +4951,14 @@ cse_insn (rtx_insn *insn) > && is_a <scalar_int_mode> (mode, &int_mode) > && (extend_op = load_extend_op (int_mode)) != UNKNOWN) > { > +#if GCC_VERSION >= 5000 > struct rtx_def memory_extend_buf; > rtx memory_extend_rtx = &memory_extend_buf; > +#else > + alignas (alignof (rtx_def)) unsigned char > + memory_extended_buf[sizeof (rtx_def)]; Looks like the simpler "alignas (rtx_def)" should work. LGTM otherwise FWIW. Richard > + rtx memory_extend_rtx = (rtx) &memory_extended_buf[0]; > +#endif > > /* Set what we are trying to extend and the operation it might > have been extended with. */ > > > Jakub
On Wed, Oct 18, 2023 at 11:23:49AM +0100, Richard Sandiford wrote: > > --- a/gcc/cse.cc > > +++ b/gcc/cse.cc > > @@ -4951,8 +4951,14 @@ cse_insn (rtx_insn *insn) > > && is_a <scalar_int_mode> (mode, &int_mode) > > && (extend_op = load_extend_op (int_mode)) != UNKNOWN) > > { > > +#if GCC_VERSION >= 5000 > > struct rtx_def memory_extend_buf; > > rtx memory_extend_rtx = &memory_extend_buf; > > +#else > > + alignas (alignof (rtx_def)) unsigned char > > + memory_extended_buf[sizeof (rtx_def)]; > > Looks like the simpler "alignas (rtx_def)" should work. It does. > LGTM otherwise FWIW. Here is what I'm bootstrapping/regtesting on gcc112 now (i.e. with 4.8.5 as system compiler), added details what bug we are working around. The reduced testcase on which I've bisected it is: struct poly_int { poly_int() = default; template <typename ...T> poly_int(const T &...) {} }; union rtunion { poly_int rt_subregrt_rtx; }; struct rtx_def { rtunion fld; }; void cse_insn() { rtx_def memory_extend_buf; } or even with just template <typename> poly_int(); line in there. Bet gcc 4.8/4.9 was unhappy about the template variadic ctor accepting empty pack and being like the default ctor but not defaulted in that case. Making it guaranteed that it has at least one argument say through template <typename U, typename ...T> poly_int(const U &, const T &...) {} fixes it for 4.8/4.9 as well. 2023-10-18 Jakub Jelinek <jakub@redhat.com> PR bootstrap/111852 * cse.cc (cse_insn): Add workaround for GCC 4.8-4.9, instead of using rtx_def type for memory_extend_buf, use unsigned char arrayy with size of rtx_def and its alignment. --- gcc/cse.cc.jj 2023-06-20 08:57:38.339505245 +0200 +++ gcc/cse.cc 2023-10-18 13:20:30.555836778 +0200 @@ -4951,8 +4951,15 @@ cse_insn (rtx_insn *insn) && is_a <scalar_int_mode> (mode, &int_mode) && (extend_op = load_extend_op (int_mode)) != UNKNOWN) { +#if GCC_VERSION >= 5000 struct rtx_def memory_extend_buf; rtx memory_extend_rtx = &memory_extend_buf; +#else + /* Workaround GCC < 5 bug, fixed in r5-3834 as part of PR63362 + fix. */ + alignas (rtx_def) unsigned char memory_extended_buf[sizeof (rtx_def)]; + rtx memory_extend_rtx = (rtx) &memory_extended_buf[0]; +#endif /* Set what we are trying to extend and the operation it might have been extended with. */ Jakub
On Wed, Oct 18, 2023 at 01:33:40PM +0200, Jakub Jelinek wrote: > Making it guaranteed that it has at least one argument say through > template <typename U, typename ...T> poly_int(const U &, const T &...) {} > fixes it for 4.8/4.9 as well. So, perhaps (but so far totally untested, the other bootstrap is still running): 2023-10-18 Jakub Jelinek <jakub@redhat.com> * poly-int.h (poly_int::poly_int): Ensure the const Cs &... argument ctor has at least one argument. --- gcc/poly-int.h.jj 2023-10-13 19:34:44.112832389 +0200 +++ gcc/poly-int.h 2023-10-18 13:49:29.038751482 +0200 @@ -379,8 +379,8 @@ public: template<typename Ca> poly_int (const poly_int<N, Ca> &); - template<typename ...Cs> - constexpr poly_int (const Cs &...); + template<typename C0, typename ...Cs> + constexpr poly_int (const C0 &, const Cs &...); poly_int &operator = (const poly_int &) = default; @@ -446,11 +446,11 @@ poly_int<N, C>::poly_int (const poly_int } template<unsigned int N, typename C> -template<typename ...Cs> +template<typename C0, typename ...Cs> inline constexpr -poly_int<N, C>::poly_int (const Cs &... cs) +poly_int<N, C>::poly_int (const C0 &c0, const Cs &... cs) : poly_int (typename poly_int_fullness<sizeof... (Cs) >= N>::type (), - cs...) {} + c0, cs...) {} /* Initialize with c0, cs..., and some trailing zeros. */ template<unsigned int N, typename C> Jakub
diff --git a/gcc/rtl.h b/gcc/rtl.h index 6850281..a7667f5 100644 --- a/gcc/rtl.h +++ b/gcc/rtl.h @@ -451,6 +451,9 @@ struct GTY((desc("0"), tag("0"), struct fixed_value fv; struct hwivec_def hwiv; struct const_poly_int_def cpi; +#if defined(__GNUC__) && GCC_VERSION < 5000 + u () {} +#endif } GTY ((special ("rtx_def"), desc ("GET_CODE (&%0)"))) u; };