| Message ID | 974986b1-a571-460c-91a4-b166f2076d4c@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | libstdc++: Workaround glibc header on ia64-linux | expand |
On Mon, 30 Sept 2024 at 18:26, Frank Scheiner wrote: > > We see: > > ``` > FAIL: 17_intro/names.cc -std=gnu++17 (test for excess errors) > FAIL: 17_intro/names_pstl.cc -std=gnu++17 (test for excess errors) > FAIL: experimental/names.cc -std=gnu++17 (test for excess errors) > ``` > > ...on ia64-linux. > > This is due to: > > * /usr/include/bits/sigcontext.h:32-38: > ``` > 32 struct __ia64_fpreg > 33 { > 34 union > 35 { > 36 unsigned long bits[2]; > 37 } u; > 38 } __attribute__ ((__aligned__ (16))); > ``` > > * /usr/include/sys/ucontext.h:39-45: > ``` > 39 struct __ia64_fpreg_mcontext > 40 { > 41 union > 42 { > 43 unsigned long __ctx(bits)[2]; > 44 } __ctx(u); > 45 } __attribute__ ((__aligned__ (16))); > ``` > > ...from glibc 2.39 (w/ia64 support re-added). See the discussion > starting on [1]. > > [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654487.html > > The following patch adds a workaround for this on the libstdc++ > testsuite side. Thanks for the patch. Please CC libstdc++@gcc.gnu.org for all libstdc++ patches, as per https://gcc.gnu.org/lists.html It looks like the glibc header also defines "bits" without using the implementation namespace. Is there a reason the re-added ia64 code in the glibc header can't be fixed to use __bits and __u? Is there software expecting to access those fields directly? > > Signed-off-by: Frank Scheiner <frank.scheiner@web.de> > --- > libstdc++-v3/testsuite/17_intro/names.cc | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/libstdc++-v3/testsuite/17_intro/names.cc > b/libstdc++-v3/testsuite/17_intro/names.cc > index 9b0ffcb50b2..b45aefe1ccf 100644 > --- a/libstdc++-v3/testsuite/17_intro/names.cc > +++ b/libstdc++-v3/testsuite/17_intro/names.cc > @@ -265,6 +265,12 @@ > #undef j > #endif > > +#if defined (__linux__) && defined (__ia64__) > +// <bits/sigcontext.h> defines __ia64_fpreg::u > +// <sys/ucontext.h> defines __ia64_fpreg_mcontext::u > +#undef u > +#endif > + > #if defined (__linux__) && defined (__powerpc__) > // <asm/types.h> defines __vector128::u > #undef u > -- > 2.45.2 >
Hi Jonathan, On 01.10.24 11:28, Jonathan Wakely wrote: > On Mon, 30 Sept 2024 at 18:26, Frank Scheiner wrote: >> The following patch adds a workaround for this on the libstdc++ >> testsuite side. > > Thanks for the patch. Please CC libstdc++@gcc.gnu.org for all > libstdc++ patches, as per https://gcc.gnu.org/lists.html Thanks for the hint, will do. I also spotted a small typo in the title ("header" instead of "headers", as there are two glibc headers involved), so I'll better send a V2 anyways. > It looks like the glibc header also defines "bits" without using the > implementation namespace. Is there a reason the re-added ia64 code in > the glibc header can't be fixed to use __bits and __u? No, I think we can do that. As it only affects C++ you proposed in [1] to "just change the libstdc++ tests" though. [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654889.html > Is there > software expecting to access those fields directly? Frankly, I have no idea. But I can run it through our toolchain autobuilder (under CI on [2]) and in addition cross-build a T2 "base" package selection ([3]) for ia64 with the changes included for "bits" and "u" in the used glibc. The list of packages to be built is extensive (as it also includes the "bootstrap" and "toolchain" lists). If we don't spot a problem there, we might never do. Give me a few days for the latter, because I never did that so far and it'll require two runs to be able to compare the results. [2]: http://epic-linux.org/ [3]: http://svn.exactcode.de/t2/trunk/target/generic/pkgsel/20-base.in IIUC, if those changes in the glibc don't bite us, this patch here can be dropped again, right? OTOH it seems to be like that in the glibc since a while, so the gcc/libstdc++ patch could be kept for builds against older glibc versions anyway. But let's first see what will happen... Cheers, Frank
On Tue, 1 Oct 2024 at 14:05, Frank Scheiner <frank.scheiner@web.de> wrote: > > Hi Jonathan, > > On 01.10.24 11:28, Jonathan Wakely wrote: > > On Mon, 30 Sept 2024 at 18:26, Frank Scheiner wrote: > >> The following patch adds a workaround for this on the libstdc++ > >> testsuite side. > > > > Thanks for the patch. Please CC libstdc++@gcc.gnu.org for all > > libstdc++ patches, as per https://gcc.gnu.org/lists.html > > Thanks for the hint, will do. I also spotted a small typo in the title > ("header" instead of "headers", as there are two glibc headers > involved), so I'll better send a V2 anyways. > > > It looks like the glibc header also defines "bits" without using the > > implementation namespace. Is there a reason the re-added ia64 code in > > the glibc header can't be fixed to use __bits and __u? > > No, I think we can do that. As it only affects C++ you proposed in [1] > to "just change the libstdc++ tests" though. > > [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654889.html Ah yes. I think the sys/ucontext.h case only affects C++ (and C when _GNU_SOURCE is defined), but the sys/sigcontext.h case affects C too. Both 'u' and 'bits' are used without the __ctx macro. > > Is there > > software expecting to access those fields directly? > > Frankly, I have no idea. But I can run it through our toolchain > autobuilder (under CI on [2]) and in addition cross-build a T2 "base" > package selection ([3]) for ia64 with the changes included for "bits" > and "u" in the used glibc. The list of packages to be built is extensive > (as it also includes the "bootstrap" and "toolchain" lists). > > If we don't spot a problem there, we might never do. Give me a few days > for the latter, because I never did that so far and it'll require two > runs to be able to compare the results. > > [2]: http://epic-linux.org/ > > [3]: http://svn.exactcode.de/t2/trunk/target/generic/pkgsel/20-base.in > > IIUC, if those changes in the glibc don't bite us, this patch here can > be dropped again, right? OTOH it seems to be like that in the glibc > since a while, so the gcc/libstdc++ patch could be kept for builds > against older glibc versions anyway. But let's first see what will happen... I think we can change the libstdc++ test anyway, so it PASSes whether or not any change happens for glibc.
Hi Jonathan, Joseph, On 01.10.24 15:32, Jonathan Wakely wrote: > On Tue, 1 Oct 2024 at 14:05, Frank Scheiner <frank.scheiner@web.de> wrote: >> On 01.10.24 11:28, Jonathan Wakely wrote: >>> On Mon, 30 Sept 2024 at 18:26, Frank Scheiner wrote: >>> It looks like the glibc header also defines "bits" without using the >>> implementation namespace. Is there a reason the re-added ia64 code in >>> the glibc header can't be fixed to use __bits and __u? >> >> No, I think we can do that. As it only affects C++ you proposed in [1] >> to "just change the libstdc++ tests" though. >> >> [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654889.html > > Ah yes. I think the sys/ucontext.h case only affects C++ (and C when > _GNU_SOURCE is defined), but the sys/sigcontext.h case affects C too. > Both 'u' and 'bits' are used without the __ctx macro. I looked at the error messages for the tests ([2]) again and for 'u', even when used with the __ctx macro in "ucontext.h", it seems to be an issue: ``` [...] compiler exited with status 1 FAIL: 17_intro/names.cc -std=gnu++17 (test for excess errors) Excess errors: /usr/include/bits/sigcontext.h:37: error: expected unqualified-id before ';' token /usr/include/bits/sigcontext.h:37: error: expected ')' before ';' token /usr/include/sys/ucontext.h:44: error: expected unqualified-id before ';' token /usr/include/sys/ucontext.h:44: error: expected ')' before ';' token [...] ```by [2]: https://gcc.gnu.org/pipermail/gcc-patches/attachments/20240614/880a99c1/attachment-0001.md Though I don't understand why. From the error message it sounds like 'u' was replaced with '(' before the __ctx macro could do its job. But Joseph also wrote that it "prepends __ in standards conformance modes" in [3]. And this might not be the case for these specific tests, so the __ctx macro might not have any effect here. [3]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654883.html BTW, the two glibc headers are available here for reference: * https://github.com/linux-ia64/glibc-ia64/blob/release/2.39/master-w-ia64/sysdeps/unix/sysv/linux/ia64/bits/sigcontext.h * https://github.com/linux-ia64/glibc-ia64/blob/release/2.39/master-w-ia64/sysdeps/unix/sysv/linux/ia64/sys/ucontext.h ...names.cc here: https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/testsuite/17_intro/names.cc;h=9b0ffcb50b2e07a5ff2c2bd480b9eecd402c78d3;hb=HEAD >> IIUC, if those changes in the glibc don't bite us, this patch here can >> be dropped again, right? OTOH it seems to be like that in the glibc >> since a while, so the gcc/libstdc++ patch could be kept for builds >> against older glibc versions anyway. But let's first see what will happen... > > > I think we can change the libstdc++ test anyway, so it PASSes whether > or not any change happens for glibc. Agreed. Cheers, Frank
On Tue, 1 Oct 2024 at 16:53, Frank Scheiner <frank.scheiner@web.de> wrote: > > Hi Jonathan, Joseph, > > On 01.10.24 15:32, Jonathan Wakely wrote: > > On Tue, 1 Oct 2024 at 14:05, Frank Scheiner <frank.scheiner@web.de> wrote: > >> On 01.10.24 11:28, Jonathan Wakely wrote: > >>> On Mon, 30 Sept 2024 at 18:26, Frank Scheiner wrote: > >>> It looks like the glibc header also defines "bits" without using the > >>> implementation namespace. Is there a reason the re-added ia64 code in > >>> the glibc header can't be fixed to use __bits and __u? > >> > >> No, I think we can do that. As it only affects C++ you proposed in [1] > >> to "just change the libstdc++ tests" though. > >> > >> [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654889.html > > > > Ah yes. I think the sys/ucontext.h case only affects C++ (and C when > > _GNU_SOURCE is defined), but the sys/sigcontext.h case affects C too. > > Both 'u' and 'bits' are used without the __ctx macro. > > I looked at the error messages for the tests ([2]) again and for 'u', > even when used with the __ctx macro in "ucontext.h", it seems to be an > issue: > > ``` > [...] > compiler exited with status 1 > FAIL: 17_intro/names.cc -std=gnu++17 (test for excess errors) > Excess errors: > /usr/include/bits/sigcontext.h:37: error: expected unqualified-id before > ';' token > /usr/include/bits/sigcontext.h:37: error: expected ')' before ';' token > /usr/include/sys/ucontext.h:44: error: expected unqualified-id before > ';' token > /usr/include/sys/ucontext.h:44: error: expected ')' before ';' token > [...] > ```by > > [2]: > https://gcc.gnu.org/pipermail/gcc-patches/attachments/20240614/880a99c1/attachment-0001.md > > Though I don't understand why. From the error message it sounds like 'u' > was replaced with '(' before the __ctx macro could do its job. > > But Joseph also wrote that it "prepends __ in standards conformance > modes" in [3]. And this might not be the case for these specific tests, > so the __ctx macro might not have any effect here. It has no effect here. G++ unconditionally defines _GNU_SOURCE which means the __ctx macro does nothing. > > [3]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654883.html > > BTW, the two glibc headers are available here for reference: > > * > https://github.com/linux-ia64/glibc-ia64/blob/release/2.39/master-w-ia64/sysdeps/unix/sysv/linux/ia64/bits/sigcontext.h > > * > https://github.com/linux-ia64/glibc-ia64/blob/release/2.39/master-w-ia64/sysdeps/unix/sysv/linux/ia64/sys/ucontext.h > > ...names.cc here: > > https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/testsuite/17_intro/names.cc;h=9b0ffcb50b2e07a5ff2c2bd480b9eecd402c78d3;hb=HEAD > > >> IIUC, if those changes in the glibc don't bite us, this patch here can > >> be dropped again, right? OTOH it seems to be like that in the glibc > >> since a while, so the gcc/libstdc++ patch could be kept for builds > >> against older glibc versions anyway. But let's first see what will happen... > > > > > > I think we can change the libstdc++ test anyway, so it PASSes whether > > or not any change happens for glibc. > > Agreed. > > Cheers, > Frank >
On 01.10.24 18:02, Jonathan Wakely wrote: > On Tue, 1 Oct 2024 at 16:53, Frank Scheiner <frank.scheiner@web.de> wrote: >> Though I don't understand why. From the error message it sounds like 'u' >> was replaced with '(' before the __ctx macro could do its job. >> >> But Joseph also wrote that it "prepends __ in standards conformance >> modes" in [3]. And this might not be the case for these specific tests, >> so the __ctx macro might not have any effect here. > > It has no effect here. G++ unconditionally defines _GNU_SOURCE which > means the __ctx macro does nothing. Thanks for the clarification. I could retrace that via [1]. [1]: https://github.com/linux-ia64/glibc-ia64/blob/release/2.39/master-w-ia64/include/features.h#L201 In the meantime I also ran the build tests (cross-build a T2 "base" package selection) with and w/o modifications on the glibc side ('bits' => '__bits' and 'u' => '__u' in [2] and [3]) and couldn't find some obvious differences. [2]: https://github.com/linux-ia64/glibc-ia64/blob/master-epic/sysdeps/unix/sysv/linux/ia64/bits/sigcontext.h [3]: https://github.com/linux-ia64/glibc-ia64/blob/master-epic/sysdeps/unix/sysv/linux/ia64/sys/ucontext.h **** Jonathan, thanks for pointing me at these failing libstdc++ tests and their possible reasons. I'll send v2 of the patch right away. Cheers, Frank
diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc index 9b0ffcb50b2..b45aefe1ccf 100644 --- a/libstdc++-v3/testsuite/17_intro/names.cc +++ b/libstdc++-v3/testsuite/17_intro/names.cc @@ -265,6 +265,12 @@ #undef j #endif +#if defined (__linux__) && defined (__ia64__) +// <bits/sigcontext.h> defines __ia64_fpreg::u +// <sys/ucontext.h> defines __ia64_fpreg_mcontext::u +#undef u +#endif + #if defined (__linux__) && defined (__powerpc__) // <asm/types.h> defines __vector128::u #undef u
We see: ``` FAIL: 17_intro/names.cc -std=gnu++17 (test for excess errors) FAIL: 17_intro/names_pstl.cc -std=gnu++17 (test for excess errors) FAIL: experimental/names.cc -std=gnu++17 (test for excess errors) ``` ...on ia64-linux. This is due to: * /usr/include/bits/sigcontext.h:32-38: ``` 32 struct __ia64_fpreg 33 { 34 union 35 { 36 unsigned long bits[2]; 37 } u; 38 } __attribute__ ((__aligned__ (16))); ``` * /usr/include/sys/ucontext.h:39-45: ``` 39 struct __ia64_fpreg_mcontext 40 { 41 union 42 { 43 unsigned long __ctx(bits)[2]; 44 } __ctx(u); 45 } __attribute__ ((__aligned__ (16))); ``` ...from glibc 2.39 (w/ia64 support re-added). See the discussion starting on [1]. [1]: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654487.html The following patch adds a workaround for this on the libstdc++ testsuite side. Signed-off-by: Frank Scheiner <frank.scheiner@web.de> --- libstdc++-v3/testsuite/17_intro/names.cc | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.45.2