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: C.stdout no longer assignable in go 1.10 #25221

Closed
sean-jc opened this issue May 2, 2018 · 8 comments
Closed

cmd/cgo: C.stdout no longer assignable in go 1.10 #25221

sean-jc opened this issue May 2, 2018 · 8 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@sean-jc
Copy link
Contributor

sean-jc commented May 2, 2018

What version of Go are you using (go version)?

go version devel +03876af91c Wed Aug 30 18:28:58 2017 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/sean/go"
GORACE=""
GOROOT="/home/sean/go/src/github.com/golang/go"
GOTOOLDIR="/home/sean/go/src/github.com/golang/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build130055863=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

Take the address of C.stdout and C.stderr to modify the underlying variable, e.g. to redirect stdout/stderr of C code. This worked in go1.9 and earlier, whereas newer Golang builds fail with cannot take the address of _Cmacro_stdout(), or cannot assign to _Cmacro_stdout() when assigning directly to C.stdout. Bisecting pointed at 03876af91c ("cmd/cgo: support niladic function-like macros")

https://play.golang.org/p/Fiwr8u-CaN8

What did you expect to see?

Successful compilation.

What did you see instead?

cmd/exec.go:35: cannot assign to _Cmacro_stdout()
cmd/exec.go:37: cannot take the address of _Cmacro_stdout()
cmd/exec.go:43: cannot take the address of _Cmacro_stderr()
@dominikh
Copy link
Member

dominikh commented May 2, 2018

Strictly speaking this has never been valid code. The C specification doesn't require stdout, stderr and stdin to be lvalues, that is, they don't have to be assignable to. For example, if you used musl instead of glibc, assigning to stdout would fail to compile even in ordinary C code.

@bcmills bcmills added this to the Go1.10.3 milestone May 2, 2018
@bcmills
Copy link
Contributor

bcmills commented May 2, 2018

CC: @ianlancetaylor @hirochachacha

@bcmills
Copy link
Contributor

bcmills commented May 2, 2018

As a (non-portable) workaround, you can probably put the address operation in the cgo preamble.

#include <stdint.h>
#include <stdio.h>

static FILE** p_stdout = &stdout;
static FILE** p_stderr = &stderr;

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label May 2, 2018
@ianlancetaylor
Copy link
Contributor

This is unfortunate but I don't think it's a bug. As @dominikh says, you were relying on specific behavior of your library, which is not guaranteed by the C standard and does not work with other libraries. We've changed cgo to support stdout and stderr in a way that we believe will work with all libraries. It's unfortunate that it broke your specific use case, but that use case was already nonportable. As @bcmills says, there is a workaround that should work with your specific library. Finally I will note that cgo is not covered by the Go 1 compatibility guarantee.

I'm going to close this issue. Please comment if you disagree.

@sean-jc
Copy link
Contributor Author

sean-jc commented May 3, 2018

@bcmills Thanks for the suggestion!

In case anyone else has a similar hack... I needed to explicitly close stdout/stderr prior to modifying the pointers to get things working with the pointer-defined-in-C approach. Apparently Go was clever enough to close the files for me when directly referencing them in Go?

@kostix
Copy link

kostix commented Jun 8, 2018

@sean-jc, you might supposedly could employ an approach which is (and always was, IIUC) correct for C:

  1. On the Go side, call .Fd() to get an OS descriptor referring to the Go's stdout. Note that this is not a pointer—it's an uintptr.
  2. Pass that value to the C side, then close(2) the stdout and then immediately dup(2) that FD to effectively retarget the C's stdout to the FD returned by Go.

The "trick" making this work is due to this feature of dup(2):

The dup() system call creates a copy of the file descriptor oldfd, using the lowest-numbered unused file descriptor for the new descriptor.

Since stdout always uses FD 1, (with stdin using 0 and stderr—2), when you close stdout and immediately dup(2) another FD, the new FD gets the number previously occupied by stdout, so the latter gets redirected.

Note that you must be sure these jumps around redirecting FDs are not done concurrently—to avoid races between close(2) and dup(2).

One caveat about this is that calling Fd() on an os.File value switches the underlying descriptor into synchronous I/O mode. I think that this makes no difference for the standard I/O streams but YMMV.

@fweimer
Copy link
Contributor

fweimer commented Jun 8, 2018

The standard way of doing this on the C side is via freopen. Assigning to stdin is a GNU extension.

@kostix Your advice regarding dup has a race condition. The runtime can create new file descriptors at any time. You need to use dup2 or dup3 instead, and you must not close the old descriptor explicitly.

@kostix
Copy link

kostix commented Jun 9, 2018

@fweimer IIUC, freopen(3) requires the pathname which might not be available (say, for a stream connected to a pipe). Thanks for pointing out dup2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants