Message ID | 20190904172219.15244-1-gabriel@inconstante.net.br |
---|---|
State | New |
Headers | show |
Series | math: Remove const attribute from totalorder* functions | expand |
On Wed, 4 Sep 2019, Gabriel F. T. Gomes wrote: > This commit removes the const attribute from the declarations of all > totalorder functions, since the pointed-to data is likely to be changed > by user code between invocations of totalorder*. This is OK, but it would be better to replace it with __attribute_pure__ (the patch with that replacement is also OK, if it works).
Hi, Joseph, On Wed, 04 Sep 2019, Joseph Myers wrote: >This is OK, but it would be better to replace it with __attribute_pure__ >(the patch with that replacement is also OK, if it works). Good point! I made this change, tested and pushed the following patch. Thanks! Gabriel From ab41100bab128fa98258aafbb0ab1622884cec4c Mon Sep 17 00:00:00 2001 From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> Date: Wed, 4 Sep 2019 13:36:23 -0300 Subject: [PATCH] math: Replace const attribute with pure in totalorder* functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the commit commit 42760d764649ad82f5fe45a26cbdf2c2500409f7 Author: Joseph Myers <joseph@codesourcery.com> Date: Thu Aug 15 15:18:34 2019 +0000 Make totalorder and totalordermag functions take pointer arguments. the test case math/test-totalorderl-ldbl-128ibm fails on every input pair, when compiled with -O2, which is the case for glibc test suite. Debugging showed that the test case is passing arguments incorrectly to totalorderl. This can also be inferred by the fact that compiling the test case with -O0 hides the bug. The documentation for the const attribute in GCC manual reads: Note that a function that has pointer arguments and examines the data pointed to must not be declared const if the pointed-to data might change between successive invocations of the function. In general, since a function cannot distinguish data that might change from data that cannot, const functions should never take pointer or, in C++, reference arguments. Likewise, a function that calls a non-const function usually must not be const itself. Since the pointed-to data is likely to be changed by user code between invocations of totalorder*, this patch removes the const attribute from the declarations of all totalorder functions, replacing it with the pure attribute, as suggested in the manual: The pure attribute imposes similar but looser restrictions on a function’s definition than the const attribute: pure allows the function to read any non-volatile memory, even if it changes in between successive invocations of the function. Tested for powerpc64le and x86_64. --- ChangeLog | 5 +++++ math/bits/mathcalls.h | 7 ++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/ChangeLog b/ChangeLog index 68aa879048..8fb6171ce5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2019-09-05 Gabriel F. T. Gomes <gabrielftg@linux.ibm.com> + + * math/bits/mathcalls.h (totalorder, totalordermag): Replace + const attribute with pure attribute. + 2019-09-04 Lukasz Majewski <lukma@denx.de> * sysdeps/unix/sysv/linux/kernel-features.h diff --git a/math/bits/mathcalls.h b/math/bits/mathcalls.h index 66832c7827..94690c3b42 100644 --- a/math/bits/mathcalls.h +++ b/math/bits/mathcalls.h @@ -373,13 +373,14 @@ __MATHDECL_1 (int, canonicalize,, (_Mdouble_ *__cx, const _Mdouble_ *__x)); #if __GLIBC_USE (IEC_60559_BFP_EXT) || __MATH_DECLARING_FLOATN /* Total order operation. */ -__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, const _Mdouble_ *__y)) - __attribute__ ((__const__)); +__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, + const _Mdouble_ *__y)) + __attribute__ ((__pure__)); /* Total order operation on absolute values. */ __MATHDECL_1 (int, totalordermag,, (const _Mdouble_ *__x, const _Mdouble_ *__y)) - __attribute__ ((__const__)); + __attribute__ ((__pure__)); /* Get NaN payload. */ __MATHCALL (getpayload,, (const _Mdouble_ *__x));
On Thu, 05 Sep 2019, Gabriel F. T. Gomes wrote: >On Wed, 04 Sep 2019, Joseph Myers wrote: > >>This is OK, but it would be better to replace it with __attribute_pure__ ~~~~~~~~~~~~~~~~~~ Oh! Only upon reading my own reply, I noticed the suggestion to use this macro, instead of __attribute__ ((__pure__)). Should I go ahead and patch it again... >+__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, >+ const _Mdouble_ *__y)) >+ __attribute__ ((__pure__)); ... replacing '__attribute__ ((__pure__))' with '__attribute_pure__'? > __MATHDECL_1 (int, totalordermag,, (const _Mdouble_ *__x, > const _Mdouble_ *__y)) >- __attribute__ ((__const__)); >+ __attribute__ ((__pure__)); Likewise, here? Or it doesn't matter? Sorry for the oversight.
On Thu, 5 Sep 2019, Gabriel F. T. Gomes wrote: > On Thu, 05 Sep 2019, Gabriel F. T. Gomes wrote: > > >On Wed, 04 Sep 2019, Joseph Myers wrote: > > > >>This is OK, but it would be better to replace it with __attribute_pure__ > ~~~~~~~~~~~~~~~~~~ > Oh! Only upon reading my own reply, I noticed the suggestion to use this > macro, instead of __attribute__ ((__pure__)). Should I go ahead and patch > it again... > > >+__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, > >+ const _Mdouble_ *__y)) > >+ __attribute__ ((__pure__)); > > ... replacing '__attribute__ ((__pure__))' with '__attribute_pure__'? I think it should use __attribute_pure__, yes (although quite likely using the older compilers for which they differ would show up other problems with current glibc headers).
Hi, Joseph, On Thu, 05 Sep 2019, Joseph Myers wrote: >I think it should use __attribute_pure__, yes (although quite likely using >the older compilers for which they differ would show up other problems >with current glibc headers). I committed the follow-up patch below with this fix. As I mentioned in the commit message, all other occurrences of attribute *pure* in glibc use the macro. This is not true for the occurrences of attribute *const* in glibc, which happen both with and without the macro, but based on your comment between parentheses above, I suppose it doesn't matter that much (let me know if I'm wrong, then I can write a similar cleanup (around 60 lines changes)). Thanks, Gabriel From 1b7f04070bd94f259e2ed24d6fb76309d64fb164 Mon Sep 17 00:00:00 2001 From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> Date: Thu, 5 Sep 2019 17:06:03 -0300 Subject: [PATCH] Use __attribute_pure__ macro in bits/mathcalls.h When the const attribute of totalorder* functions was replaced with the pure attribute, by commit ID ab41100bab12, it did not use the __attribute_pure__ macro, but the __attribute__ ((__pure__)) statement. All other uses of the pure attribute in glibc use the macro. Tested for powerpc64le. --- math/bits/mathcalls.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/math/bits/mathcalls.h b/math/bits/mathcalls.h index 94690c3b42..4703b28fee 100644 --- a/math/bits/mathcalls.h +++ b/math/bits/mathcalls.h @@ -375,12 +375,12 @@ __MATHDECL_1 (int, canonicalize,, (_Mdouble_ *__cx, const _Mdouble_ *__x)); /* Total order operation. */ __MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, const _Mdouble_ *__y)) - __attribute__ ((__pure__)); + __attribute_pure__; /* Total order operation on absolute values. */ __MATHDECL_1 (int, totalordermag,, (const _Mdouble_ *__x, const _Mdouble_ *__y)) - __attribute__ ((__pure__)); + __attribute_pure__; /* Get NaN payload. */ __MATHCALL (getpayload,, (const _Mdouble_ *__x));
diff --git a/math/bits/mathcalls.h b/math/bits/mathcalls.h index 66832c7827..1361b55fb6 100644 --- a/math/bits/mathcalls.h +++ b/math/bits/mathcalls.h @@ -373,13 +373,12 @@ __MATHDECL_1 (int, canonicalize,, (_Mdouble_ *__cx, const _Mdouble_ *__x)); #if __GLIBC_USE (IEC_60559_BFP_EXT) || __MATH_DECLARING_FLOATN /* Total order operation. */ -__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, const _Mdouble_ *__y)) - __attribute__ ((__const__)); +__MATHDECL_1 (int, totalorder,, (const _Mdouble_ *__x, + const _Mdouble_ *__y)); /* Total order operation on absolute values. */ __MATHDECL_1 (int, totalordermag,, (const _Mdouble_ *__x, - const _Mdouble_ *__y)) - __attribute__ ((__const__)); + const _Mdouble_ *__y)); /* Get NaN payload. */ __MATHCALL (getpayload,, (const _Mdouble_ *__x));
From: "Gabriel F. T. Gomes" <gabrielftg@linux.ibm.com> NOTE: If this is not a bug in glibc, then it might be a bug in gcc (I tested with Debian's gcc and with the toolchain generated by build-many-glibcs with top of the tree binutils, gcc, and glibc as of yesterday). -- 8< -- Since the commit commit 42760d764649ad82f5fe45a26cbdf2c2500409f7 Author: Joseph Myers <joseph@codesourcery.com> Date: Thu Aug 15 15:18:34 2019 +0000 Make totalorder and totalordermag functions take pointer arguments. the test case math/test-totalorderl-ldbl-128ibm fails on every input pair, when compiled with -O2, which is the case for glibc test suite. Debugging showed that the test case is passing arguments incorrectly to totalorderl. This can also be inferred by the fact that compiling the test case with -O0 hides the bug. The documentation for the const attribute in GCC manual reads: Note that a function that has pointer arguments and examines the data pointed to must not be declared const if the pointed-to data might change between successive invocations of the function. In general, since a function cannot distinguish data that might change from data that cannot, const functions should never take pointer or, in C++, reference arguments. Likewise, a function that calls a non-const function usually must not be const itself. This commit removes the const attribute from the declarations of all totalorder functions, since the pointed-to data is likely to be changed by user code between invocations of totalorder*. Tested for powerpc64le and x86_64. * math/bits/mathcalls.h (totalorder, totalordermag): Remove const attribute. --- math/bits/mathcalls.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)