BUG #15844: MIPS: remove .set mips2 in s_lock.h to fix r6 build

On Tue, Jun 11, 2019 at 04:39:35AM +0000, PG Bug reporting form wrote:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.

This is quite architecture-specific. May I suggest to add this patch
to the follow-up commit fest for awareness:
https://commitfest.postgresql.org/23/

Do you have any references, like documentation, which summarize that?
--
Michael

Michael Paquier <michael@paquier.xyz> 于2019年6月11日周二 下午12:57写道：

On Tue, Jun 11, 2019 at 04:39:35AM +0000, PG Bug reporting form wrote:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.

This is quite architecture-specific. May I suggest to add this patch
to the follow-up commit fest for awareness:
https://commitfest.postgresql.org/23/

Do you have any references, like documentation, which summarize that?

https://s3-eu-west-1.amazonaws.com/downloads-mips/documents/MD00087-2B-MIPS64BIS-AFP-6.06.pdf
The page: 307

--
Michael

--
YunQiang Su

PG Bug reporting form <noreply@postgresql.org> writes:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.
This patch can fix this problem.

So I have a bunch of questions about this patch.

1. What problem is this actually solving? We have what I think are
reasonably modern MIPS machines in the buildfarm (frogfish,
topminnow) and they aren't complaining.

2. What toolchains define __mips_isa_rev at all? Seems like we could
be trading one portability issue for another one.

3. If we actually need something here, don't we need a run-time probe
rather than a compile-time check? Distro packagers, for example,
can't assume that their packages will be run on exactly the hardware
they build on.

regards, tom lane

PG Bug reporting form <noreply@postgresql.org> writes:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.
...
In fact if we not willing to support MIPS I or any CPU without ll/sc
at all, we can just remove .set mips2 here.

After further digging around, I'm liking the alternative of just
removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
and the MIPS-II instruction set has a lot of other substantial advantages
over MIPS-I besides having LL/SC, so it's pretty hard to believe that
anyone is still using toolchains that default to assuming MIPS-I
instruction set.

Digging around in our archives, it looks like ".set mips2" was required
when it was added, but that was on the strength of testing with a
machine running Linux 2.4.27-r5k-cobalt. We need to research when/if
Linux changed their default configuration.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月13日周四 下午10:07写道：

PG Bug reporting form <noreply@postgresql.org> writes:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.
This patch can fix this problem.

So I have a bunch of questions about this patch.

1. What problem is this actually solving? We have what I think are
reasonably modern MIPS machines in the buildfarm (frogfish,
topminnow) and they aren't complaining.

Yes. The MIPS r6 is quite new, and the machine that you have should be pre-r6.

2. What toolchains define __mips_isa_rev at all? Seems like we could
be trading one portability issue for another one.

It is defined by gcc/llvm.
For gcc:
https://github.com/gcc-mirror/gcc/blob/41d6b10e96a1de98e90a7c0378437c3255814b16/gcc/config/mips/mips.h#L527

3. If we actually need something here, don't we need a run-time probe
rather than a compile-time check? Distro packagers, for example,
can't assume that their packages will be run on exactly the hardware
they build on.

We don't think run-time probe is a good idea, since we treat r6 as a
new architectures.
It has it own port of OS, like Debian.
Runtime probe make none sense here.

regards, tom lane

--
YunQiang Su

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道：

PG Bug reporting form <noreply@postgresql.org> writes:

MIPS r6 changes the encoding of LL/SC instruction,
while the .set mips2 will force assembler to generate
old encoding.
...
In fact if we not willing to support MIPS I or any CPU without ll/sc
at all, we can just remove .set mips2 here.

After further digging around, I'm liking the alternative of just
removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
and the MIPS-II instruction set has a lot of other substantial advantages
over MIPS-I besides having LL/SC, so it's pretty hard to believe that
anyone is still using toolchains that default to assuming MIPS-I
instruction set.

You are right. I have no idea anyone is using MIPS I.

Digging around in our archives, it looks like ".set mips2" was required
when it was added, but that was on the strength of testing with a
machine running Linux 2.4.27-r5k-cobalt. We need to research when/if
Linux changed their default configuration.

It depends on the options to `as'.
If we build `as' without any option, and call it directly, it still
generate MIPS I
binary now.

