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

go/internal/srcimporter: TestImportStdLib failing on NetBSD #38649

Closed
ianlancetaylor opened this issue Apr 24, 2020 · 17 comments
Closed

go/internal/srcimporter: TestImportStdLib failing on NetBSD #38649

ianlancetaylor opened this issue Apr 24, 2020 · 17 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-NetBSD release-blocker
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

TestImportStdLib is failing on NetBSD on tip. As seen on

https://build.golang.org/log/fad56ee11c8b5522a96342ee2a936845758f1ab0

--- FAIL: TestImportStdLib (2.10s)
    srcimporter_test.go:30: import "archive/tar" failed (type-checking package "archive/tar" failed (/tmp/workdir/go/src/archive/tar/stat_unix.go:11:2: could not import os/user (type-checking package "os/user" failed (/tmp/workdir/go/src/os/user/getgrouplist_unix.go:25:43: gid_t not declared by package C))))
    srcimporter_test.go:43: testing time used up
    srcimporter_test.go:89: tested 1 imports
FAIL
FAIL	go/internal/srcimporter	6.949s

CC @mdempsky @griesemer

@ianlancetaylor ianlancetaylor added OS-NetBSD NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Apr 24, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Apr 24, 2020
@ianlancetaylor
Copy link
Contributor Author

Based on the build dashboard, this appears to have been broken by https://golang.org/cl/33677.

@mdempsky
Copy link
Member

Thanks for the heads up. I don't immediately see why that wouldn't work.

If someone can run go build -work os/user on NetBSD and then post os/user's _cgo_gotypes.go file, that would be handy.

@ianlancetaylor
Copy link
Contributor Author

On NetBSD, <sys/types.h> says

#ifndef gid_t
typedef __gid_t         gid_t;          /* group id */
#define gid_t           __gid_t
#endif

The type __gid_t is defined in <sys/ansi.h>, included by <sys/types.h>, as __uint32_t.

Because of the macro definition in <sys/types.h>, the generated _cgo_gotypes.go file defines _Ctype__gid_t and uses that wherever the original Go code uses C.gid_t. I assume that is confusing the rest of the code into thinking that there is no definition for C.gid_t. In the general case I think this code is going to need to be aware of preprocessor macros.

@mdempsky
Copy link
Member

@ianlancetaylor I see. That's frustrating.

Does cgo emit any information about preprocessor macros that go/types can make use of?

Could cgo emit a type alias for type _Ctype_gid_t = _Ctype___gid_t in this scenario?

@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Apr 24, 2020

_cgo_gotypes.go on amd64 NetBSD 9.0:

//go:cgo_ldflag "-g"
//go:cgo_ldflag "-O2"
// Code generated by cmd/cgo; DO NOT EDIT.

package user

import "unsafe"

import _ "runtime/cgo"

import "syscall"

var _ syscall.Errno
func _Cgo_ptr(ptr unsafe.Pointer) unsafe.Pointer { return ptr }

//go:linkname _Cgo_always_false runtime.cgoAlwaysFalse
var _Cgo_always_false bool
//go:linkname _Cgo_use runtime.cgoUse
func _Cgo_use(interface{})
type _Ctype___gid_t = _Ctype___uint32_t

type _Ctype___int64_t = _Ctype_long

type _Ctype___uid_t = _Ctype___uint32_t

type _Ctype___uint32_t = _Ctype_uint

type _Ctype_char int8

type _Ctype_int int32

type _Ctype_intgo = _Ctype_ptrdiff_t

type _Ctype_long int64

type _Ctype_ptrdiff_t = _Ctype_long

type _Ctype_size_t = _Ctype_ulong

type _Ctype_struct_group struct {
        gr_name         *_Ctype_char
        gr_passwd       *_Ctype_char
        gr_gid          _Ctype___gid_t
        gr_mem          **_Ctype_char
}

type _Ctype_struct_passwd struct {
        pw_name         *_Ctype_char
        pw_passwd       *_Ctype_char
        pw_uid          _Ctype___uid_t
        pw_gid          _Ctype___gid_t
        pw_change       _Ctype_time_t
        pw_class        *_Ctype_char
        pw_gecos        *_Ctype_char
        pw_dir          *_Ctype_char
        pw_shell        *_Ctype_char
        pw_expire       _Ctype_time_t
}

