[go-nuts] Bad code contribution experience
I want to vent my frustration. The experience of contributing has been unpleasant for me. I have noticed that similar fixes are prioritized and merged faster, while I spent time resolving the issue and reporting it, but it did not receive an effective merge. Although it was during the freeze period, I assumed it would be merged soon after the release. However, that was not the case. In terms of submission time, my pull request was submitted almost a month earlier than others', yet the newly submitted ones were merged more quickly (https://go-review.googlesource.com/c/go/+/55). Perhaps I should not have created a pull request during the freeze period. https://go-review.googlesource.com/c/go/+/543177 -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/cc2303b0-7daa-4392-b3c7-74d576c6a142n%40googlegroups.com.
Re: [go-nuts] Re: Passing 2d arrays from Go to C using runtime.Pinner
Thanks for the reproducer. I think I've puzzled out what's going wrong and it's pretty subtle. TL;DR: You can work around this by either calling `calloc` or just allocating `inRows` as Go memory and pinning that as well. The latter will be safer and faster overall. It's not totally clear to me at this moment whether just using `calloc` instead should be sufficient. Read on for more details. Writing pointers into C memory from Go may invoke a write barrier. The current write barrier is a hybrid barrier: it writes down the pointers that are both being written, and those that are being deleted in the write. In this particular case, we're writing a Go pointer into C memory *from Go*. If that C memory isn't zeroed *and* it points to some dead Go memory, we have a problem. That appears to be what's happening here. If I use calloc instead of malloc to create inRows, the problem goes away. Also, if I create inRows in Go and pin it, that also works. I think this is actually documented in the cgo pointer passing rules ( https://pkg.go.dev/cmd/cgo#hdr-Passing_pointers), but we all missed it: Note: the current implementation has a bug. While Go code is permitted to write nil or a C pointer (but not a Go pointer) to C memory, the current implementation may sometimes cause a runtime error if the contents of the C memory appear to be a Go pointer. Therefore, avoid passing uninitialized C memory to Go code if the Go code is going to store pointer values in it. Zero out the memory in C before passing it to Go. AFAICT, the rest of the page does not say one way or the other whether Go code writing a Go pointer into C memory is allowed (note that, counterintuitively, *C.float is a Go pointer, because it may point to Go memory). One question that I have to wonder about is whether it's OK to write a Go pointer into C memory at all and if we should just restrict that like we did before. The text above already implies that it is not OK, in which case the code in this thread is invalid and inRows basically must be allocated on the Go side. However, I think this may also just be an oversight from when I was updating the documentation for pinning; I think my original intent was to say that it's OK for Go code to write pinned Go pointers to C memory. Anyway, this needs more thought, since allowing Go code to write Go pointers to C memory (pinned or not) may end up restricting GC implementations in a way we don't want to. I filed https://github.com/golang/go/issues/65349 to track this. On Mon, Jan 29, 2024 at 11:17 AM peterGo wrote: > Michael, > > The OP appears to have lost interest in debugging. Here's my minimal, > reproducible example that produces the same fatal errror: > > https://go.dev/play/p/flEmSh1euqR (run locally) > > If the runtime.GC() statement is uncommented then the fatal error does not > occur. > > The use of this profligate, inefficient algorithm is for debugging > purposes only. The use of runtime.Pinner is not required. A single > implicitly pinned C wrapper function argument pointing to contiguous > underlying slice data arrays would suffice. > > Peter > > > On Friday, January 26, 2024 at 12:05:38 PM UTC-5 Michael Knyszek wrote: > >> Ignoring more efficient ways to pass memory to C for a moment, >> superficially I do think your code using Pinner should work. Do you have a >> full reproducer? It's hard to tell from just looking at your code if your >> code is the problem, or its just enough to trigger some other cgo issue >> elsewhere in your codebase. There's a slim chance this is a bug in the >> Pinner implementation, but that would be surprising to me. The Pinner >> trivially keeps all pointers passed to it live by just holding onto their >> references. >> >> Also, have you tried running your code with GODEBUG=cgocheck=1 and/or >> building your code with GOEXPERIMENT=cgocheck2? That might help identify >> what the issue is. >> On Friday, January 26, 2024 at 7:05:45 AM UTC-5 Tamás Gulácsi wrote: >> >>> To convert a Go slice to C array is easy with unsafe.Slice. >>> >>> The problem is that the multi-dimensional C array is an array of >>> pointers (*(*float64)) >>> which cannot travel between the C/Go barrier. >>> >>> In your example, you flatten your slice, and recreate the pointers in >>> that one big slice. >>> You could go on with that example, but from the Go side: >>> >>> 1. have a "flat" []float64 wihich is N*M >>> 2. have the [][]float64 as subslices of that one big flattened >>> []float64: flat[i*M:i*(M+1)] >>> 3. send that flat slice to the C side with unsafe.Slice >>> 4. have the C side implement the same subslicing: create a []*float64 >>> array and have each point to the corredt [i*M]. >>> 5. profit >>> >>> >>> Denis a következőt írta (2024. január 26., péntek, 1:18:59 UTC+1): >>> I am trying to pass 2d array from Go to some C function void foo(in **float, out *double). Since I want to have wrapper for this C function, I'd like that Go function has definition like func
[go-nuts] Re: Passing 2d arrays from Go to C using runtime.Pinner
Michael,
The OP appears to have lost interest in debugging. Here's my minimal,
reproducible example that produces the same fatal errror:
https://go.dev/play/p/flEmSh1euqR (run locally)
If the runtime.GC() statement is uncommented then the fatal error does not
occur.
The use of this profligate, inefficient algorithm is for debugging purposes
only. The use of runtime.Pinner is not required. A single implicitly pinned
C wrapper function argument pointing to contiguous underlying slice data
arrays would suffice.
Peter
On Friday, January 26, 2024 at 12:05:38 PM UTC-5 Michael Knyszek wrote:
> Ignoring more efficient ways to pass memory to C for a moment,
> superficially I do think your code using Pinner should work. Do you have a
> full reproducer? It's hard to tell from just looking at your code if your
> code is the problem, or its just enough to trigger some other cgo issue
> elsewhere in your codebase. There's a slim chance this is a bug in the
> Pinner implementation, but that would be surprising to me. The Pinner
> trivially keeps all pointers passed to it live by just holding onto their
> references.
>
> Also, have you tried running your code with GODEBUG=cgocheck=1 and/or
> building your code with GOEXPERIMENT=cgocheck2? That might help identify
> what the issue is.
> On Friday, January 26, 2024 at 7:05:45 AM UTC-5 Tamás Gulácsi wrote:
>
>> To convert a Go slice to C array is easy with unsafe.Slice.
>>
>> The problem is that the multi-dimensional C array is an array of pointers
>> (*(*float64))
>> which cannot travel between the C/Go barrier.
>>
>> In your example, you flatten your slice, and recreate the pointers in
>> that one big slice.
>> You could go on with that example, but from the Go side:
>>
>> 1. have a "flat" []float64 wihich is N*M
>> 2. have the [][]float64 as subslices of that one big flattened []float64:
>> flat[i*M:i*(M+1)]
>> 3. send that flat slice to the C side with unsafe.Slice
>> 4. have the C side implement the same subslicing: create a []*float64
>> array and have each point to the corredt [i*M].
>> 5. profit
>>
>>
>> Denis a következőt írta (2024. január 26., péntek, 1:18:59 UTC+1):
>>
>>> I am trying to pass 2d array from Go to some C function void foo(in
>>> **float, out *double). Since I want to have wrapper for this C
>>> function, I'd like that Go function has definition like func
>>> FooWrapper([][]float32) []float64. The easiest but not efficient
>>> implementation is allocating all memory through C that listed below:
>>>
>>> func FooWrapper(values [][]float32) []float64 {
>>> totalObj := len(values)
>>> totalParams := len(values[0])
>>> results := make([]float64, totalObj)
>>>
>>> ptrArrLength := uintptr(totalObj) * cPointerSize
>>> paramArrLength := uintptr(totalParams) * cFloatSize
>>>
>>> ptr := C.malloc(C.size_t(ptrArrLength +
>>> paramArrLength*uintptr(totalObj)))
>>> defer C.free(ptr)
>>>
>>> ptrSlice := unsafe.Slice((**C.float)(ptr), totalObj)
>>> for i, obj := range values {
>>> paramArrPtr := (*C.float)(unsafe.Add(ptr,
>>> ptrArrLength+uintptr(i)*paramArrLength))
>>> ptrSlice[i] = paramArrPtr
>>>
>>> paramSlice := unsafe.Slice(paramArrPtr, totalParams)
>>> for j, param := range obj {
>>> paramSlice[j] = (C.float)(param)
>>> }
>>> }
>>>
>>> C.foo((**C.float)(ptr), (*C.double)([0]))
>>>
>>> return results
>>> }
>>>
>>> Is that safe implementation? Can I pass pointer of result data? As far
>>> as I know, this pointer will be pinned because it passed to C function.
>>>
>>> But I want to allocate less memory just reusing Go memory, I've learned
>>> about runtime.Pinner that make pointer pinned until
>>> runtime.Pinner.Unpin() invocation. I tried to write another
>>> implementation using pinner:
>>>
>>> func FooWrapper(values [][]float32) []float64 {
>>> length := len(values)
>>> results := make([]float64, length)
>>>
>>> pinner := runtime.Pinner{}
>>> defer pinner.Unpin()
>>>
>>> arr := (**C.float)(C.malloc(C.size_t(uintptr(length) *
>>> cPointerSize)))
>>> defer C.free(unsafe.Pointer(arr))
>>> slice := unsafe.Slice(arr, length)
>>> for i, v := range values {
>>> pinner.Pin([0])
>>> slice[i] = (*C.float)([0])
>>> }
>>>
>>> C.foo(arr, (*C.double)([0]))
>>>
>>> return results
>>> }
>>>
>>> But, unfortunately, this code doesn't work
>>>
>>> runtime: pointer 0xc016ecbfc0 to unused region of span
>>> span.base()=0xc016eca000 span.limit=0xc016ecbfa0 span.state=1
>>> fatal error: found bad pointer in Go heap (incorrect use of unsafe or
>>> cgo?)
>>>
>>> Do I use runtime.Pinner wrong (as far as I know, I can pin slice data)?
>>> Or there is another error in this code. Are there some implementations for
>>> passing 3d (4d and so on) array to C function except for allocatiing and
>>> copying all data to C memory?
>>>
>>
--
You received this message because you are
[go-nuts] array index not starting at 0
go version go1.21.5 windows/amd64 There have been 9 garbage collections according to NumGC and also the PauseEnd array; However, the PauseNs array only shows two pause times, and they are incorrectly positioned in the array (the first time is logged at PauseNs[4] rather than PauseNs[0]) Can anyone provide some information about this. Leah ,"PauseTotalNs":1999400,"PauseNs":[0,0,0,0,1000100,999300,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0],"PauseEnd":[1706534601618276400,1706534601625558600,1706534601643261400,1706534601677238900,1706534601781174700,1706534602022683900,1706534602465533900,1706534603516006900,1706534604837389800,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0],"NumGC":9, -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/a462748e-dacd-43bd-935c-40ffdec6bbd3n%40googlegroups.com.
[go-nuts] discrepancy between goal heap size and NextGC
In the twelfth garbage collection of a short program I ran, GC trace reports the goal heap size as 735 MB (see below for gc11, gc12 and gc13 from GC trace) I also checked runtime Memstats (see attached image), and it says the nextGC target after 12 GCs have run is 838103616. Should goal Heap Size of GC Trace and NextGC of runtime.Memstats not be the same? gc 11 @22.009s 0%: 0.004+12+0.034 ms clock, 0.019+0.13/11/0.94+0.13 ms cpu, 562->562->367 MB, 964 MB goal, 4 P gc 12 @32.925s 0%: 0.003+16+0.024 ms clock, 0.014+0/3.7/13+0.099 ms cpu, 410->410->399 MB, 735 MB goal, 4 P (forced) gc 13 @32.964s 0%: 0.019+21+0.036 ms clock, 0.079+0/21/0.56+0.14 ms cpu, 400->400->399 MB, 799 MB goal, 4 P (forced) -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/91aa0a78-7271-43b5-8fb0-be2f5064b265n%40googlegroups.com.
[go-nuts] thoughts about https://go.dev/blog/loopvar-preview and how to change Go
The proposal says: To ensure backwards compatibility with existing code, the new semantics will only apply in packages contained in modules that declare go 1.22 or later in their go.mod files. This per-module decision provides developer control of a gradual update to the new semantics throughout a codebase I think this is not *safe*, since the `go` directive is often not explicitly chosen by the programmer. An example is when using `go mod init`; I suspect that most users will simply keep the current version. This is probably not an issue with `loopvar-preview` but it may prevent other language changes. As an example, suppose someone want to implement support for `error-propagation` (as done by Zig). In this case the user **must** explicitly declare that he want this feature, so that it (probably) does not count as breaking the Go1 compatibility. A possible solution is to add a new `feature` field to `go.mod`. As an example: ```go feature ( loopvar error-propagation ) ``` Regards Manlio Perillo -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/9f96ba8a-1a2d-4914-875a-c8c65ed7f4een%40googlegroups.com.
