MIPS: Use `.set arch=mips2' to emulate LL/SC for the R5900 too
Commit Message
`.set arch=' was implemented in Binutils by Thiemo Seufer 29 June 2003 in
commit fef14a42892a297bf, and the GNU C Library requires Binutils 2.25,
from 2014, or later, so it ought to be compatible as used here.
This change has been tested by compiling the GNU C Library 2.27 with a
GCC 8.2.0 cross-compiler for mipsr5900el-unknown-linux-gnu under Gentoo.
---
sysdeps/mips/sys/tas.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Hi Fredrik,
> `.set arch=' was implemented in Binutils by Thiemo Seufer 29 June 2003 in
> commit fef14a42892a297bf, and the GNU C Library requires Binutils 2.25,
> from 2014, or later, so it ought to be compatible as used here.
>
> This change has been tested by compiling the GNU C Library 2.27 with a
> GCC 8.2.0 cross-compiler for mipsr5900el-unknown-linux-gnu under Gentoo.
Thank you for your contribution.
This change looks good if not obvious to me, however I think it would
make sense if you mentioned in the change description why you actually
need this to build for the R5900, i.e. quoted the lack of hardware LL/SC
instructions in the R5900 despite it matching `__mips >= 2', the build
errors you've still seen with `.set mips2', and the analysis of their
cause we have made off-list (which would serve as an explanation of the
difference between `.set mips2' and `.set arch=mips2', which is subtle
enough for someone coming across this change to wonder).
NB this change is legally insignificant enough WRT copyright to be
applied right away once we have consensus as per
<https://sourceware.org/glibc/wiki/Consensus> even while your paperwork
with FSF is (still) in progress.
Maciej
On Sat, 27 Oct 2018, Maciej W. Rozycki wrote:
> This change looks good if not obvious to me, however I think it would
> make sense if you mentioned in the change description why you actually
> need this to build for the R5900, i.e. quoted the lack of hardware LL/SC
> instructions in the R5900 despite it matching `__mips >= 2', the build
> errors you've still seen with `.set mips2', and the analysis of their
> cause we have made off-list (which would serve as an explanation of the
> difference between `.set mips2' and `.set arch=mips2', which is subtle
> enough for someone coming across this change to wonder).
I think an explanation of the need for the "|| defined(_MIPS_ARCH_R5900)"
(which is missing a space before the '(' as per GNU style) should go in a
comment in the source code, not just the commit message.
@@ -41,8 +41,8 @@ __NTH (_test_and_set (int *__p, int __v))
__asm__ __volatile__
("/* Inline test and set */\n"
".set push\n\t"
-#if _MIPS_SIM == _ABIO32 && __mips < 2
- ".set mips2\n\t"
+#if _MIPS_SIM == _ABIO32 && (__mips < 2 || defined(_MIPS_ARCH_R5900))
+ ".set arch=mips2\n\t"
#endif
"sync\n\t"
"1:\n\t"