type _Ctype_time_t = _Ctype___int64_t

type _Ctype_uint uint32

type _Ctype_ulong uint64

type _Ctype_void [0]byte

//go:linkname _cgo_runtime_cgocall runtime.cgocall
func _cgo_runtime_cgocall(unsafe.Pointer, uintptr) int32

//go:linkname _cgo_runtime_cgocallback runtime.cgocallback
func _cgo_runtime_cgocallback(unsafe.Pointer, unsafe.Pointer, uintptr, uintptr)

//go:linkname _cgoCheckPointer runtime.cgoCheckPointer
func _cgoCheckPointer(interface{}, interface{})

//go:linkname _cgoCheckResult runtime.cgoCheckResult
func _cgoCheckResult(interface{})
const _Ciconst__SC_GETGR_R_SIZE_MAX = 0x2f
const _Ciconst__SC_GETPW_R_SIZE_MAX = 0x30


//go:linkname _cgo_runtime_gostring runtime.gostring
func _cgo_runtime_gostring(*_Ctype_char) string

func _Cfunc_GoString(p *_Ctype_char) string {
        return _cgo_runtime_gostring(p)
}

func _Cfunc__CMalloc(n _Ctype_size_t) unsafe.Pointer {
        return _cgo_cmalloc(uint64(n))
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_free
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_free _cgo_bbce630bddcc_Cfunc_free
var __cgofn__cgo_bbce630bddcc_Cfunc_free byte
var _cgo_bbce630bddcc_Cfunc_free = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_free)

//go:cgo_unsafe_args
func _Cfunc_free(p0 unsafe.Pointer) (r1 _Ctype_void) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_free, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_mygetgrgid_r
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrgid_r _cgo_bbce630bddcc_Cfunc_mygetgrgid_r
var __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrgid_r byte
var _cgo_bbce630bddcc_Cfunc_mygetgrgid_r = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_mygetgrgid_r)

//go:cgo_unsafe_args
func _Cfunc_mygetgrgid_r(p0 _Ctype_int, p1 *_Ctype_struct_group, p2 *_Ctype_char, p3 _Ctype_size_t, p4 **_Ctype_struct_group) (r1 _Ctype_int) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_mygetgrgid_r, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
                _Cgo_use(p2)
                _Cgo_use(p3)
                _Cgo_use(p4)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_mygetgrnam_r
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrnam_r _cgo_bbce630bddcc_Cfunc_mygetgrnam_r
var __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrnam_r byte
var _cgo_bbce630bddcc_Cfunc_mygetgrnam_r = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_mygetgrnam_r)

//go:cgo_unsafe_args
func _Cfunc_mygetgrnam_r(p0 *_Ctype_char, p1 *_Ctype_struct_group, p2 *_Ctype_char, p3 _Ctype_size_t, p4 **_Ctype_struct_group) (r1 _Ctype_int) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_mygetgrnam_r, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
                _Cgo_use(p2)
                _Cgo_use(p3)
                _Cgo_use(p4)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_mygetgrouplist
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrouplist _cgo_bbce630bddcc_Cfunc_mygetgrouplist
var __cgofn__cgo_bbce630bddcc_Cfunc_mygetgrouplist byte
var _cgo_bbce630bddcc_Cfunc_mygetgrouplist = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_mygetgrouplist)

//go:cgo_unsafe_args
func _Cfunc_mygetgrouplist(p0 *_Ctype_char, p1 _Ctype___gid_t, p2 *_Ctype___gid_t, p3 *_Ctype_int) (r1 _Ctype_int) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_mygetgrouplist, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
                _Cgo_use(p2)
                _Cgo_use(p3)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_mygetpwnam_r
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_mygetpwnam_r _cgo_bbce630bddcc_Cfunc_mygetpwnam_r
var __cgofn__cgo_bbce630bddcc_Cfunc_mygetpwnam_r byte
var _cgo_bbce630bddcc_Cfunc_mygetpwnam_r = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_mygetpwnam_r)

