| Message ID | 20201029004204.GA15149@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | PowerPC: PR libgcc/97543, build libgcc with -mno-gnu-attribute | expand |
Hi Mike, On Wed, Oct 28, 2020 at 08:42:04PM -0400, Michael Meissner via Gcc-patches wrote: > PowerPC: PR libgcc/97543, fix 64-bit long double issues > > There are two issues in PR libgcc/97543 which shows up if you build a GCC > compiler with long double defaulting to 64-bit instead of 128-bit with IBM > extended double: > > 1) The first issue was the t-linux file forced the entire libgcc library > to be compiled with the -mlong-double-128 option. Why is this is wrong? If you are configuring using --without-long-double-128 then that doesn't mean 128-bit long doubles are unsupported, it just selects the default to be 64-bit long double. A compiler built using --without-long-double-128 can generate code for 128-bit long double by simply using -mlong-double-128. In which case you need the libgcc support for 128-bit long doubles. Well, I suppose you are passing -mlong-double-128 for those objects that need it, but I can't see any harm in passing -mlong-double-128 everywhere in libgcc. It seems to me that *not* using -mlong-double-128 then opens you up to the .gnu_attribute bug where we mark an object as using 64-bit long double when it really is just using plain double. > > 2) The second issue is that the GNU attribute #4 is set to reflect using > 128-bit long doubles, and you get linker warnings when you use use the > compiler, since libgcc_s.so indicates 128-bit IBM long doubles were > used. I ran into a similar issue with my patches to extend libgcc to > work if long doubles were configured to use the 128-bit IEEE format > instead of the 128-bit IBM format. > > One feature of the current GNU attribute implementation is if you have a shared > library (such as libgcc_s.so), the GNU attributes for the shared library is an > inclusive OR of all of the modules within the library. We do OR in non-conflicting attributes, but conflicting ones cause errors, or are removed if the linker is given --no-warn-mismatch. For example: cat > attr-ibm.s <<EOF .gnu_attribute 4,5 EOF cat > attr-64.s <<EOF .gnu_attribute 4,9 EOF powerpc64le-linux-as -o attr-ibm.o attr-ibm.s powerpc64le-linux-readelf -A attr-ibm.o powerpc64le-linux-as -o attr-64.o attr-64.s powerpc64le-linux-readelf -A attr-64.o powerpc64le-linux-ld -shared -o foo.so attr-ibm.o attr-64.o #fails with an error powerpc64le-linux-ld -shared --no-warn-mismatch -o foo.so attr-ibm.o attr-64.o #succeeds powerpc64le-linux-readelf -A foo.so #displays nothing, conflicting attributes are removed > This means if any > module uses the -mlong-double-128 option and uses long double, the GNU > attributes for the library will indicate that it uses 128-bit IBM long > doubles. If you have a static library, you will get the warning only if you > actually reference a module with the attribute set. > > This patch does two things: > > 1) Instead of compiling the whole library with -mlong-double-128, it only > compiles the modules that process the IBM extended double format with > this switch. It also specifies that these files must be compiled using > the IBM format for long double. > > 2) I turned off GNU attributes for the whole library. Originally, I just > turned off GNU attributes for just the modules that process IBM > extended format values. But this doesn't work if the compiler defaults > long double to 64-bits. What happens is the logic in rs6000.c that > sets the GNU attribute bits, will set the bits for 64-bit long double > if a normal double (DFmode) is used. So I just turned off the > attributes for the whole library. > > This patch replaces the patch I previously did for IEEE 128-bit to turn off > GNU attributes for just the ibm-ldouble.o module. > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556863.html > > I have tested this by building a compiler on a little endian power9 system > running Linux with long double defaulting to 64-bits using the configure > option: --without-long-double-128, and I verified that the warning no longer is > generated by the linker. > > I then built a bootstrap compiler, by first building a non-bootstrap version. > With that non-bootstrap compiler, I built versions of the MPC and MPFR. Using > those libraries, and the non-bootstrap compiler as the host compiler, I was > able to do a full bootstrap compiler. > > There are differences in the regression test suite where the test implicitly > assumed long double was 128-bits or was a float128 test. > > Can I install this patch into the master branch? I would also like to install > it in the GCC 10 branch after an appropriate period. > > libgcc/ > 2020-10-28 Michael Meissner <meissner@linux.ibm.com> > > PR libgcc/97543 > * config/rs6000/t-linux (HOST_LIBGCC2_CFLAGS): Don't set > -mlong-double-128 for all modules. Instead set > -mno-gnu-attributes. > (IBM128_OBJS): New make variable for long double support. > (IBM128_S_OBJS): New make variable for long double support. > (IBM128_ALL_OBJS): New make variable for long double support. > (IBM128_CFLAGS): New make variable for long double support. > --- > libgcc/config/rs6000/t-linux | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/libgcc/config/rs6000/t-linux b/libgcc/config/rs6000/t-linux > index ed821947b66..b2a079c6b54 100644 > --- a/libgcc/config/rs6000/t-linux > +++ b/libgcc/config/rs6000/t-linux > @@ -1,6 +1,22 @@ > SHLIB_MAPFILES += $(srcdir)/config/rs6000/libgcc-glibc.ver > > -HOST_LIBGCC2_CFLAGS += -mlong-double-128 > +# On the modules that deal with IBM 128-bit values, we need to make sure that > +# TFmode uses the IBM extended double format. > +IBM128_OBJS = ibm-ldouble$(objext) _powitf2$(objext) ppc64-fp$(objext) \ > + _divtc3$(object) _multc3$(object) \ > + _fixtfdi$(object) _fixunstfdi$(object) \ > + _floatditf$(objext) _floatunsditf$(objext) > + > +IBM128_S_OBJS = $(patsubst %$(objext),%_s$(objext),$(IBM128_OBJS)) > +IBM128_ALL_OBJS = $(IBM128_OBJS) $(IBM128_S_OBJS) > + > +IBM128_CFLAGS = -mlong-double-128 -Wno-psabi -mabi=ibmlongdouble > + > +$(IBM128_ALL_OBJS) : INTERNAL_CFLAGS += $(IBM128_CFLAGS) > + > +# Turn off gnu attributes for the whole library. This allows us to build > +# libgcc that supports the different long double formats. > +HOST_LIBGCC2_CFLAGS += -mno-gnu-attribute It would be good if you could pass -mno-gnu-attribute just for the libgcc_s.so objects. That way you might save users from making mistakes due to using the wrong libgcc.a somehow. I think you may be able to do that with gcc_s_compile += -mno-gnu-attribute And, yes, I completely agree that libgcc_s.so should not be marked with .gnu_attribute. > # This is a way of selecting -mcmodel=small for ppc64, which gives > # smaller and faster libgcc code. Directly specifying -mcmodel=small > -- > 2.22.0 > > > -- > Michael Meissner, IBM > IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA > email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
Self-ping. Mike, can you reply to Alan's mail please? He makes a lot of points, not all of them bad certainly! Segher On Wed, Oct 28, 2020 at 08:42:04PM -0400, Michael Meissner wrote: > PowerPC: PR libgcc/97543, fix 64-bit long double issues > > There are two issues in PR libgcc/97543 which shows up if you build a GCC > compiler with long double defaulting to 64-bit instead of 128-bit with IBM > extended double: > > 1) The first issue was the t-linux file forced the entire libgcc library > to be compiled with the -mlong-double-128 option. > > 2) The second issue is that the GNU attribute #4 is set to reflect using > 128-bit long doubles, and you get linker warnings when you use use the > compiler, since libgcc_s.so indicates 128-bit IBM long doubles were > used. I ran into a similar issue with my patches to extend libgcc to > work if long doubles were configured to use the 128-bit IEEE format > instead of the 128-bit IBM format. > > One feature of the current GNU attribute implementation is if you have a shared > library (such as libgcc_s.so), the GNU attributes for the shared library is an > inclusive OR of all of the modules within the library. This means if any > module uses the -mlong-double-128 option and uses long double, the GNU > attributes for the library will indicate that it uses 128-bit IBM long > doubles. If you have a static library, you will get the warning only if you > actually reference a module with the attribute set. > > This patch does two things: > > 1) Instead of compiling the whole library with -mlong-double-128, it only > compiles the modules that process the IBM extended double format with > this switch. It also specifies that these files must be compiled using > the IBM format for long double. > > 2) I turned off GNU attributes for the whole library. Originally, I just > turned off GNU attributes for just the modules that process IBM > extended format values. But this doesn't work if the compiler defaults > long double to 64-bits. What happens is the logic in rs6000.c that > sets the GNU attribute bits, will set the bits for 64-bit long double > if a normal double (DFmode) is used. So I just turned off the > attributes for the whole library. > > This patch replaces the patch I previously did for IEEE 128-bit to turn off > GNU attributes for just the ibm-ldouble.o module. > https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556863.html > > I have tested this by building a compiler on a little endian power9 system > running Linux with long double defaulting to 64-bits using the configure > option: --without-long-double-128, and I verified that the warning no longer is > generated by the linker. > > I then built a bootstrap compiler, by first building a non-bootstrap version. > With that non-bootstrap compiler, I built versions of the MPC and MPFR. Using > those libraries, and the non-bootstrap compiler as the host compiler, I was > able to do a full bootstrap compiler. > > There are differences in the regression test suite where the test implicitly > assumed long double was 128-bits or was a float128 test. > > Can I install this patch into the master branch? I would also like to install > it in the GCC 10 branch after an appropriate period. > > libgcc/ > 2020-10-28 Michael Meissner <meissner@linux.ibm.com> > > PR libgcc/97543 > * config/rs6000/t-linux (HOST_LIBGCC2_CFLAGS): Don't set > -mlong-double-128 for all modules. Instead set > -mno-gnu-attributes. > (IBM128_OBJS): New make variable for long double support. > (IBM128_S_OBJS): New make variable for long double support. > (IBM128_ALL_OBJS): New make variable for long double support. > (IBM128_CFLAGS): New make variable for long double support. > --- > libgcc/config/rs6000/t-linux | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/libgcc/config/rs6000/t-linux b/libgcc/config/rs6000/t-linux > index ed821947b66..b2a079c6b54 100644 > --- a/libgcc/config/rs6000/t-linux > +++ b/libgcc/config/rs6000/t-linux > @@ -1,6 +1,22 @@ > SHLIB_MAPFILES += $(srcdir)/config/rs6000/libgcc-glibc.ver > > -HOST_LIBGCC2_CFLAGS += -mlong-double-128 > +# On the modules that deal with IBM 128-bit values, we need to make sure that > +# TFmode uses the IBM extended double format. > +IBM128_OBJS = ibm-ldouble$(objext) _powitf2$(objext) ppc64-fp$(objext) \ > + _divtc3$(object) _multc3$(object) \ > + _fixtfdi$(object) _fixunstfdi$(object) \ > + _floatditf$(objext) _floatunsditf$(objext) > + > +IBM128_S_OBJS = $(patsubst %$(objext),%_s$(objext),$(IBM128_OBJS)) > +IBM128_ALL_OBJS = $(IBM128_OBJS) $(IBM128_S_OBJS) > + > +IBM128_CFLAGS = -mlong-double-128 -Wno-psabi -mabi=ibmlongdouble > + > +$(IBM128_ALL_OBJS) : INTERNAL_CFLAGS += $(IBM128_CFLAGS) > + > +# Turn off gnu attributes for the whole library. This allows us to build > +# libgcc that supports the different long double formats. > +HOST_LIBGCC2_CFLAGS += -mno-gnu-attribute > > # This is a way of selecting -mcmodel=small for ppc64, which gives > # smaller and faster libgcc code. Directly specifying -mcmodel=small > -- > 2.22.0 > > > -- > Michael Meissner, IBM > IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA > email: meissner@linux.ibm.com, phone: +1 (978) 899-4797
On Sat, Oct 31, 2020 at 11:39:23PM +1030, Alan Modra wrote: > Hi Mike, > On Wed, Oct 28, 2020 at 08:42:04PM -0400, Michael Meissner via Gcc-patches wrote: > > PowerPC: PR libgcc/97543, fix 64-bit long double issues > > > > There are two issues in PR libgcc/97543 which shows up if you build a GCC > > compiler with long double defaulting to 64-bit instead of 128-bit with IBM > > extended double: > > > > 1) The first issue was the t-linux file forced the entire libgcc library > > to be compiled with the -mlong-double-128 option. > > Why is this is wrong? If you are configuring using > --without-long-double-128 then that doesn't mean 128-bit long doubles > are unsupported, it just selects the default to be 64-bit long double. > A compiler built using --without-long-double-128 can generate code > for 128-bit long double by simply using -mlong-double-128. In which > case you need the libgcc support for 128-bit long doubles. Well, I > suppose you are passing -mlong-double-128 for those objects that need > it, but I can't see any harm in passing -mlong-double-128 everywhere > in libgcc. It was more just a case for explicitly adding -mabi=ibmlongdouble for the modules that need it, and letting the default option be used for the modules that don't use IBM long doubles. Because we only have set of GNU attributes for an entire shared library, it is important that all 3 cases for long double not set the attributes. I first took off the -mno-gnu-attributes options, and built libgcc with 3 different compilers (long double = 64 bits, long double = IEEE 128-bits, and long double = IBM 128-bit). The modules in the patch were the ones that set gnu attribute #4 to 5 (i.e. use IBM 128-bit floating point). I then looked in the libgcc built with the configure option: --without-long-double-128 and a bunch of modules showed setting gnu attribute #4 to 9 (i.e. use of long double as double). The reason is within GCC if you compile with -mlong-double-64 (or build with a compiler configured with --without-long-double-128), every time you use double or _Complex double, it will set gnu attribute #4 to 5. It is due to this code in rs6000_emit_move in rs6000.c: #ifdef HAVE_AS_GNU_ATTRIBUTE /* If we use a long double type, set the flags in .gnu_attribute that say what the long double type is. This is to allow the linker's warning message for the wrong long double to be useful, even if the function does not do a call (for example, doing a 128-bit add on power9 if the long double type is IEEE 128-bit. Do not set this if __ibm128 or __floa128 are used if they aren't the default long dobule type. */ if (rs6000_gnu_attr && (HAVE_LD_PPC_GNU_ATTR_LONG_DOUBLE || TARGET_64BIT)) { if (TARGET_LONG_DOUBLE_128 && (mode == TFmode || mode == TCmode)) rs6000_passes_float = rs6000_passes_long_double = true; else if (!TARGET_LONG_DOUBLE_128 && (mode == DFmode || mode == DCmode)) rs6000_passes_float = rs6000_passes_long_double = true; } #endif The code in rs6000-call.c (init_cumulative_args and rs6000_function_arg_advance_1) actually does look at the types and only sets rs6000_passes_long_double if the type is the long double type. So I figured it was better just to build libgcc where we explicitly enable the IBM floating point and just turn off gnu attributes globally in the library. The code in rs6000_emit_move is perhaps well intentioned, but by looking at just modes and not types, it can miss things. However, that is a giant can of worms, and we can play whack-a-mole trying to get everything right. Perhaps somebody (else) can tackle it. But I would prefer to just fix the issue at hand, rather than possibly having an endless battle to get things right. As I recall, what the code in rs6000_emit_move is trying to do is to catch places where you use long double, but no call is made. I.e. struct foo { /* ... */ long double ld; /* ... */ }; void bar (struct foo *p) { /* ... */ (p->ld)++; /* ... */ } On a power9/10 system with IEEE long double or on a system with a 64-bit long double, that code would not generate a call, but it does depend on the long double format (and hence should set the gnu attribute #4).
Hi! On Tue, Nov 03, 2020 at 10:25:05PM -0500, Michael Meissner wrote: > On Sat, Oct 31, 2020 at 11:39:23PM +1030, Alan Modra wrote: > > Why is this is wrong? If you are configuring using > > --without-long-double-128 then that doesn't mean 128-bit long doubles > > are unsupported, it just selects the default to be 64-bit long double. > > A compiler built using --without-long-double-128 can generate code > > for 128-bit long double by simply using -mlong-double-128. In which > > case you need the libgcc support for 128-bit long doubles. Well, I > > suppose you are passing -mlong-double-128 for those objects that need > > it, but I can't see any harm in passing -mlong-double-128 everywhere > > in libgcc. Yeah. > It was more just a case for explicitly adding -mabi=ibmlongdouble for the > modules that need it, and letting the default option be used for the modules > that don't use IBM long doubles. > > Because we only have set of GNU attributes for an entire shared library, it is > important that all 3 cases for long double not set the attributes. > > I first took off the -mno-gnu-attributes options, and built libgcc with 3 > different compilers (long double = 64 bits, long double = IEEE 128-bits, and > long double = IBM 128-bit). The modules in the patch were the ones that set > gnu attribute #4 to 5 (i.e. use IBM 128-bit floating point). > > I then looked in the libgcc built with the configure option: > --without-long-double-128 > > and a bunch of modules showed setting gnu attribute #4 to 9 (i.e. use of long > double as double). > > The reason is within GCC if you compile with -mlong-double-64 (or build with a > compiler configured with --without-long-double-128), every time you use double > or _Complex double, it will set gnu attribute #4 to 5. It is due to this code > in rs6000_emit_move in rs6000.c: > > #ifdef HAVE_AS_GNU_ATTRIBUTE > /* If we use a long double type, set the flags in .gnu_attribute that say > what the long double type is. This is to allow the linker's warning > message for the wrong long double to be useful, even if the function does > not do a call (for example, doing a 128-bit add on power9 if the long > double type is IEEE 128-bit. Do not set this if __ibm128 or __floa128 are > used if they aren't the default long dobule type. */ > if (rs6000_gnu_attr && (HAVE_LD_PPC_GNU_ATTR_LONG_DOUBLE || TARGET_64BIT)) > { > if (TARGET_LONG_DOUBLE_128 && (mode == TFmode || mode == TCmode)) > rs6000_passes_float = rs6000_passes_long_double = true; > > else if (!TARGET_LONG_DOUBLE_128 && (mode == DFmode || mode == DCmode)) > rs6000_passes_float = rs6000_passes_long_double = true; > } > #endif Yeah that is just bad. Fix that? Modes are not types. > The code in rs6000-call.c (init_cumulative_args and > rs6000_function_arg_advance_1) actually does look at the types and only sets > rs6000_passes_long_double if the type is the long double type. > > So I figured it was better just to build libgcc where we explicitly enable the > IBM floating point and just turn off gnu attributes globally in the library. It is much better to just fix the bug. Please try that, it is a real bug that needs real fixing no matter what, so might as well do it now (in a separate patch) and not have to work around it for the one place you know it to hurt now. > The code in rs6000_emit_move is perhaps well intentioned, but by looking at > just modes and not types, it can miss things. However, that is a giant can of > worms, and we can play whack-a-mole trying to get everything right. Perhaps > somebody (else) can tackle it. But I would prefer to just fix the issue at > hand, rather than possibly having an endless battle to get things right. It does very much the wrong thing now. It needs fixing. You need it fixed for this patch, to do this patch properly. Please just fix it. > As I recall, what the code in rs6000_emit_move is trying to do is to catch > places where you use long double, but no call is made. I.e. > > struct foo { > /* ... */ > long double ld; > /* ... */ > }; > > void bar (struct foo *p) > { > /* ... */ > (p->ld)++; > /* ... */ > } > > On a power9/10 system with IEEE long double or on a system with a 64-bit long > double, that code would not generate a call, but it does depend on the long > double format (and hence should set the gnu attribute #4). Yes, but setting it for any DFmode (with 64-bit long double) is just wrong. Segher
diff --git a/libgcc/config/rs6000/t-linux b/libgcc/config/rs6000/t-linux index ed821947b66..b2a079c6b54 100644 --- a/libgcc/config/rs6000/t-linux +++ b/libgcc/config/rs6000/t-linux @@ -1,6 +1,22 @@ SHLIB_MAPFILES += $(srcdir)/config/rs6000/libgcc-glibc.ver -HOST_LIBGCC2_CFLAGS += -mlong-double-128 +# On the modules that deal with IBM 128-bit values, we need to make sure that +# TFmode uses the IBM extended double format. +IBM128_OBJS = ibm-ldouble$(objext) _powitf2$(objext) ppc64-fp$(objext) \ + _divtc3$(object) _multc3$(object) \ + _fixtfdi$(object) _fixunstfdi$(object) \ + _floatditf$(objext) _floatunsditf$(objext) + +IBM128_S_OBJS = $(patsubst %$(objext),%_s$(objext),$(IBM128_OBJS)) +IBM128_ALL_OBJS = $(IBM128_OBJS) $(IBM128_S_OBJS) + +IBM128_CFLAGS = -mlong-double-128 -Wno-psabi -mabi=ibmlongdouble + +$(IBM128_ALL_OBJS) : INTERNAL_CFLAGS += $(IBM128_CFLAGS) + +# Turn off gnu attributes for the whole library. This allows us to build +# libgcc that supports the different long double formats. +HOST_LIBGCC2_CFLAGS += -mno-gnu-attribute # This is a way of selecting -mcmodel=small for ppc64, which gives # smaller and faster libgcc code. Directly specifying -mcmodel=small
PowerPC: PR libgcc/97543, fix 64-bit long double issues There are two issues in PR libgcc/97543 which shows up if you build a GCC compiler with long double defaulting to 64-bit instead of 128-bit with IBM extended double: 1) The first issue was the t-linux file forced the entire libgcc library to be compiled with the -mlong-double-128 option. 2) The second issue is that the GNU attribute #4 is set to reflect using 128-bit long doubles, and you get linker warnings when you use use the compiler, since libgcc_s.so indicates 128-bit IBM long doubles were used. I ran into a similar issue with my patches to extend libgcc to work if long doubles were configured to use the 128-bit IEEE format instead of the 128-bit IBM format. One feature of the current GNU attribute implementation is if you have a shared library (such as libgcc_s.so), the GNU attributes for the shared library is an inclusive OR of all of the modules within the library. This means if any module uses the -mlong-double-128 option and uses long double, the GNU attributes for the library will indicate that it uses 128-bit IBM long doubles. If you have a static library, you will get the warning only if you actually reference a module with the attribute set. This patch does two things: 1) Instead of compiling the whole library with -mlong-double-128, it only compiles the modules that process the IBM extended double format with this switch. It also specifies that these files must be compiled using the IBM format for long double. 2) I turned off GNU attributes for the whole library. Originally, I just turned off GNU attributes for just the modules that process IBM extended format values. But this doesn't work if the compiler defaults long double to 64-bits. What happens is the logic in rs6000.c that sets the GNU attribute bits, will set the bits for 64-bit long double if a normal double (DFmode) is used. So I just turned off the attributes for the whole library. This patch replaces the patch I previously did for IEEE 128-bit to turn off GNU attributes for just the ibm-ldouble.o module. https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556863.html I have tested this by building a compiler on a little endian power9 system running Linux with long double defaulting to 64-bits using the configure option: --without-long-double-128, and I verified that the warning no longer is generated by the linker. I then built a bootstrap compiler, by first building a non-bootstrap version. With that non-bootstrap compiler, I built versions of the MPC and MPFR. Using those libraries, and the non-bootstrap compiler as the host compiler, I was able to do a full bootstrap compiler. There are differences in the regression test suite where the test implicitly assumed long double was 128-bits or was a float128 test. Can I install this patch into the master branch? I would also like to install it in the GCC 10 branch after an appropriate period. libgcc/ 2020-10-28 Michael Meissner <meissner@linux.ibm.com> PR libgcc/97543 * config/rs6000/t-linux (HOST_LIBGCC2_CFLAGS): Don't set -mlong-double-128 for all modules. Instead set -mno-gnu-attributes. (IBM128_OBJS): New make variable for long double support. (IBM128_S_OBJS): New make variable for long double support. (IBM128_ALL_OBJS): New make variable for long double support. (IBM128_CFLAGS): New make variable for long double support. --- libgcc/config/rs6000/t-linux | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)