Go patch committed: Intrinsify runtime/internal/atomic functions

[Ian Lance Taylor]

This patch to the Go frontend by Cherry Zhang intrinsifies the
runtime/internal/atomic functions. Currently the
runtime/internal/atomic functions are implemented in C using C
compiler intrinsics. This patch lets the Go frontend recognize these
functions and turn them into intrinsics directly. Bootstrapped and
ran Go testsuite on x86_64-pc-linux-gnu. Committed to mainline.

Ian

2019-05-16 Cherry Zhang <cher...@google.com>

* go-gcc.cc (Gcc_backend::Gcc_backend): Define atomic builtins.

[Andreas Schwab]

I'm getting this crash on riscv:

/daten/riscv64/gcc/gcc-20190518/Build/./gcc/gccgo -B/daten/riscv64/gcc/gcc-20190518/Build/./gcc/ -B/usr/riscv64-suse-linux/bin/ -B/usr/riscv64-suse-linux/lib/ -isystem /usr/riscv64-suse-linux/include -isystem /usr/riscv64-suse-linux/sys-include -O2 -g -I . -c -fgo-pkgpath=runtime -fgo-c-header=runtime.inc.raw -fgo-compiling-runtime ../../../libgo/go/runtime/alg.go ../../../libgo/go/runtime/atomic_pointer.go ../../../libgo/go/runtime/cgo_gccgo.go ../../../libgo/go/runtime/cgocall.go ../../../libgo/go/runtime/cgocheck.go ../../../libgo/go/runtime/chan.go ../../../libgo/go/runtime/compiler.go ../../../libgo/go/runtime/cpuprof.go ../../../libgo/go/runtime/cputicks.go ../../../libgo/go/runtime/debug.go ../../../libgo/go/runtime/env_posix.go ../../../libgo/go/runtime/error.go ../../../libgo/go/runtime/extern.go ../../../libgo/go/runtime/fastlog2.go ../../../libgo/go/runtime/fastlog2table.go ../../../libgo/go/runtime/ffi.go ../../../libgo/go/runtime/float.go ../../../libgo/go/runtime/hash64.go ../../../libgo/go/runtime/heapdump.go ../../../libgo/go/runtime/iface.go ../../../libgo/go/runtime/lfstack.go ../../../libgo/go/runtime/lfstack_64bit.go ../../../libgo/go/runtime/lock_futex.go ../../../libgo/go/runtime/malloc.go ../../../libgo/go/runtime/map.go ../../../libgo/go/runtime/map_fast32.go ../../../libgo/go/runtime/map_fast64.go ../../../libgo/go/runtime/map_faststr.go ../../../libgo/go/runtime/mbarrier.go ../../../libgo/go/runtime/mbitmap.go ../../../libgo/go/runtime/mcache.go ../../../libgo/go/runtime/mcentral.go ../../../libgo/go/runtime/mem_gccgo.go ../../../libgo/go/runtime/mfinal.go ../../../libgo/go/runtime/mfixalloc.go ../../../libgo/go/runtime/mgc.go ../../../libgo/go/runtime/mgc_gccgo.go ../../../libgo/go/runtime/mgclarge.go ../../../libgo/go/runtime/mgcmark.go ../../../libgo/go/runtime/mgcsweep.go ../../../libgo/go/runtime/mgcsweepbuf.go ../../../libgo/go/runtime/mgcwork.go ../../../libgo/go/runtime/mheap.go ../../../libgo/go/runtime/mprof.go ../../../libgo/go/runtime/msan0.go ../../../libgo/go/runtime/msize.go ../../../libgo/go/runtime/mstats.go ../../../libgo/go/runtime/mwbbuf.go ../../../libgo/go/runtime/netpoll.go ../../../libgo/go/runtime/netpoll_epoll.go ../../../libgo/go/runtime/os_gccgo.go ../../../libgo/go/runtime/os_linux.go ../../../libgo/go/runtime/os_linux_noauxv.go ../../../libgo/go/runtime/panic.go ../../../libgo/go/runtime/print.go ../../../libgo/go/runtime/proc.go ../../../libgo/go/runtime/profbuf.go ../../../libgo/go/runtime/proflabel.go ../../../libgo/go/runtime/race0.go ../../../libgo/go/runtime/rdebug.go ../../../libgo/go/runtime/relax_stub.go ../../../libgo/go/runtime/runtime.go ../../../libgo/go/runtime/runtime1.go ../../../libgo/go/runtime/runtime2.go ../../../libgo/go/runtime/rwmutex.go ../../../libgo/go/runtime/select.go ../../../libgo/go/runtime/sema.go ../../../libgo/go/runtime/signal_gccgo.go ../../../libgo/go/runtime/signal_sighandler.go ../../../libgo/go/runtime/signal_unix.go ../../../libgo/go/runtime/sigqueue.go ../../../libgo/go/runtime/sizeclasses.go ../../../libgo/go/runtime/slice.go ../../../libgo/go/runtime/string.go ../../../libgo/go/runtime/stubs.go ../../../libgo/go/runtime/stubs2.go ../../../libgo/go/runtime/stubs3.go ../../../libgo/go/runtime/stubs_linux.go ../../../libgo/go/runtime/symtab.go ../../../libgo/go/runtime/time.go ../../../libgo/go/runtime/timestub.go ../../../libgo/go/runtime/timestub2.go ../../../libgo/go/runtime/trace.go ../../../libgo/go/runtime/traceback_gccgo.go ../../../libgo/go/runtime/type.go ../../../libgo/go/runtime/typekind.go ../../../libgo/go/runtime/unaligned1.go ../../../libgo/go/runtime/utf8.go ../../../libgo/go/runtime/write_err.go runtime_sysinfo.go sigtab.go -fPIC -o .libs/runtime.o
during RTL pass: expand
../../../libgo/go/runtime/mbitmap.go: In function ‘runtime.setMarked.runtime.markBits’:
../../../libgo/go/runtime/mbitmap.go:291:9: internal compiler error: Segmentation fault
291 | atomic.Or8(m.bytep, m.mask)
| ^
0x6a02b7 crash_signal
../../gcc/toplev.c:326
0x917cf6 get_callee_fndecl(tree_node const*)
../../gcc/tree.c:9570
0x2c2e6b expand_call(tree_node*, rtx_def*, int)
../../gcc/calls.c:3364
0x2aa3e9 expand_builtin_atomic_fetch_op
../../gcc/builtins.c:6541
0x2b5981 expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode, int)
../../gcc/builtins.c:8220
0x3bdfef expand_expr_real_1(tree_node*, rtx_def*, machine_mode, expand_modifier, rtx_def**, bool)
../../gcc/expr.c:11030
0x2d4ee5 expand_expr
../../gcc/expr.h:279
0x2d4ee5 expand_call_stmt
../../gcc/cfgexpand.c:2724
0x2d4ee5 expand_gimple_stmt_1
../../gcc/cfgexpand.c:3700
0x2d5847 expand_gimple_stmt
../../gcc/cfgexpand.c:3859
0x2da083 expand_gimple_basic_block
../../gcc/cfgexpand.c:5895
0x2dbff3 execute
../../gcc/cfgexpand.c:6518

