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

Jump to bottom

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

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

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

DryHumour opened this issue Jun 12, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.13

Comments

@DryHumour
Copy link

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 the NeedsFix The path to resolution is known, but the work has not been done. label Jun 13, 2019
@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Jun 13, 2019
@kawakami-o3
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@gopherbot
Copy link

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

@tandr
Copy link

tandr commented Jun 24, 2019

Is it a candidate for a backport?

@ianlancetaylor
Copy link
Contributor

@gopherbot please open backport to 1.12

@gopherbot gopherbot mentioned this issue Jun 24, 2019
Closed
@gopherbot
Copy link

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
Copy link
Contributor

1.11 doesn't have the bug but 1.12 does.

@gopherbot
Copy link

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

@gopherbot
Copy link

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
@ianlancetaylor
misc/cgo/test: use char, not int, so test works on big-endian systems
6d1aaf1 
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
@ianlancetaylor
[release-branch.go1.12] cmd/cgo: fix inappropriate array copy
77c14d2 
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>
@golang golang locked and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@kawakami-o3 @ianlancetaylor @tandr @gopherbot @DryHumour