@dr2chase

Note that what Minux describes is a change from previous behavior, and escape analysis is "tricked" because now there is a "Go function body" that used to be written in C (hence, opaque to escape analysis).

Speaking as someone new to Go and with a pathological imagination about how optimizers can jerk you around, I'd be a little worried that (future) better dead code elimination might disappear all the Go-side pointers and make the underlying object eligible for GC. In addition, if anyone on the C side gets the idea that they can mutate pointers in objects on the Go heap, sadness and woe might ensue (in a crashy and Heisenbuggy way).

I was actually wondering about that too when a NOP function was mentioned but I know very little about Go internals.

FWIW, I frequently use cgo libraries that mutate Go-allocated memory.

Go-allocated objects should not be used by C outside of the cgo call (i.e. remembered for future use). So allocating them on stack is OK and beneficial, we do this for syscalls as well.
If a C function remembers an object for future use, then it must be C allocated.
I would say WAI.

Whether or not I agree with that argument (I'm definitely an "opposing voice in the community" when it comes to the current GC plans wrt heap data, see https://groups.google.com/forum/#!msg/golang-dev/JO99bwdOICs/oV3p-vaBbeEJ or https://groups.google.com/forum/#!msg/golang-dev/qvOqcmAkKnA/CcdW-ogVgrYJ), we are definitely getting corruption due to this issue for cgo code that is not remembering the passed-in pointer somewhere after it returns.

If I recall correctly, certain small syscalls don't have to do as much bookkeeping and setup as cgo?

Either way, for a C function that was purely operating on the passed-in pointer and not remembering it, the pointer became invalid due to stack-moving, in our experience.

we are definitely getting corruption due to this issue for cgo code that is not remembering the passed-in pointer somewhere after it returns.

that's kinda an important detail worth noting in the issue description
I guess there is a bug in cgo wrappers, they remember a pointer as uintptr and then grow stack making the pointer invalid. That can be fixed without promoting the objects to heap. Just as we do for syscalls.

You're right, I apologize for leaving it out. The reason we discovered the unexpected difference in escape analysis is due to infrequent corruption in one of our cgo libraries. We were able to prove that when the corruption happened, the correct pointer address had changed in Go-land.

Also, on further thought, I actually think requiring that C libraries don't save any Go pointers is probably a fine rule, even given the concerns about passing Go-allocated byte-slices to C. Update: on even further thought, I think not being able to remember Go pointers in C makes many callback-based C libraries unusable.

On Wed, Apr 1, 2015 at 2:04 AM, Dmitry Vyukov notifications@github.com
wrote:

we are definitely getting corruption due to this issue for cgo code that
is not remembering the passed-in pointer somewhere after it returns.

that's kinda an important detail worth noting in the issue description
I guess there is a bug in cgo wrappers, they remember a pointer as uintptr
and then grow stack making the pointer invalid. That can be fixed without
promoting the objects to heap. Just as we do for syscalls.

I think the way we did for syscall will move pointers argument to heap, no?

Syscalls also accept uintptr's, so they should not promote pointers to heap.

Passing pointers to C prevents (or if one prefers breaks) optimizations
that affect your entire Go program. It interferes with whole classes of GC
optimizations, some related to making GC run faster while others, such as
improving cache locality, are related to making application code run
faster. The only reasonable solution seems to be for C code to malloc any
memory it uses and share those pointers with Go, not the other way around.
Using Go as a path to adding garbage collection to C code is not the way
forward.

The runtime team has chosen to port all of its code, including the entire
runtime and compiler, to Go to avoid these problems. Your choice may be
different and well founded but the C / Go barrier is going to come with
sharp edges.

On Wed, Apr 1, 2015 at 3:44 AM, Dmitry Vyukov notifications@github.com
wrote:

Syscalls also accept uintptr's, so they should not promote pointers to
heap.

—
Reply to this email directly or view it on GitHub
#10303 (comment).

Hi @RLH!

As discussed on https://groups.google.com/forum/#!msg/golang-dev/JO99bwdOICs/oV3p-vaBbeEJ and https://groups.google.com/forum/#!msg/golang-dev/qvOqcmAkKnA/CcdW-ogVgrYJ at length (and perhaps in other places), this effectively kills the performance of a number of cgo libraries.

The two main examples are people hoping to use fast math libraries written in C, and in my case, people who want to implement io.Reader and io.Writer in C. There is no way to implement an io.Reader or an io.Writer using a cgo library, given what you say, without adding forced (and imo unnecessary) memory copies, which will dramatically slow performance. There's no way to require that all byte slices passed to my cgo io.Reader/io.Writer were originally allocated with C.malloc or equivalent.

The discussion in both threads seemed to indicate that this might be a good point, and the Go team was considering another way.

In the mean time, as long as these GC optimizations aren't yet implemented, fixing this particular issue seems to be a great way to keep current Go/cgo code working as expected by people using this type of scheme.

I assume this issue isn't the only reason the Go team chose to port the Go compiler to Go.

Syscalls still stay in C. Most real Go programs crucially rely on performance of zero-copy passing of byte slices between Go and C. Allocating these byte slices in C memory in not feasible. So zero copy passing of byte slices between Go and C should stay.

I agree it does seem like keeping zero-copy byte slices between C and Go kills a lot of possible Go GC strategies (unless byte slices are special cased and come out of a different memory segment I suppose). @dvyukov, in the case that the heap isn't being moved around, is there much harm in forcing things that go into C-land to the heap? I think you end up getting the ability to remember Go pointers in C-land for free that way.

I can see the problem with passing stack pointers into C. While in C the
stack can shrink, and that induces a stack copy. Any pointers into the
stack held by C code become invalid (the stack copier doesn't know where
those are so it can't update them).

So I don't think we should allow pointers to stack objects to escape to C.

The other option would be to disable shrinking while in cgo. Or shrink
stacks in place. Both are possible, but come with nontrivial penalties.

On Wed, Apr 1, 2015 at 7:36 AM, JT Olds notifications@github.com wrote:

I agree it does seem like keeping zero-copy byte slices between C and Go
kills a lot of possible Go GC strategies (unless byte slices are special
cased and come out of a different memory segment I suppose). @dvyukov
https://github.com/dvyukov, in the case that the heap isn't being moved
around, is there much harm in forcing things that go into C-land to the
heap? I think you end up getting the ability to remember Go pointers in
C-land for free that way.

—
Reply to this email directly or view it on GitHub
#10303 (comment).

I was going to say more last night but did not, but the way this has been
dealt with in other languages with a formal safe/unsafe separation
(Modula-3, Cedar Mesa, one niche-and-not-widely-known Java implementation)
is to have a way in the safe language to talk about unsafe (C-malloc'd,
arithemetic'd, mmap'd, etc) memory layout (i.e, structures and pointers to
those structures). Go is a little funny from that POV in that it doesn't
have a traced/untraced distinction on its struct types, but it does have
the implementation artifact that its gc is okay with untraced pointers
leaking into the heap (and RLH says he's relatively happy with that).

In particular, that brand-X Java implementation had an all-objects-moving
garbage collector, and this technique worked fine there, and we used it for
file and network buffers. I'll try to work up an example (I am still
learning Go) and post it later.

The lack of traced/untraced in the type system means that you are not
statically prohibited from making the mistake of (in Go code) storing Go
pointers into the C heap, but it allows zero-copy interchange of C pointers
into Go. The other issue with lacking a traced/untraced distinction is
that if this interchange is supported, then Go structure layout must always
mimic the C structure layout (but this is a problem we have already leaking
Go into C). Both of these can be avoided if types are explicitly marked
traced/untraced.

An altnerate way to address the leaking-pointers problem w/o separating
traced from untraced is to provide a CMalloc interface that will only work
for data types that do not themselves contain pointers. This is less
satisfying for things like array-of-arrays.

Slices are also a problem because the usual idiom uses finalizers in a
wrapper to ensure eventual reclamation of the C storage, and slices are not
tied to the wrapper object.

On Wed, Apr 1, 2015 at 10:36 AM, JT Olds notifications@github.com wrote:

I agree it does seem like keeping zero-copy byte slices between C and Go
kills a lot of possible Go GC strategies (unless byte slices are special
cased and come out of a different memory segment I suppose). @dvyukov
https://github.com/dvyukov, in the case that the heap isn't being moved
around, is there much harm in forcing things that go into C-land to the
heap? I think you end up getting the ability to remember Go pointers in
C-land for free that way.

—
Reply to this email directly or view it on GitHub
#10303 (comment).

@randall77

While in C the stack can shrink, and that induces a stack copy.

We don't shrink stack while in C or in syscall.
I guess we just copy stack after we store a pointer to stack into an uintptr.

This discussion is getting way out of scope for the issue. We are not going to redefine the way cgo works in this issue.

The problem is that when I converted cgo from generating C stubs to generating Go stubs, I did not preserve the semantics of the generated C stubs, namely that (1) from the compiler's perspective all pointers passed to the stubs escape, and (2) from the garbage collector's perspective all pointers passed to the stubs are kept live for the duration of the stub call.

As Minux wrote, the fix is similar to what we did in syscall: insert use(x) calls in the stubs after C is done with x, both to make x appear to escape and to make sure it is considered live until that point. Something along these lines should work and not require any assembly. The diff below illustrates what I mean but is not complete (isPointer needs to be defined) and not tested.

g% git diff 
diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go
index 346ae94..0d36fd0 100644
--- a/src/cmd/cgo/out.go
+++ b/src/cmd/cgo/out.go
@@ -76,6 +76,10 @@ func (p *Package) writeDefs() {
        fmt.Fprintf(fgo2, "var _ syscall.Errno\n")
    }
    fmt.Fprintf(fgo2, "func _Cgo_ptr(ptr unsafe.Pointer) unsafe.Pointer { return ptr }\n\n")
+   if !*gccgo {
+       fmt.Fprintf(fgo2, "//go:linkname _Cgo_use runtime.cgoUse\n")
+       fmt.Fprintf(fgo2, "func _Cgo_use(unsafe.Pointer)\n")
+   }

    typedefNames := make([]string, 0, len(typedef))
    for name := range typedef {
@@ -403,7 +407,7 @@ func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
        return
    }

-   // C wrapper calls into gcc, passing a pointer to the argument frame.
+   // Wrapper calls into gcc, passing a pointer to the argument frame.
    fmt.Fprintf(fgo2, "//go:cgo_import_static %s\n", cname)
    fmt.Fprintf(fgo2, "//go:linkname __cgofn_%s %s\n", cname, cname)
    fmt.Fprintf(fgo2, "var __cgofn_%s byte\n", cname)
@@ -438,6 +442,11 @@ func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
    if n.AddError {
        fmt.Fprintf(fgo2, "\tif errno != 0 { r2 = syscall.Errno(errno) }\n")
    }
+   for i, param := range d.Type.Params.List {
+       if isPointer(param.Type) {
+           fmt.Fprintf(fgo2, "\t_Cgo_use(unsafe.Pointer(p%d))\n", i)
+       }
+   }
    fmt.Fprintf(fgo2, "\treturn\n")
    fmt.Fprintf(fgo2, "}\n")
 }
diff --git a/src/runtime/cgo.go b/src/runtime/cgo.go
index 5dc83c0..3fc576a 100644
--- a/src/runtime/cgo.go
+++ b/src/runtime/cgo.go
@@ -28,3 +28,11 @@ var iscgo bool
 // cgoHasExtraM is set on startup when an extra M is created for cgo.
 // The extra M must be created before any C/C++ code calls cgocallback.
 var cgoHasExtraM bool
+
+// cgoUse is called by cgo-generated code (using go:linkname to get at
+// an unexported name). The calls serve two purposes:
+// 1) they are opaque to escape analysis, so the argument is considered to
+// escape to the heap.
+// 2) they keep the argument alive until the call site; the call is emitted after
+// the end of the (presumed) use of the argument by C.
+func cgoUse(unsafe.Pointer) {}
g%

Dmitry, I think you're right that copying can never happen in a syscall.
But it can happen in cgo, specifically during a Go callback. See
https://codereview.appspot.com/144130043/

On Wed, Apr 1, 2015 at 12:23 PM, Russ Cox notifications@github.com wrote:

This discussion is getting way out of scope for the issue. We are not
going to redefine the way cgo works in this issue.

The problem is that when I converted cgo from generating C stubs to
generating Go stubs, I did not preserve the semantics of the generated C
stubs, namely that (1) from the compiler's perspective all pointers passed
to the stubs escape, and (2) from the garbage collector's perspective all
pointers passed to the stubs are kept live for the duration of the stub
call.

As Minux wrote, the fix is similar to what we did in syscall: insert
use(x) calls in the stubs after C is done with x, both to make x appear to
escape and to make sure it is considered live until that point. Something
along these lines should work and not require any assembly. The diff below
illustrates what I mean but is not complete (isPointer needs to be defined)
and not tested.

g% git diff
diff --git a/src/cmd/cgo/out.go b/src/cmd/cgo/out.go
index 346ae94..0d36fd0 100644
--- a/src/cmd/cgo/out.go
+++ b/src/cmd/cgo/out.go
@@ -76,6 +76,10 @@ func (p *Package) writeDefs() {
fmt.Fprintf(fgo2, "var _ syscall.Errno\n")
}
fmt.Fprintf(fgo2, "func _Cgo_ptr(ptr unsafe.Pointer) unsafe.Pointer { return ptr }\n\n")

• if !*gccgo {

•   fmt.Fprintf(fgo2, "//go:linkname _Cgo_use runtime.cgoUse\n")
  

•   fmt.Fprintf(fgo2, "func _Cgo_use(unsafe.Pointer)\n")
  

• }

typedefNames := make([]string, 0, len(typedef))
for name := range typedef {
@@ -403,7 +407,7 @@ func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
return
}

• // C wrapper calls into gcc, passing a pointer to the argument frame.
• // Wrapper calls into gcc, passing a pointer to the argument frame.
  fmt.Fprintf(fgo2, "//go:cgo_import_static %s\n", cname)
  fmt.Fprintf(fgo2, "//go:linkname _cgofn%s %s\n", cname, cname)
  fmt.Fprintf(fgo2, "var _cgofn%s byte\n", cname)
  @@ -438,6 +442,11 @@ func (p *Package) writeDefsFunc(fgo2 io.Writer, n *Name) {
  if n.AddError {
  fmt.Fprintf(fgo2, "\tif errno != 0 { r2 = syscall.Errno(errno) }\n")
  }
• for i, param := range d.Type.Params.List {

•   if isPointer(param.Type) {
  

•       fmt.Fprintf(fgo2, "\t_Cgo_use(unsafe.Pointer(p%d))\n", i)
  

•   }
  

• }
  fmt.Fprintf(fgo2, "\treturn\n")
  fmt.Fprintf(fgo2, "}\n")
  }
  diff --git a/src/runtime/cgo.go b/src/runtime/cgo.go
  index 5dc83c0..3fc576a 100644
  --- a/src/runtime/cgo.go
  +++ b/src/runtime/cgo.go
  @@ -28,3 +28,11 @@ var iscgo bool
  // cgoHasExtraM is set on startup when an extra M is created for cgo.
  // The extra M must be created before any C/C++ code calls cgocallback.
  var cgoHasExtraM bool

+// cgoUse is called by cgo-generated code (using go:linkname to get at
+// an unexported name). The calls serve two purposes:
+// 1) they are opaque to escape analysis, so the argument is considered to
+// escape to the heap.
+// 2) they keep the argument alive until the call site; the call is emitted after
+// the end of the (presumed) use of the argument by C.
+func cgoUse(unsafe.Pointer) {}
g%

—
Reply to this email directly or view it on GitHub
#10303 (comment).

@randall77 You are right! How it ever worked?.. Need more tests.
I am curious whether it also affects windows syscalls. As far as I remember some windows syscalls also do callbacks. @alexbrainman

Thank you @dvyukov I will try to create similar test for windows callbacks.

Alex

@dvyukov

I think windows syscall might have same problem:

func syscall_Syscall(fn, nargs, a1, a2, a3 uintptr) (r1, r2, err uintptr) {
        var c libcall
        c.fn = fn
        c.n = nargs
        c.args = uintptr(unsafe.Pointer(&a1))
        cgocall_errno(asmstdcallAddr, unsafe.Pointer(&c))
        return c.r1, c.r2, c.err
}

c does not escape to the heap. Our asm code writes into c.r1, c.r2, c.err just before syscall returns. That could be fatal if stack moved. Isn't it?

Alex

@alexbrainman It looks fatal. Also, are user args (a1, a2, a3) escape?
Are there syscalls that do callbacks? You can write a test that grows stack in a callback and run it with GODEBUG=efence=1, it should crash.

I suspect some Syscall parameters do not escape. (I could not check #10391) I fixed special case with string parameters. But I don't see why other parameters have to escape. I suspect we don't see the problem, because we don't use many windows syscalls that do callbacks. And the syscalls that do, perhaps, do not receive Go pointers as arguments. Maybe we just need to fix "return values" as described above.

I will try to create a test that breaks as per your description. Thank you.

Alex

@alexbrainman

Our asm code writes into c.r1, c.r2, c.err just before syscall returns.

If the asm code saves c pointer on Go stack, then it should be updated as well, provided that the stack slow is marked pointer.

Looking at the code:

// void runtime<C2><B7>asmstdcall(void *c);
TEXT runtime<C2><B7>asmstdcall(SB),NOSPLIT,$0
        // asmcgocall will put first argument into CX.
        PUSHQ   CX                      // save for later
        MOVQ    libcall_fn(CX), AX
        MOVQ    libcall_args(CX), SI
        MOVQ    libcall_n(CX), CX

It seems that c is saved on stack, however, I don't see anything that marks this stack slot as pointer...

asmstdcall is running on OS stack. We switching stacks in asmcgocall. I guess we should be OK then. Except for when syscall is doing callback, then we need to be careful to have any syscall "addressed" parameters stored on the heap, not on Go stack. Sounds right?

Alex

Right. Including "var c libcall", it also must be on heap. PRobably it can be part of m struct.

What is the advantage of having libcall field in m?

Alex

Hold on. We save original c address, but then Go stack can move, so asm will be using old c address. I will try and write those tests to see what happens.

Alex

It would be a good idea to have a GODEBUG option which forces runtime to offset all stacks by a random constant. And then run all.bash in a loop in this mode. Or maybe it can be testing package flag. I.e. run tests with all possible stack offsets.

Created new issue #10406 for windows.

Alex

Interesting, @minux, how come http://golang.org/src/syscall/syscall.go#L92 has //go:noescape? Isn't that the opposite of what we want?

For anyone who cares, here's how I fixed this for Go 1.4.2 (compiles, passes all tests, etc). If someone wants to turn it into a patchset for tip I was too afraid to merge my new byValue function with cgoType and change cgoType, but it probably should just use cgoType instead. I'd be easily talked into giving it a shot though if this is a reasonable approach.

It'd also be worth writing a test for this behavior instead of my by-hand testing, but I wasn't sure the best way. Maybe put a bunch of stuff on the stack, call a C function with something from the middle of all the stack-allocated variable definitions, and make sure it's outside of that memory range?

https://gist.github.com/jtolds/83a8cf059d496586709f

We definitely want syscall.use to be noescape. The point of that function is not to make the pointers escape--from the point of view of Go, they do not. The point is to make sure that the pointers are live after the function call, as the function call itself will not make them live since they are being passed as uintptr. See https://golang.org/cl/139360044 .

Well a use call like this is good for both keeping a variable live, but also keeping it on the heap. My understanding of this specific issue is that due to stack moving we want to force variables passed to other systems (C/kernel/whatever) to the heap? I guess syscalls are probably different.

Syscalls are different because we know for sure that they don't squirrel away a pointer. Pointers passed to system calls do not escape.

On the other hand, if we can shrink the stack while blocked in syscall.Read, we definitely have a bug.

One option for the cgo callback case is to start a new stack for the callback. It might be a bit heavyweight, but it means we can forbid stack copying while in cgo. That might buy us a lot more than the (relatively uncommon?) callback case.

certainly worth benchmarking, but the impetus for this issue is to be able to implement io.Readers and io.Writers with cgo (at least for me), so the overhead of doing a brand new stack every time i want to send a range of bytes (like a write to our openssl wrapper for example) sounds like it'd be a pretty rough perf hit. i'm an idiot who can't read

whoops, so sorry, i read way wrong. forbid stack copying. that's probably great. i thought you were saying new special stack for every cgo call.

it definitely seems like there's two concerns that may have different solutions

basically I think cgo has the same two dimensions that syscalls have: (1) whether or not memory passed in escapes (syscalls definitely don't squirrel away memory, cgo could (saved callbacks references, etc)), and (2) whether or not the stack can shrink under cgo calls invalidating passed in memory.

seems like dimension 2 could be solved a number of ways (forcing to the heap, or yeah, forbid stack copying while in cgo) separately from how dimension 1 is solved, but yeah it would be nice if it was pretty snappy at least for my case.

forcing stuff to the heap seems necessary for dimension 1, but perhaps that's not the common case and could be opt in?

CL https://golang.org/cl/10814 mentions this issue.