Andreas.

--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[Jim Wilson]

On Sun, May 19, 2019 at 5:22 AM Andreas Schwab <sch...@linux-m68k.org> wrote:
> ../../../libgo/go/runtime/mbitmap.go: In function ‘runtime.setMarked.runtime.markBits’:
> ../../../libgo/go/runtime/mbitmap.go:291:9: internal compiler error: Segmentation fault
> 291 | atomic.Or8(m.bytep, m.mask)
> | ^

This is failing for RISC-V because __atomic_or_fetch_1 isn't a
built-in function that can be expanded inline. You have to call the
library function in libatomic. The C front-end is registering all of
the built-in functions, but it looks like the go front-end is only
registering functions it thinks it needs and this list is incomplete.
In expand_builtin, case BUILT_IN_ATOMIC_OR_FETCH_1, the external
library call for this gets set to BUILT_IN_ATOMIC_FETCH_OR_1. Then in
expand_builtin_atomic_fetch_op when we call builtin_decl_explicit
(ext_call) it returns NULL. This is because the go front end
registered BUILT_IN_ATOMIC_OR_FETCH_1 as a built-in, but did not
register BUILT_IN_ATOMIC_FETCH_OR_1 as a built-in. The NULL return
from builtin_decl_explicit gives us an ADDR_EXPR with a NULL operand
which eventually causes the internal compiler error. It looks like
the same thing is done with all of the op_fetch built-ins, so use of
any of them means that the fetch_op built-in also has to be
registered. I verified with a quick hack that I need both
BUILT_IN_ATOMIC_FETCH_OR_1 and BUILT_IN_ATOMIC_FETCH_AND_1 defined as
built-ins to make a RISC-V go build work. I haven't done any testing
yet.

Jim

[Cherry Zhang]

Jim, thank you for the fix! The patch looks good to me. Maybe we should also apply this to __atomic_add_fetch_4 and __atomic_add_fetch_8?

Thanks,

Cherry

--
You received this message because you are subscribed to the Google Groups "gofrontend-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to gofrontend-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/gofrontend-dev/CAFyWVaY8aMmhmnWpoGdywaeEhiXmSUe8qqha%2BHhvbxYHhUnisQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Ian Lance Taylor]