//go:cgo_unsafe_args
func _Cfunc_mygetpwnam_r(p0 *_Ctype_char, p1 *_Ctype_struct_passwd, p2 *_Ctype_char, p3 _Ctype_size_t, p4 **_Ctype_struct_passwd) (r1 _Ctype_int) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_mygetpwnam_r, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
                _Cgo_use(p2)
                _Cgo_use(p3)
                _Cgo_use(p4)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_mygetpwuid_r
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_mygetpwuid_r _cgo_bbce630bddcc_Cfunc_mygetpwuid_r
var __cgofn__cgo_bbce630bddcc_Cfunc_mygetpwuid_r byte
var _cgo_bbce630bddcc_Cfunc_mygetpwuid_r = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_mygetpwuid_r)

//go:cgo_unsafe_args
func _Cfunc_mygetpwuid_r(p0 _Ctype_int, p1 *_Ctype_struct_passwd, p2 *_Ctype_char, p3 _Ctype_size_t, p4 **_Ctype_struct_passwd) (r1 _Ctype_int) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_mygetpwuid_r, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
                _Cgo_use(p2)
                _Cgo_use(p3)
                _Cgo_use(p4)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_realloc
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_realloc _cgo_bbce630bddcc_Cfunc_realloc
var __cgofn__cgo_bbce630bddcc_Cfunc_realloc byte
var _cgo_bbce630bddcc_Cfunc_realloc = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_realloc)

//go:cgo_unsafe_args
func _Cfunc_realloc(p0 unsafe.Pointer, p1 _Ctype_size_t) (r1 unsafe.Pointer) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_realloc, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
                _Cgo_use(p1)
        }
        return
}
//go:cgo_import_static _cgo_bbce630bddcc_Cfunc_sysconf
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc_sysconf _cgo_bbce630bddcc_Cfunc_sysconf
var __cgofn__cgo_bbce630bddcc_Cfunc_sysconf byte
var _cgo_bbce630bddcc_Cfunc_sysconf = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc_sysconf)

//go:cgo_unsafe_args
func _Cfunc_sysconf(p0 _Ctype_int) (r1 _Ctype_long) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc_sysconf, uintptr(unsafe.Pointer(&p0)))
        if _Cgo_always_false {
                _Cgo_use(p0)
        }
        return
}

//go:cgo_import_static _cgo_bbce630bddcc_Cfunc__Cmalloc
//go:linkname __cgofn__cgo_bbce630bddcc_Cfunc__Cmalloc _cgo_bbce630bddcc_Cfunc__Cmalloc
var __cgofn__cgo_bbce630bddcc_Cfunc__Cmalloc byte
var _cgo_bbce630bddcc_Cfunc__Cmalloc = unsafe.Pointer(&__cgofn__cgo_bbce630bddcc_Cfunc__Cmalloc)

//go:linkname runtime_throw runtime.throw
func runtime_throw(string)

//go:cgo_unsafe_args
func _cgo_cmalloc(p0 uint64) (r1 unsafe.Pointer) {
        _cgo_runtime_cgocall(_cgo_bbce630bddcc_Cfunc__Cmalloc, uintptr(unsafe.Pointer(&p0)))
        if r1 == nil {
                runtime_throw("runtime: C malloc failed")
        }
        return
}

@ianlancetaylor
Copy link
Contributor Author

If you pass -debug-define to cgo it will emit #define lines on stderr that you can read to pick out redefinitions. I think that should suffice to handle this case. Maybe. Give it a try.

@mdempsky
Copy link
Member

Can repro on !netbsd by adding $GOROOT/misc/cgo/test/issue38649.go with contents:

package cgotest

// #define issue38649int int
import "C"

var _ C.issue38649int

and then running go test -run=TestCgo go/internal/srcimporter.

@mdempsky
Copy link
Member

@ianlancetaylor Thanks, I'll take a look at the -debug-define output. That might work for srcimporter, because it runs cgo itself; but in general, we don't currently have control over how cgo is invoked. E.g., go/packages on its underlying driver (e.g., cmd/go) to invoke cmd/cgo and supply the generated _cgo_gotypes.go file.

@ianlancetaylor
Copy link
Contributor Author

I don't know the issues here, but we could perhaps change cgo to emit special comments for macros in the _cgo_gotypes.go file.

@mdempsky
Copy link
Member

