Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cgo: generates code with inappropriate array copy #32579

Closed
DryHumour opened this issue Jun 12, 2019 · 10 comments

Comments

@DryHumour
Copy link

commented Jun 12, 2019

The following code behaves as expected under go1.11 and go.1.11.10 but panics under go1.12 and go1.12.6.

Minor changes to the expression passed to the address-of operator "fix" the behaviour. Debugging and instrumentation show that an "unexpected" pointer is passed to the underlying cgo wrapper and C function; perhaps a heap allocated temporary? The presence of both IndexExpr appear to be necessary to the reproduction of the problem; with only one or the other, it apparently does not reproduce.

The ast.Print() output appears to match between 1.11 and 1.12, but the liveness/opt analysis (gcflags -m -live), parse tree (-W), Go asm (-S), and final object code all differ in various ways. (I currently lack enough experience with Go compiler internals to further interpret the information.)

What did you do?

(Code does not run under Go playground because of Cgo.)

package main

// #include <string.h>
// struct S { unsigned char data[1]; };
import "C"
import "unsafe"

func main() {
	const fill = 0xff
	var s [1]C.struct_S
	C.memset(unsafe.Pointer(&s[0].data[0]), fill, C.size_t(unsafe.Sizeof(s[0].data))) // panic
	// C.memset(unsafe.Pointer(&(s[0].data[0])), fill, C.size_t(unsafe.Sizeof(s[0].data))) // ok
	// C.memset(unsafe.Pointer(&s[0].data), fill, C.size_t(unsafe.Sizeof(s[0].data))) // ok
	if s[0].data[0] != fill {
		panic("oops")
	}
}

What did you expect to see?

No output, exit 0.

With go run -gcflags -r:

# command-line-arguments
MOVE [0xc00038bb30]
.   NAME-main.s a(true) g(1) l(13) x(0) class(PAUTO) ld(1) tc(1) addrtaken used ARRAY-[1]_Ctype_struct_S

What did you see instead?


goroutine 1 [running]:
main.main()
	/home/nschelle/go/src/cgo/cgo.go:15 +0x42
exit status 2

With go run -gcflags -r:

# command-line-arguments
MOVE [0xc0003b73b0]
.   NAME-main._cgoIndex0 a(true) g(2) l(14) x(0) class(PAUTO) ld(1) tc(1) addrtaken used ARRAY-[1]_Ctype_uchar
panic: oops

goroutine 1 [running]:
main.main()
	/home/nschelle/go/src/cgo/cgo.go:15 +0x42
exit status 2

Does this issue reproduce with the latest release (go1.12.6)?

Yes.

System details

go version go1.12.5 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nschelle/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nschelle/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.12.5 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.12.5
uname -sr: Linux 4.4.0-146-generic
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial
/gnu/store/h90vnqw0nwd0hhm1l5dgxsdrigddfmq4-glibc-2.28/lib/libc.so.6: GNU C Library (GNU libc) stable release version 2.28.
gdb --version: GNU gdb (GDB) 8.3

@ianlancetaylor ianlancetaylor changed the title cgo behaviour change 1.11 to 1.12 cmd/cgo: generates code with inappropriate array copy Jun 13, 2019

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Jun 13, 2019

@kawakami-o3

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

I investigated this issue and realized that @ianlancetaylor understood this issue completely. But, for my curiosity, I report my result.

This behavior is produced after the CL, https://go-review.googlesource.com/c/go/+/142884/ . For example, it rewrites C function call,

C.memset(unsafe.Pointer(&s[0].data[0]), 1, C.size_t(1))

with

func() _cgo_unsafe.Pointer{ _cgoIndex0 := s[0].data; _cgo0 := unsafe.Pointer(&_cgoIndex0[0]); _cgo1 := _Ctype_int(1); _cgo2 := _Ctype_size_t(1); _cgoCheckPointer(_cgo0, _cgoInd  ex0); return ...

, intead of the right one,

func() _cgo_unsafe.Pointer{ _cgoIndex0 := &s[0].data; _cgo0 := unsafe.Pointer(&(*_cgoIndex0)[0]); _cgo1 := _Ctype_int(1); _cgo2 := _Ctype_size_t(1); _cgoCheckPointer(_cgo0, _cgoInd  ex0); return ...

.

This behavior can be patched as follows,

func (p *Package) isVariable(x ast.Expr) bool {
  switch x := x.(type) {
  case *ast.Ident:
    return true
  case *ast.SelectorExpr:
    return p.isVariable(x.X)
  case *ast.IndexExpr:   // new line
    return true          // new line
  }
  return false
}

Is it the expeced patch ?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Yes, at least at first glance that seems like the right general idea. Would you like to send that in as a GitHub pull request or in Gerrit as described at https://golang.org/doc/contribute.html? Please also add a test to misc/cgo/test. Thanks.

@kawakami-o3

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Thank you for your checking. I will send my patch and test via Gerrit as soon as possible.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 22, 2019

Change https://golang.org/cl/183458 mentions this issue: cmd/cgo: fix fix inappropriate array copy

@gopherbot gopherbot closed this in 38fc0af Jun 24, 2019

@tandr

This comment has been minimized.

Copy link

commented Jun 24, 2019

Is it a candidate for a backport?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@gopherbot please open backport to 1.12

@gopherbot

This comment has been minimized.

Copy link

commented Jun 24, 2019

Backport issue(s) opened: #32756 (for 1.12).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

1.11 doesn't have the bug but 1.12 does.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 24, 2019

Change https://golang.org/cl/183627 mentions this issue: [release-branch.go1.12] cmd/cgo: fix inappropriate array copy

@gopherbot

This comment has been minimized.

Copy link

commented Jun 25, 2019

Change https://golang.org/cl/183778 mentions this issue: misc/cgo/test: use char, not int, so test works on big-endian systems

gopherbot pushed a commit that referenced this issue Jun 25, 2019
misc/cgo/test: use char, not int, so test works on big-endian systems
Updates #32579
Fixes #32770

Change-Id: I32d1dea7505e8ad22e11a9806e10d096924b729b
Reviewed-on: https://go-review.googlesource.com/c/go/+/183778
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue Jun 26, 2019
[release-branch.go1.12] cmd/cgo: fix inappropriate array copy
Ensure that during rewriting of expressions that take the address of
an array, that we properly recognize *ast.IndexExpr as an operation
to create a pointer variable and thus assign the proper addressOf
and deference operators as "&" and "*" respectively.

This fixes a regression from CL 142884.

This is a backport of CLs 183458 and 183778 to the 1.12 release branch.
It is not a cherry pick because the code in misc/cgo/test has changed.

Updates #32579
Fixes #32756

Change-Id: I0daa75ec62cccbe82ab658cb2947f51423e0c235
Reviewed-on: https://go-review.googlesource.com/c/go/+/183627
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.