Jim, you can go ahead and commit that patch with a ChangeLog entry.
(The files in go/gcc but not in go/gcc/gofrontend) are under normal
GCC rules.) Thanks.

Ian

[Andreas Schwab]

Here are the test results:
http://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02903.html

[Jim Wilson]

On Wed, May 22, 2019 at 6:41 PM Ian Lance Taylor <ia...@golang.org> wrote:
> Jim, you can go ahead and commit that patch with a ChangeLog entry.
> (The files in go/gcc but not in go/gcc/gofrontend) are under normal
> GCC rules.) Thanks.

OK. Committed. I did an x86_64-linux go build and check to make sure
I didn't break that.

Jim

[Jim Wilson]

On Wed, May 22, 2019 at 6:36 PM Cherry Zhang <cher...@google.com> wrote:
> Jim, thank you for the fix! The patch looks good to me. Maybe we should also apply this to __atomic_add_fetch_4 and __atomic_add_fetch_8?

Sorry about the delay, I caught a virus last week and am behind on everything.

For 64-bit RISC-V those are open coded and hence not a problems. All
of the major targets open code everything except RISC-V which needs a
library call for the 1 and 2 byte atomics, though this is on our list
of things to fix. We just haven't gotten around to it yet. For
32-bit RISC-V, the 8-byte atomic would require a library call, but
32-bit riscv-linux isn't formally supported, as the glibc patches are
not upstream yet, so this isn't a practical problem yet.

Anyways, yes, in theory, we should be handling these two also, but
there probably aren't any well supported targets that will fail if
they are missing.

Jim

[Jim Wilson]

On Thu, May 30, 2019 at 11:11 AM Andreas Schwab <sch...@linux-m68k.org> wrote:
> Here are the test results:
> http://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02903.html

I tried running RISC-V go checks on my system. I see 7 unexpected
failures for gcc check-go, 6 unexpected failures for check-gotools,
and 1 unexpected failure for check-target-libgo. I haven't tried
building and testing the go support before so I don't have any old
results to compare against. It seems reasonable for a first attempt
though.

Jim

[Maciej Rozycki]

On Thu, 30 May 2019, Jim Wilson wrote:

> > Here are the test results:
> > http://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02903.html
>
> I tried running RISC-V go checks on my system. I see 7 unexpected
> failures for gcc check-go, 6 unexpected failures for check-gotools,
> and 1 unexpected failure for check-target-libgo. I haven't tried
> building and testing the go support before so I don't have any old
> results to compare against. It seems reasonable for a first attempt
> though.

I have results as at r270765, taken with QEMU run in the user emulation
mode (I have patches outstanding for configury/Makefiles to correctly
support libgo and other library testing with a build sysroot in such a
setup, pending the completion of my WDC copyright assignment paperwork
with FSF):

FAIL: go.test/test/args.go execution, -O2 -g
FAIL: go.test/test/fixedbugs/bug347.go execution, -O0 -g
FAIL: go.test/test/fixedbugs/bug348.go execution, -O0 -g
FAIL: go.test/test/fixedbugs/issue4562.go execution, -O2 -g
FAIL: go.test/test/fixedbugs/issue6055.go execution, -O2 -g
FAIL: go.test/test/method5.go execution, -O2 -g
FAIL: go.test/test/nil.go execution, -O2 -g
FAIL: go.test/test/recover3.go execution, -O2 -g

=== go Summary ===

# of expected passes 3188
# of unexpected failures 8
# of expected failures 1
# of untested testcases 32
# of unsupported tests 3

I don't know why the number of passes (effectively # of tests run) is
reduced by more than a half with otherwise similar figures; it could be
due to the use of a simulator vs native test setup.

For some reason I haven't looked into libgo tests are deliberatly run
individually and results, concatenated into libgo-all.sum, have to be
processed manually to get a summary. Here's one I have come up with:

WARNING: program timed out.
FAIL: archive/zip
FAIL: cmd/go/internal/lockedfile/internal/filelock
FAIL: compress/flate
FAIL: compress/lzw
FAIL: compress/zlib
WARNING: program timed out.
FAIL: crypto/dsa
FAIL: crypto/tls
FAIL: database/sql
FAIL: html/template
FAIL: image/draw
FAIL: image/jpeg
FAIL: internal/cpu
FAIL: internal/x/crypto/cryptobyte
FAIL: internal/x/net/http/httpproxy
FAIL: log/syslog
FAIL: net
FAIL: net/http
FAIL: net/http/cgi
FAIL: net/http/internal
FAIL: net/http/pprof
FAIL: os
FAIL: os/exec
FAIL: os/signal
FAIL: reflect
WARNING: program timed out.
FAIL: regexp
WARNING: program timed out.
FAIL: runtime
FAIL: runtime/pprof
FAIL: sync
FAIL: sync/atomic
FAIL: syscall
FAIL: text/template
FAIL: time