mdempsky commented Apr 24, 2020

@ianlancetaylor The goal is given the unmodified original Go source files and the _cgo_gotypes.go file generated from running cgo on them, to have go/types able to figure out the Go type of all C.foo identifiers.

The way CL 33677 does that is it has a list of all the prefixes that cgo currently emits, and it tries looking each of them up. E.g., for C.foo it looks up _Ciconst_foo, _Cfconst_foo, _Ctype_foo, _Cvar_foo, ... until one succeeds. It also knows like that _Cvar_foo is actually a pointer to the variable and so it needs an implicit dereference, or that _Cmacro_foo is a function that needs to be called to evaluate to the value.

The problem is here C.gid_t is being translated by cgo into an identifier not found using this approach.

It would be best for go/types if the special comments were spelled type _Ctype_gid_t = _Ctype___gid_t. :)

@mdempsky
Copy link
Member

Looking at (*Package).rewriteName, it looks like the expr = r.Name.Type.Go lines are the concerning ones.

It looks like those lines predate type aliases. I'd imagine we can just emit the type alias under the normal mangled name, and then use the normal mangling. I'll look into this.

@mdempsky mdempsky self-assigned this Apr 24, 2020
@gopherbot
Copy link

Change https://golang.org/cl/230037 mentions this issue: cmd/cgo: use type aliases for #define type macros

@gopherbot
Copy link

Change https://golang.org/cl/230038 mentions this issue: Revert "go/types: add UsesCgo config to support _cgo_gotypes.go"

@andybons
Copy link
Member

andybons commented Apr 27, 2020

Seems that there are multiple failures being masked: reverting https://golang.org/cl/33677 gives a different test failure on NetBSD:

--- FAIL: TestSegv (0.00s)

    --- FAIL: TestSegv/Segv (0.01s)
        crash_cgo_test.go:569: 
        crash_cgo_test.go:571: expected crash from signal
    --- FAIL: TestSegv/SegvInCgo (0.01s)
        crash_cgo_test.go:569: 
        crash_cgo_test.go:571: expected crash from signal
FAIL
FAIL	runtime	39.518s

If this is due to a different change, I’d really like us to revert liberally and soon to get us back into a passing state.

@andybons andybons added the Soon This needs to be done soon. (regressions, serious bugs, outages) label Apr 27, 2020
@gopherbot
Copy link

Change https://golang.org/cl/230309 mentions this issue: go/internal/srcimporter: disable TestCgo on NetBSD

@josharian
Copy link
Contributor

That is #38595 and it was supposed to be fixed by https://go-review.googlesource.com/c/go/+/229484. cc @ianlancetaylor

gopherbot pushed a commit that referenced this issue Apr 27, 2020
This reverts CL 33677.

Reason for revert: NetBSD is broken

Updates #38649

Change-Id: Id60e3c97d3cb4fb0053dea03b95dbbb0b850c883
Reviewed-on: https://go-review.googlesource.com/c/go/+/230038
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@andybons
Copy link
Member

Thanks, @josharian. Will keep an eye the builders and report back on that issue if it’s still failing with the same TestSegv failure.

@andybons andybons removed the Soon This needs to be done soon. (regressions, serious bugs, outages) label Apr 30, 2020
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
This reverts CL 33677.

Reason for revert: NetBSD is broken

Updates golang#38649

Change-Id: Id60e3c97d3cb4fb0053dea03b95dbbb0b850c883
Reviewed-on: https://go-review.googlesource.com/c/go/+/230038
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
xujianhai666 pushed a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
Cgo's initial design for handling "#define foo int*" involved
rewriting "C.foo" to "*_Ctype_int" everywhere. But now that we have
type aliases, we can declare "type _Ctype_foo = *_Ctype_int" once, and
then rewrite "C.foo" to just "_Ctype_foo".

This is important for go/types's UsesCgo mode, where go/types needs to
be able to figure out a type for each C.foo identifier using only the
information written into _cgo_gotypes.go.

Fixes golang#38649.

Change-Id: Ia0f8c2d82df81efb1be5bc26195ea9154c0af871
Reviewed-on: https://go-review.googlesource.com/c/go/+/230037
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators May 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-NetBSD release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants