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/compile: constant bounds check inconsistency with type parameters #65417

Closed
fabe-xx opened this issue Feb 1, 2024 · 9 comments
Closed

cmd/compile: constant bounds check inconsistency with type parameters #65417

fabe-xx opened this issue Feb 1, 2024 · 9 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@fabe-xx
Copy link

fabe-xx commented Feb 1, 2024

Go version

go version go1.21.6 windows/amd64

Output of go env in your module/workspace:

set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\telic\AppData\Local\go-build
set GOENV=C:\Users\telic\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\telic\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\telic\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLCHAIN=auto
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.21.6
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=C:\Users\telic\AppData\Local\Temp\go-build2584633790=/tmp/go-build -gno-record-gcc-switches

What did you do?

This won't build, as it's known at compile time that the index is out of bounds of the constant string (playground):

func wontBuild[T byte]() {
	const str = "a"
	var t T
	var _ = str[unsafe.Sizeof(t)]
}

This does build, though, even though the only difference is some arithmetic (+1) on the index, which is even more out of bounds. It instead produces a runtime panic (playground):

func panics[T byte]() {
	const str = "a"
	var t T
	var _ = str[unsafe.Sizeof(t)+1]
}

What did you see happen?

An inconsistency in how the unsafe.Sizeof() pseudo-constants are applied to static bounds checks.

What did you expect to see?

I'd expect neither version to build.

This was discovered when attempting to make an offset slice into a string constant, the offset depending on the passed type's size. In that particular case it would have panicked if a sufficiently large type was applied to the function, but no code ever used that large types with it. It would be nice if the compiler could ignore this error in case it's only ever fed smaller types. Probably difficult, though.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 1, 2024
@go101
Copy link

go101 commented Feb 1, 2024

Really an inconsistency.

By the current rule, a call to Sizeof with a argument of a type parameter type is not viewed as constant expression.
So both should compile okay and panic at run time.

[edit]: a complete and a bit modified version:

package main

import "unsafe"

func main() {
  wontBuild(0) // comment on to compile
  panics(0)
}

func wontBuild[T byte](t T) {
  const str = "a"
  _ = str[unsafe.Sizeof(t)]
}

func panics[T byte](t T) {
  const str = "a"
  _ = str[unsafe.Sizeof(t)+0]
}

@mknyszek
Copy link
Contributor

mknyszek commented Feb 1, 2024

CC @golang/compiler

@mknyszek mknyszek added this to the Backlog milestone Feb 1, 2024
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 1, 2024
@mdempsky
Copy link
Member

mdempsky commented Feb 1, 2024

Like @go101 said, this is working as intended. unsafe.Sizeof applied to a derived type (for example, a type parameter or a composite type containing one) is a regular value expression. It will still be evaluated at compile-time and optimized like a Go constant. But as far as the type checker is concerned, it's not a Go constant. So it won't emit diagnostics relating to invalid uses of Go constant.

We could change the language spec such that unsafe.Sizeof(t) in your code is recognized as the constant 1, since T must be byte and therefore have size 1. But that would need to go through the proposal process.

@mdempsky mdempsky closed this as completed Feb 1, 2024
@go101
Copy link

go101 commented Feb 1, 2024

So the inconsistency is a compiler dependent behavior?
(More specifically, whether or not the unsafe.Sizeof(t)+0 expression evaluated at run time is compiler dependent?)

@mdempsky
Copy link
Member

mdempsky commented Feb 1, 2024

So the inconsistency is a compiler dependent behavior?

No.

@mdempsky
Copy link
Member

mdempsky commented Feb 1, 2024

Thanks to @griesemer for pointing out I misread the issue description. Reopening.

@mdempsky mdempsky reopened this Feb 1, 2024
@mdempsky mdempsky self-assigned this Feb 1, 2024
@mdempsky mdempsky modified the milestones: Backlog, Go1.23 Feb 1, 2024
@mdempsky mdempsky added Suggested Issues that may be good for new contributors looking for work to do. NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 1, 2024
@mdempsky
Copy link
Member

mdempsky commented Feb 1, 2024

Thanks for the report. @griesemer and I agree both programs should panic.

Running go build -gcflags=-h shows a stack trace in the unified reader. So there's probably just somewhere that we're turning the unsafe.Sizeof expression into an OLITERAL, and then applying bounds checks in typecheck still. types2 handles all constant-related bounds checks in user Go code now, so it's perhaps safe to just remove those checks from typecheck. (These are unrelated to the runtime bounds checks inserted at code generation later in the compiler.)

CC @cuonglm @cagedmantis in case either of you are interested in tackling this. It should be a small fix.

@griesemer
Copy link
Contributor

I like that types2 is now considered user Go code 👍🏻

@cuonglm cuonglm self-assigned this Feb 2, 2024
@gopherbot
Copy link

Change https://go.dev/cl/560575 mentions this issue: cmd/compile/internal/typecheck: remove constant bounds check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
Development

No branches or pull requests

7 participants