=== libgo Summary ===

# of expected passes 136
# of unexpected failures 32

These are significantly worse, but I suspect the use of the user emulation
mode of QEMU may have contributed here.

No gotools tests run in my setup for some reason:

make[2]: Entering directory '.../gcc/gotools'
make[2]: Nothing to be done for 'check'.
make[2]: Leaving directory '.../gcc/gotools'

-- maybe this is not supported with a cross-toolchain.

I have current test results (also for the other frontends/libraries, e.g.
GNAT) with the same setup as well in case someone is interested. There
are minor differences.

Maciej

[Ian Lance Taylor]

On Mon, Jun 3, 2019 at 7:12 AM Maciej Rozycki <ma...@wdc.com> wrote:
>
> On Thu, 30 May 2019, Jim Wilson wrote:
>
> > > Here are the test results:
> > > http://gcc.gnu.org/ml/gcc-testresults/2019-05/msg02903.html
> >
> > I tried running RISC-V go checks on my system. I see 7 unexpected
> > failures for gcc check-go, 6 unexpected failures for check-gotools,
> > and 1 unexpected failure for check-target-libgo. I haven't tried
> > building and testing the go support before so I don't have any old
> > results to compare against. It seems reasonable for a first attempt
> > though.
>
> I have results as at r270765, taken with QEMU run in the user emulation
> mode (I have patches outstanding for configury/Makefiles to correctly
> support libgo and other library testing with a build sysroot in such a
> setup, pending the completion of my WDC copyright assignment paperwork
> with FSF):
>
> FAIL: go.test/test/args.go execution, -O2 -g

That is a fairly trivial test so if that one fails then something is
pretty fundamentally wrong somewhere.

You can run just that test by running

make RUNTESTFLAGS=go-test.exp=args.go check-gcc-go

If it fails you should have an executable gcc/testsuite/go/args.x.
Run that executable as `args.x arg1 arg2`.

Ian

[Andreas Schwab]

On Jun 03 2019, Maciej Rozycki <ma...@wdc.com> wrote:

> These are significantly worse, but I suspect the use of the user emulation
> mode of QEMU may have contributed here.

I think all your failures are due to bugs/limitations in the qemu
emulation.

[Maciej Rozycki]

On Mon, 3 Jun 2019, Ian Lance Taylor wrote:

> > I have results as at r270765, taken with QEMU run in the user emulation
> > mode (I have patches outstanding for configury/Makefiles to correctly
> > support libgo and other library testing with a build sysroot in such a
> > setup, pending the completion of my WDC copyright assignment paperwork
> > with FSF):
> >
> > FAIL: go.test/test/args.go execution, -O2 -g
>
> That is a fairly trivial test so if that one fails then something is
> pretty fundamentally wrong somewhere.
>
> You can run just that test by running
>
> make RUNTESTFLAGS=go-test.exp=args.go check-gcc-go
>
> If it fails you should have an executable gcc/testsuite/go/args.x.

The .log file reports (leading paths edited throughout):

spawn qemu-riscv64 -E LD_LIBRARY_PATH=.:.../riscv64-linux-gnu/lib64/lp64d/libgo/.libs:.../gcc:.../gcc/lib64/lp64:.../gcc/lib64/lp64d .../gcc/testsuite/go/args.x
panic: argc

goroutine 1 [running]:
main.main
.../gcc/testsuite/go.test/test/args.go:18

FAIL: go.test/test/args.go execution, -O2 -g

The same happens when this is invoked manually.

> Run that executable as `args.x arg1 arg2`.

That works however. I've modified `sim_spawn', which is declared as:

proc sim_spawn { dest cmdline args }

by adding this line:

clone_output "sim_spawn $dest $cmdline $args"

and it reports no command-line arguments being passed to the test case
from the test harness:

sim_spawn qemu-user-lp64d .../gcc/testsuite/go/args.x

so I gather there is something wrong at an upper level. I'll see if I can
investigate it at one point, but given that it is probably the only test
case that expects arguments I consider it a very low priority issue.
Also not all simulators may support arguments in the first place,
especially for bare metal.

NB I had to make a local copy of `sim_spawn' in the qemu-user-lp64d.exp
board description file used, to get `-E ...' passed to QEMU with the
LD_LIBRARY_PATH setting for uninstalled shared libraries to work. That
does not affect received `args' being empty however.

Maciej