Go: cmd/compile: -d=checkptr should disable inlining reflect.Value.{Pointer,UnsafeAddr} even when package reflect is compiled without it

[wI2L]

goos/goarch: linux/amd64
version: go version devel +7f98c0e Tue Oct 22 08:54:50 2019 +0000 linux/amd64

This program should be valid according to unsafe rules number 5.

package main

import (
    "fmt"
    "reflect"
    "unsafe"
)

func main() {
    n := 10
    v := reflect.ValueOf(&n)
    p := unsafe.Pointer(v.Elem().UnsafeAddr()) // <- panic

    fmt.Println(p)
}

When runned with -d=checkptr, it panics on runtime.PtrArith.

godev run -gcflags=-d=checkptr test.go
panic: (runtime.ptrArith) (0x4a37c0,0xc00000c080)

goroutine 1 [running]:
main.main()
    /home/wpoussie/test.go:12 +0x11a
exit status 2

The rule number 5, unless I misunderstood something, specify that the uintptr-typed return of functions UnsafeAddr and Pointer MUST be converted immediately to unsafe.Pointer after the call, but nothing about that unsafe pointer being reinterpreted immediately to another pointer type.

This variant, where the pointer is reinterpreted and dereferenced at the same time panics as well.

package main

import (
    "fmt"
    "reflect"
    "unsafe"
)

func main() {
    n := 10
    v := reflect.ValueOf(&n)
    n1 := *(*int)(unsafe.Pointer(v.Elem().UnsafeAddr())) // <- panic

    fmt.Println(n1)
}

/cc @mdempsky @cuonglm

Edit: @mdempsky for reference, I found this because of this line in the project we mentionned in https://github.com/golang/go/issues/35027.

[mdempsky]

Yeah those programs are both valid.

Weird, I thought checkptr was handling this:

https://github.com/golang/go/blob/19d2a1cc2c76a14b4d5066c871b3f3c11125e147/src/cmd/compile/internal/gc/walk.go#L3961-L3966

[wI2L]

I think the reproducers are representative of the issue, but nonetheless I'd like to add the stacktrace of the original panic from my wI2L/jettison project.

godev test -race -covermode=atomic -gcflags=-d=checkptr
--- FAIL: TestCompositeTypes (0.00s)
panic: (runtime.ptrArith) (0x81c900,0xc00000eb80) [recovered]
    panic: (runtime.ptrArith) (0x81c900,0xc00000eb80)

goroutine 12 [running]:
testing.tRunner.func1(0xc0000c2b00)
    /home/wpoussie/github/golang/go/src/testing/testing.go:874 +0x69f
panic(0x81c900, 0xc00000eb80)
    /home/wpoussie/github/golang/go/src/runtime/panic.go:679 +0x1b2
github.com/wI2L/jettison.(*Encoder).encode(0xc0000e8ab0, 0x8c6220, 0x804020, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, ...)
    /home/wpoussie/github/wI2L/jettison/encoder.go:231 +0x1cb
github.com/wI2L/jettison.(*Encoder).Encode(0xc0000e8ab0, 0x804020, 0xc0000e86c0, 0x8c0c60, 0xc0000e8ae0, 0x0, 0x0, 0x0, 0x0, 0x0)
    /home/wpoussie/github/wI2L/jettison/encoder.go:201 +0x1ba
github.com/wI2L/jettison.TestCompositeTypes(0xc0000c2b00)
    /home/wpoussie/github/wI2L/jettison/encoder_test.go:187 +0x82a
testing.tRunner(0xc0000c2b00, 0x866220)
    /home/wpoussie/github/golang/go/src/testing/testing.go:909 +0x19a
created by testing.(*T).Run
    /home/wpoussie/github/golang/go/src/testing/testing.go:960 +0x652
exit status 2
FAIL    github.com/wI2L/jettison    0.009s

I will check tonight when I get home if the issue reproduce on darwin/amd64, because I didn't see any issue yesterday evening after I fixed the remaining unsafe pointer misuses per your suggestions in https://github.com/wI2L/jettison/commit/293c0b0c5eef835d7b6a0e623a897678bf55ce5d.

[cuonglm]

It's interesting that:

go run -gcflags=-d=checkptr main.go

panics, but:

go run -gcflags=all=-d=checkptr main.go

doesn't.

[wI2L]

@cuonglm Good catch. So I've reran the test of my package wI2L/jettison again with -gcflags=all=-d=checkptr and it doesn't panic this time.

[mdempsky]

@cuonglm Oh right. Thanks for noticing that!

The UnsafeAddr and Pointer methods are marked as go:nocheckptr, which means they won't be inlined. This is necessary because otherwise walk.go can't recognize the function calls in their inlined forms.

But go:nocheckptr only disables inlining when that package is compiled with -d=checkptr. That's why all= makes a difference here.

I'll have to think about how best to handle this. Probably inl.go should be changed to disable inlining those calls based on whether the calling compilation unit is using checkptr, rather than whether package reflect was compiled with it.

[cuonglm]

@mdempsky This seems work:

        // If marked "go:nocheckptr", don't inline.
    if fn.Func.Pragma&NoCheckPtr != 0 {
        reason = "marked go:nocheckptr"
        return
    }

    // If -d checkptr compilation, don't inline.
    if Debug_checkptr != 0 {
        reason = "compile with -d=checkptr"
        return
    }

[cuonglm]

@mdempsky This seems work:

        // If marked "go:nocheckptr", don't inline.
  if fn.Func.Pragma&NoCheckPtr != 0 {
      reason = "marked go:nocheckptr"
      return
  }

  // If -d checkptr compilation, don't inline.
  if Debug_checkptr != 0 {
      reason = "compile with -d=checkptr"
      return
  }

Ah no, it doesn't, as it will disable inline if compiled with -d=checkptr alone.

[mdempsky]

I think the short-term solution here is just to declare that you have to enable -d=checkptr for all compilation units if you're going to use it (i.e., you have to use -gcflags=all=-d=checkptr, not just -gcflags=-d=checkptr).

The longer-term fix requires making inlining smarter about pragmas. This might be beneficial for the runtime too, since currently we disable inlining for a lot of runtime functions just because it's the easiest way to ensure write barriers are enabled/disabled correctly.

[wI2L]

@mdempsky Feel free to close this issue then, or keeping it for future references.

[mdempsky]

@wI2L Thanks, I'd like to keep it open for now. There's fundamentally no reason -d=checkptr can't work on a per-package basis, and I would like it to work; it just doesn't work today, and I don't think it's a priority at the moment.

[cuonglm]

@mdempsky Sorry to hjack here, but It sounds like -d=checkptr does not handle unsafe pointer rule 3rd at this moment, is it intended?

[mdempsky]

@cuonglm What makes you say that?

[cuonglm]

@mdempsky This one should be invalid per rule 3rd, but it passes:

package main

import "unsafe"

func main() {
    n := 10
    b := make([]byte, n)
    end := unsafe.Pointer(uintptr(unsafe.Pointer(&b[0])) + uintptr(n))
    println(end)
}

I tried other invalid cases in rule 3rd, the only one that -d=checkptr reports correctly is:

u := unsafe.Pointer(nil)
p := unsafe.Pointer(uintptr(u) + offset)

[mdempsky]

@cuonglm The detection is best effort. It's expected to have false negatives, but it should never have false positives.

In your example, make([]byte, 10) is actually allocated as a 16-byte GC object by the runtime. So &b[0] + 10 still points into this GC object, and we're not able to detect that it's unsafe.

If you change n := 10 to n := 16, then it's detected.

[cuonglm]

@cuonglm The detection is best effort. It's expected to have false negatives, but it should never have false positives.

In your example, make([]byte, 10) is actually allocated as a 16-byte GC object by the runtime. So &b[0] + 10 still points into this GC object, and we're not able to detect that it's unsafe.

If you change n := 10 to n := 16, then it's detected.

Thanks for details explanation 👍

[gopherbot]

Change https://golang.org/cl/222878 mentions this issue: cmd/compile: don't inline reflect.Value.UnsafeAddr/Pointer if enable checkptr

[bcmills]

@mdempsky, could you update the title of this issue to reflect your current understanding of the root cause?

[mdempsky]

@bcmills Done.