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

Jump to bottom

x/tools/cmd/gotype: fails on arrays with Cgo-defined lengths #23712

Closed
rmmh opened this issue Feb 6, 2018 · 4 comments
Closed

x/tools/cmd/gotype: fails on arrays with Cgo-defined lengths #23712

rmmh opened this issue Feb 6, 2018 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.11

Comments

@rmmh
Copy link
Contributor

rmmh commented Feb 6, 2018

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

1.10rc1

Does this issue reproduce with the latest release?

Yes.

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

Ubuntu 16.04.

What did you do?

Use gotype (or go/types) to typecheck code that includes C declarations. Reduced test case:

package gonvml

// #cgo LDFLAGS: -ldl
/*
#include "nvml.h"

nvmlReturn_t (*nvmlSystemGetDriverVersionFunc)(char *version, unsigned int length);
nvmlReturn_t nvmlSystemGetDriverVersion(char *version, unsigned int length) {
  if (nvmlSystemGetDriverVersionFunc == NULL) {
    return NVML_ERROR_FUNCTION_NOT_FOUND;
  }
  return nvmlSystemGetDriverVersionFunc(version, length);
}
*/
import "C"

// SystemDriverVersion returns the the driver version on the system.
func SystemDriverVersion() {
	var driver [C.NVML_SYSTEM_DRIVER_VERSION_BUFFER_SIZE]C.char
	C.nvmlSystemGetDriverVersion(&driver[0], len(driver))
}

What did you expect to see?

No errors.

What did you see instead?

$ gotype nvml.go
nvml.go:21:39: index 0 (constant of type int) is out of bounds
@gopherbot gopherbot added this to the Unreleased milestone Feb 6, 2018
@rmmh
Copy link
Contributor Author

rmmh commented Feb 6, 2018 •
edited

One fix: change typexpr.go:378 (arrayLength) to return -1 for non-constant arrays-- this seems to be the correct option to indicate an "error/invalid-length" array and prevent cascading errors.

I can make a CL for that (with tests) if that's the correct route.

Related: #22090

@ianlancetaylor ianlancetaylor changed the title x/tools/cmd/gotype has errors for arrays with Cgo-defined lengths x/tools/cmd/gotype: fails on arrays with Cgo-defined lengths Feb 6, 2018
@ianlancetaylor
Copy link
Contributor

CC @griesemer

@cblecker cblecker mentioned this issue Feb 9, 2018
Merged
@griesemer griesemer self-assigned this Feb 12, 2018
@griesemer griesemer modified the milestones: Unreleased, Go1.11 Feb 12, 2018
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 12, 2018
@griesemer
Copy link
Contributor

@rmmh Setting the array length to -1 works here but it's a somewhat unintended consequence of how go/types handles [...]T{} array literals (note that arrays always should have a constant length). I'll make the change and will add respective comments. Thanks for looking into it.

@gopherbot
Copy link

Change https://golang.org/cl/93438 mentions this issue: go/types: handle arrays with unknown (Cgo-defined) lengths better

rmmh pushed a commit to rmmh/kubernetes that referenced this issue Feb 27, 2018
Vendor golang's go/types to include a fix for CGo typechecking.
c3acf07 
Ts is go/types forked from go@236abdb46b (just after 1.10) and
cherry-picked go@1006f703ffc, which fixes golang/go#23712.
It can be removed when all builders are >= go1.11.
jingxu97 pushed a commit to jingxu97/kubernetes that referenced this issue Mar 13, 2018
@jingxu97
Vendor golang's go/types to include a fix for CGo typechecking.
b30430f 
Ts is go/types forked from go@236abdb46b (just after 1.10) and
cherry-picked go@1006f703ffc, which fixes golang/go#23712.
It can be removed when all builders are >= go1.11.
@cblecker cblecker mentioned this issue Oct 25, 2018
Merged
@golang golang locked and limited conversation to collaborators Feb 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
4 participants
@rmmh @ianlancetaylor @griesemer @gopherbot