While, as I know that we are using gcc to call `as', it won't be a
problem anymore.
Since no body set the default target of gcc as MIPS I nowdays.

regards, tom lane

--
YunQiang Su

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道：

After further digging around, I'm liking the alternative of just
removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
and the MIPS-II instruction set has a lot of other substantial advantages
over MIPS-I besides having LL/SC, so it's pretty hard to believe that
anyone is still using toolchains that default to assuming MIPS-I
instruction set.

You are right. I have no idea anyone is using MIPS I.

After further looking, it seems that isn't going to fly. I found from
the Debian release notes that they dropped MIPS-I support as of Stretch,
which means removing ".set mips2" would break both of our live MIPS
buildfarm machines (which run wheezy and jessie). I also found by
experimentation that NetBSD as of 7.0.2 doesn't default to assuming
MIPS2 either. (I didn't try anything newer, but a scan through the
port-mips mailing list found no suggestion that they've changed the
default since then.) So even though the hardware in use nowadays might
be fine with this, the toolchains are still behind the times.

So we'll have to go with the #if solution, I think. But I dislike
hardwiring "#if __mips_isa_rev >= 6" into s_lock.h. I'd suggest
modeling this hack on our rather-ancient hacks for similar problems
with PPC: put something like this into pg_config_manual.h

#if __mips_isa_rev >= 6
#define FORCE_MIPS2_ARCHITECTURE
#endif

(with a suitable comment) and then make s_lock.h do

#ifdef FORCE_MIPS2_ARCHITECTURE
" .set mips2 \n"
#endif

That'll make it a lot easier for people to tweak the condition
if they need to.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道：

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月14日周五 上午3:53写道：

After further digging around, I'm liking the alternative of just
removing the ".set mips2" lines. MIPS-I has been obsolete since 1989,
and the MIPS-II instruction set has a lot of other substantial advantages
over MIPS-I besides having LL/SC, so it's pretty hard to believe that
anyone is still using toolchains that default to assuming MIPS-I
instruction set.

You are right. I have no idea anyone is using MIPS I.

After further looking, it seems that isn't going to fly. I found from
the Debian release notes that they dropped MIPS-I support as of Stretch,
which means removing ".set mips2" would break both of our live MIPS
buildfarm machines (which run wheezy and jessie). I also found by

The default of wheezy and jessie is MIPS II.
https://metadata.ftp-master.debian.org/changelogs//main/g/gcc-4.8/gcc-4.8_4.8.4-1_changelog

Debian use MIPS II as default since gcc 4.5, (may be Squeeze)

experimentation that NetBSD as of 7.0.2 doesn't default to assuming

I have no idea about NetBSD. Can you run gcc -v on it?

MIPS2 either. (I didn't try anything newer, but a scan through the
port-mips mailing list found no suggestion that they've changed the
default since then.) So even though the hardware in use nowadays might
be fine with this, the toolchains are still behind the times.

So we'll have to go with the #if solution, I think. But I dislike
hardwiring "#if __mips_isa_rev >= 6" into s_lock.h. I'd suggest
modeling this hack on our rather-ancient hacks for similar problems
with PPC: put something like this into pg_config_manual.h

#if __mips_isa_rev >= 6
#define FORCE_MIPS2_ARCHITECTURE
#endif

If you'd like to do like this:
#if __mips_isa_rev > 0
is enough.

(with a suitable comment) and then make s_lock.h do

#ifdef FORCE_MIPS2_ARCHITECTURE
" .set mips2 \n"
#endif

That'll make it a lot easier for people to tweak the condition
if they need to.

regards, tom lane

--
YunQiang Su

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道：

experimentation that NetBSD as of 7.0.2 doesn't default to assuming

I have no idea about NetBSD. Can you run gcc -v on it?

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/lto-wrapper
Target: mipsel--netbsd
Configured with: /usr/7/src/tools/gcc/../../external/gpl3/gcc/dist/configure --target=mipsel--netbsd --enable-long-long --enable-threads --with-bugurl=http://www.NetBSD.org/Misc/send-pr.html --with-pkgversion='NetBSD nb2 20150115' --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-threads --enable-libstdcxx-time=rt --enable-lto --with-mpc-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpc/lib/libmpc --with-mpfr-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpfr/lib/libmpfr --with-gmp-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/gmp/lib/libgmp --with-mpc-include=/usr/7/src/external/lgpl3/mpc/dist/src --with-mpfr-include=/usr/7/src/external/lgpl3/mpfr/dist/src --with-gmp-include=/usr/7/src/external/lgpl3/gmp/lib/libgmp/arch/mipsel --enable-tls --disable-multilib --disable-symvers --disable-libstdcxx-pch --build=x86_64-unknown-netbsd6.0. --host=mipsel--netbsd --with-sysroot=/var/obj/mknative/pmax-mipsel/usr/7/src/destdir.pmax
Thread model: posix
gcc version 4.8.4 (nb2 20150115)

I found that specifying -march=mips2 gets it to accept the s_lock.h
code without ".set mips2". Given that we don't make any pretense of
actually running on MIPS-I hardware, I wonder if some hack involving
forcing -march would be sane? You'd get better code quality across
the board, presumably.

Also, checking the predefined symbols on this gcc, I don't see
anything about __mips_isa_rev, but I do see that "__mips" is defined
and -march=mips2 changes it from "1" to "2". So I'm wondering about
some test along the lines of

#if __mips_isa_rev > 0 || __mips == 1
#define FORCE_MIPS2_ARCHITECTURE
#endif

or alternatively, teach configure to force -march=mips2 if it sees
that "__mips" is predefined as 1.

(BTW, have you got any recommendations for booting recent Debian/MIPS
under qemu? I can't get anything newer than wheezy to work.)

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 下午12:28写道：

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 上午11:32写道：

experimentation that NetBSD as of 7.0.2 doesn't default to assuming

I have no idea about NetBSD. Can you run gcc -v on it?

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/lto-wrapper
Target: mipsel--netbsd
Configured with: /usr/7/src/tools/gcc/../../external/gpl3/gcc/dist/configure --target=mipsel--netbsd --enable-long-long --enable-threads --with-bugurl=http://www.NetBSD.org/Misc/send-pr.html --with-pkgversion='NetBSD nb2 20150115' --with-system-zlib --enable-__cxa_atexit --enable-libstdcxx-threads --enable-libstdcxx-time=rt --enable-lto --with-mpc-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpc/lib/libmpc --with-mpfr-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/mpfr/lib/libmpfr --with-gmp-lib=/var/obj/mknative/pmax-mipsel/usr/7/src/external/lgpl3/gmp/lib/libgmp --with-mpc-include=/usr/7/src/external/lgpl3/mpc/dist/src --with-mpfr-include=/usr/7/src/external/lgpl3/mpfr/dist/src --with-gmp-include=/usr/7/src/external/lgpl3/gmp/lib/libgmp/arch/mipsel --enable-tls --disable-multilib --disable-symvers --disable-libstdcxx-pch --build=x86_64-unknown-netbsd6.0. --host=mipsel--netbsd --with-sysroot=/var/obj/mknative/pmax-mipsel/usr/7/src/destdir.pmax
Thread model: posix
gcc version 4.8.4 (nb2 20150115)

I found that specifying -march=mips2 gets it to accept the s_lock.h
code without ".set mips2". Given that we don't make any pretense of
actually running on MIPS-I hardware, I wonder if some hack involving
forcing -march would be sane? You'd get better code quality across
the board, presumably.

-march is not a good idea, since r6 cannot compatible with the previous version.
If you use -march=mips2 or something else, it will failed to build for r6.
Which -march to use should be determined by finial user or distribution vendor.

If the vendor of NetBSD believe they have no need to support so old hardware,
they should change the options that they build their gcc.

For example build gcc with "--with-arch-32=mips32r2" or similar.

Also, checking the predefined symbols on this gcc, I don't see
anything about __mips_isa_rev, but I do see that "__mips" is defined
and -march=mips2 changes it from "1" to "2". So I'm wondering about
some test along the lines of

#if __mips_isa_rev > 0 || __mips == 1
#define FORCE_MIPS2_ARCHITECTURE
#endif

or alternatively, teach configure to force -march=mips2 if it sees
that "__mips" is predefined as 1.

If you wish to use __mips, you can just use
#if __mips > 1

(BTW, have you got any recommendations for booting recent Debian/MIPS
under qemu? I can't get anything newer than wheezy to work.)

I have an r6 one:
http://mips64el.bfsu.edu.cn/debian-new/tarball/qemu/
It is buster.

And I will figure out a r2 one for you.

regards, tom lane

--
YunQiang Su

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月16日周日 下午12:28写道：

I found that specifying -march=mips2 gets it to accept the s_lock.h
code without ".set mips2". Given that we don't make any pretense of
actually running on MIPS-I hardware, I wonder if some hack involving
forcing -march would be sane? You'd get better code quality across
the board, presumably.

-march is not a good idea, since r6 cannot compatible with the previous version.
If you use -march=mips2 or something else, it will failed to build for r6.
Which -march to use should be determined by finial user or distribution vendor.

Sure, the trick would be to not override a (default or specified)
architecture setting that's higher than mips2. I was imagining
using AC_EGREP_CPP() to see if __mips expands to exactly 1. But
on reflection, probably a better idea is to just see if asm with
ll/sc/sync compiles, and add -march=mips2 if not.

(We should not do anything if --disable-spinlocks has been specified,
btw, since then we don't need those asm commands to work. This is
and would remain the workaround for building PG for MIPS-I, if some
benighted soul insists on doing that.)

(BTW, have you got any recommendations for booting recent Debian/MIPS
under qemu? I can't get anything newer than wheezy to work.)

I have an r6 one:
http://mips64el.bfsu.edu.cn/debian-new/tarball/qemu/
It is buster.

Thanks for the pointer.

regards, tom lane

I wrote:

Sure, the trick would be to not override a (default or specified)
architecture setting that's higher than mips2. I was imagining
using AC_EGREP_CPP() to see if __mips expands to exactly 1. But
on reflection, probably a better idea is to just see if asm with
ll/sc/sync compiles, and add -march=mips2 if not.

Concretely, here's a patch that does it like that. I've verified
that this builds on my netbsd/gxemul installation.

regards, tom lane

Hi,

On 2019-06-16 12:16:53 -0400, Tom Lane wrote:

(We should not do anything if --disable-spinlocks has been specified,
btw, since then we don't need those asm commands to work. This is
and would remain the workaround for building PG for MIPS-I, if some
benighted soul insists on doing that.)

I think we're pretty much at the point where we should just rip out all
of our own spinlock implementations for non-common platforms, and solely
rely on compiler intrinsics... The open coded ASM really doesn't age
very well, and there's very little chance of us actually testing them
properly.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

I think we're pretty much at the point where we should just rip out all
of our own spinlock implementations for non-common platforms, and solely
rely on compiler intrinsics... The open coded ASM really doesn't age
very well, and there's very little chance of us actually testing them
properly.

It's completely not true that this code isn't tested; we have two
different MIPS buildfarm machines that surely exercise spinlocks plenty.

(There's a separate argument to be had about whether we should drop
source-code support for platforms that aren't represented in the
buildfarm. I'm not inclined to, but you could tenably hold that
position.)

As for compiler intrinsics, I dunno. I don't have very much faith in
the quality of those for non-mainstream platforms, either --- see
/messages/by-id/25414.1483076673@sss.pgh.pa.us
for a not-too-old example. And a lot of platforms like this are running
pretty old compilers, so even if the problems have been fixed it'll be a
long time before we can depend on that.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月18日周二 上午12:50写道：

Andres Freund <andres@anarazel.de> writes:

I think we're pretty much at the point where we should just rip out all
of our own spinlock implementations for non-common platforms, and solely
rely on compiler intrinsics... The open coded ASM really doesn't age
very well, and there's very little chance of us actually testing them
properly.

It's completely not true that this code isn't tested; we have two
different MIPS buildfarm machines that surely exercise spinlocks plenty.

Since NetBSD set the default ISA as MIPS I, and then we use ".set mips2",
in fact a bug.

I think that we should use some non-asm code for MIPS I ( our own
spinlock implementations?)

Do you have any NetBSD image that I can have a test of new patch?

(There's a separate argument to be had about whether we should drop
source-code support for platforms that aren't represented in the
buildfarm. I'm not inclined to, but you could tenably hold that
position.)

As for compiler intrinsics, I dunno. I don't have very much faith in
the quality of those for non-mainstream platforms, either --- see
/messages/by-id/25414.1483076673@sss.pgh.pa.us
for a not-too-old example. And a lot of platforms like this are running
pretty old compilers, so even if the problems have been fixed it'll be a
long time before we can depend on that.

regards, tom lane

--
YunQiang Su

YunQiang Su <wzssyqa@gmail.com> writes:

Since NetBSD set the default ISA as MIPS I, and then we use ".set mips2",
in fact a bug.
I think that we should use some non-asm code for MIPS I ( our own
spinlock implementations?)

You probably won't be surprised to hear that we keep having this
discussion ;-). Sure, we could essentially throw away all of s_lock.h in
favor of relying on __sync_lock_test_and_set() and __sync_lock_release().
But doing that would essentially abdicate having control over, or even
having much knowledge about, infrastructure that's absolutely critical
to the correctness and performance of Postgres. And we've seen a bunch
of cases where the compiler builtins are just not very well implemented
for non-mainstream architectures.

I got debian-mips-under-qemu working thanks to the files you sent a link
to, and I was curious to see what that gcc version would generate for
__sync_lock_test_and_set() and __sync_lock_release(), if I asked for
MIPS-I code. That's not the default of course, but "gcc -march=mips1
-mabi=32" seems to work. And what I get is

.set push
.set mips2
1:
ll $2,0($3)
li $1,1
sc $1,0($3)
beq $1,$0,1b
nop
sync
.set pop

and

.set push
.set mips2
sync
.set pop
sw $0,0($3)

So the gcc guys aren't any more wedded than we are to the rule that
one must not generate LL/SC on MIPS-I. They're probably assuming the
same thing we are, which is that if you're actually doing this on
MIPS-I then you'll get an instruction trap and the kernel will
emulate it for you. Any other choice would be completely disastrous
for performance on anything newer than MIPS-I ... and really, if you
are doing something that requires userland synchronization primitives,
you'd better not be using MIPS-I. It hasn't got them.

In short, moving to __sync_lock_test_and_set() would change basically
nothing on MIPS, but we'd no longer have any visibility into or control
over what it was doing. It would open us to other problems too -- for
example, if you try to apply __sync_lock_test_and_set() to a char
rather than an int, the code you get for that on MIPS is a whole lot
worse, but we'd have no visibility of that.

Do you have any NetBSD image that I can have a test of new patch?

I've not been able to get NetBSD/MIPS to work under qemu. It does
mostly work under gxemul, but that has so many bugs as to be nigh
useless --- the float arithmetic emulation is what usually gets me
when I try to do anything with Postgres.

regards, tom lane

I wrote:

So the gcc guys aren't any more wedded than we are to the rule that
one must not generate LL/SC on MIPS-I. They're probably assuming the
same thing we are, which is that if you're actually doing this on
MIPS-I then you'll get an instruction trap and the kernel will
emulate it for you.

Actually, after further contemplating that argument (which I was reminded
of by excavating in our commit log), I realize that having a .set mips2
in the ASM is actually the right way to do this, because that only results
in LL/SC getting emulated when we need them to be. Forcing -march=mips2
across the entire Postgres build would exceed our authority really ---
yeah, it might produce better code quality, but it's not our business to
override platform-level target architecture decisions.

So I now think your original proposal was about the best way to do this,
though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as
attached. It doesn't seem like forcing the ISA level down is ever
something we want to do, even if we know there's no practical difference.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月23日周日 上午6:37写道：

I wrote:

So the gcc guys aren't any more wedded than we are to the rule that
one must not generate LL/SC on MIPS-I. They're probably assuming the
same thing we are, which is that if you're actually doing this on
MIPS-I then you'll get an instruction trap and the kernel will
emulate it for you.

Actually, after further contemplating that argument (which I was reminded
of by excavating in our commit log), I realize that having a .set mips2
in the ASM is actually the right way to do this, because that only results
in LL/SC getting emulated when we need them to be. Forcing -march=mips2
across the entire Postgres build would exceed our authority really ---
yeah, it might produce better code quality, but it's not our business to
override platform-level target architecture decisions.

you are correct, that we should follow the target architecture
decisions of OS vendor,
if possible.

So I now think your original proposal was about the best way to do this,
though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as

here we should use #if __mips < 2 instead of __mips_isa_rev .
The history of MIPS revisions:

MIPS I : __mips=1 __mips_isa_rev = undef
MIPS II : __mips=2 __mips_isa_rev = undef
MIPS III : __mips=3 __mips_isa_rev = undef
MIPS IV : __mips=4 __mips_isa_rev = undef
MIPS 32/64(r1) : __mips=32/64 __mips_isa_rev = 1
MIPS 32/64 r2 : __mips=32/64 __mips_isa_rev = 2
MIPS 32/64 r3 : __mips=32/64 __mips_isa_rev = 3
MIPS 32/64 r5 : __mips=32/64 __mips_isa_rev = 5
MIPS 32/64 r6 : __mips=32/64 __mips_isa_rev = 6

attached. It doesn't seem like forcing the ISA level down is ever
something we want to do, even if we know there's no practical difference.

regards, tom lane

--
YunQiang Su

YunQiang Su <wzssyqa@gmail.com> writes:

Tom Lane <tgl@sss.pgh.pa.us> 于2019年6月23日周日 上午6:37写道：

So I now think your original proposal was about the best way to do this,
though I'd tweak it to make the test be "#if __mips_isa_rev < 2", as

here we should use #if __mips < 2 instead of __mips_isa_rev .

OK, will do.

regards, tom lane