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/cgo: cgo parameter type mismatch between generated go wrapper and call to wrapper #40494

Closed
somersf opened this issue Jul 30, 2020 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@somersf
Copy link
Contributor

somersf commented Jul 30, 2020

An example cgo program which passes an enum and a union to an external C function previously compiled successfully, but fails to compile with go1.15rc1.

Example code is provided below with concrete types/signatures, but in summary:

  • Previously, an enum parameter was simplified to a uint32 in the generated go wrapper for the C function, and also in the generated go code which calls the wrapper. Similarly, a union was simplified to a *[num]byte.
  • With go1.15rc1, the enum and union are simplified in the generated go wrapper for the C function, but are not simplified in the generated go code which calls the wrapper, leading to type mismatch errors.

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

$ go version
go version go1.15rc1 linux/386

( also with GOARCH=amd64 )

Does this issue reproduce with the latest release?

The example fails with compilation errors with go1.15rc1 but compiles successfully with multiple versions <= go1.14.6.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="386"
GOBIN=""
GOCACHE="/bld/go-build-cache"
GOENV="/home/homedir/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="386"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/homedir/15-issues/cgo-simplified/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/homedir/15-issues/cgo-simplified"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_386"
GCCGO="gccgo"
GO386="sse2"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/homedir/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m32 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build634173991=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Here is a fairly minimal example for an apply package, consisting of apply.go, apply.h and apply.cpp. It was built using go build . within the apply package directory.

apply.go

package apply

// #include "apply.h"
import "C"

import (
        "unsafe"
)

const (
        OP_SET = 0
        OP_CLR = 1
)

func Apply(op int) {
        params := unsafe.Pointer(nil)
        
        /* Example of why this construct is used, but can be omitted
           to reproduce the error.
        switch op {
        case OP_SET:
                set_params := C.struct_set_params{
                        int_val: 1,
                }
                params = unsafe.Pointer(&set_params)
        case OP_CLR:
                clr_params := C.struct_clr_params{
                        long_val: 2,
                }
                params = unsafe.Pointer(&clr_params)
        }
        */
        
        C.Apply(C.enum_operation_type(op), (*C.union_operation_params)(params))
}

apply.h

#ifndef APPLY_H
#define APPLY_H

#ifdef __cplusplus
extern "C" {
#endif

struct set_params {
  int int_val;
};

struct clr_params {
  long long_val;
};

union operation_params {
  struct set_params op_set;
  struct clr_params op_clr;
};

enum operation_type {
  OP_SET,
  OP_CLR,
};

void Apply(enum operation_type op_type, union operation_params * op_params);

#ifdef __cplusplus
}
#endif

#endif

apply.cpp

#include "apply.h"

void Apply(enum operation_type op_type, union operation_params * op_params) {
}

What did you expect to see?

Compiling with go1.14.6 is successful, so I'd expected it would also succeed with go1.15rc1.

What did you see instead?

% cd apply
% go build .
# tools/15-issues/cgo-simplified/src/apply
./apply.go:31:31: cannot use _Ctype_enum_operation_type(op) (type _Ctype_enum_operation_type) as type uint32 in argument to _Cfunc_Apply
./apply.go:31:64: cannot use (*_Ctype_union_operation_params)(params) (type *_Ctype_union_operation_params) as type *[4]byte in argument to _Cfunc_Apply

Inspecting the go build cache, I found the generated C function wrapper:

//go:cgo_unsafe_args
func _Cfunc_Apply(p0 uint32, p1 *[4]byte) (r1 _Ctype_void) { ... }

and the generated call from apply.go to it (line-wrapped):

func Apply(op int) {
        params := unsafe.Pointer(nil)
        ...
        ( /*line :31:2*/_Cfunc_Apply /*line :31:8*/)(
            /*line :31:10*/_Ctype_enum_operation_type /*line :31:31*/(op),
            (* /*line :31:39*/_Ctype_union_operation_params /*line :31:63*/)(params))
}

This fails to compile because the call parameter types (which do match the C declaration) don't match the generated go wrapper definition.

For comparison, the go1.14.6 generated C function wrapper is essentially the same (the _Cfunc_Apply signature matches exactly, but there are other differences in the same file), but the call from apply.go differs (line-wrapped):

func Apply(op int) {
        params := unsafe.Pointer(nil)
        ...
        ( /*line :31:2*/_Cfunc_Apply /*line :31:8*/)(
            /*line :31:10*/uint32 /*line :31:31*/(op),
            (* /*line :31:39*/[4]byte /*line :31:63*/)(params))
}

In go1.14, the compiler has emitted call parameter types which match the generated wrapper.

Presumably the correct thing to do would be for compiler to use the correct rather than simplified type names for both the generated C function wrapper and the calls to that wrapper (or at least be consistent).

@cagedmantis cagedmantis changed the title cgo: parameter type mismatch between generated go wrapper and call to wrapper cmd/go: cgo parameter type mismatch between generated go wrapper and call to wrapper Jul 30, 2020
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 30, 2020
@cagedmantis cagedmantis added this to the Go1.15 milestone Jul 30, 2020
@cagedmantis
Copy link
Contributor

/cc @bcmills @jayconrod @matloob

@bcmills bcmills changed the title cmd/go: cgo parameter type mismatch between generated go wrapper and call to wrapper cmd/cgo: cgo parameter type mismatch between generated go wrapper and call to wrapper Jul 30, 2020
@bcmills
Copy link
Contributor

bcmills commented Jul 30, 2020

CC @ianlancetaylor

@aclements
Copy link
Member

Bisect says this was CL 230037, "cmd/cgo: use type aliases for #define type macros". cc @mdempsky

@mdempsky
Copy link
Member

Thanks, I can reproduce this. Looking.

@mdempsky mdempsky self-assigned this Jul 30, 2020
@mdempsky
Copy link
Member

mdempsky commented Jul 30, 2020

Here's a minimal, standalone reproducer for the issue:

package p

/*
enum E { E0 };
void f(enum E e) {}

union U { long x; };
void g(union U* up) {}
*/
import "C"

func f() {
	C.f((C.enum_E)(C.E0))
	C.g((*C.union_U)(nil))
}

Still looking into this. I'm thinking the fix may be to change _Ctype_enum_* and _Ctype_union_* to use type aliases. Right now they're defined types, but then uses of them are inconsistently rewritten to the underlying type.

@gopherbot
Copy link

Change https://golang.org/cl/246057 mentions this issue: cmd/cgo: fix mangling of enum and union types

@mdempsky
Copy link
Member

@somersf CL 246057 fixes your test case for me. Do you mind verifying that it fixes your original issue?

@aclements I think @ianlancetaylor is out this week. I don't know if this can wait until Monday, or if I should find someone else to review/approve it before then?

@somersf
Copy link
Contributor Author

somersf commented Jul 31, 2020

@mdempsky Thanks for the prompt investigation

@somersf CL 246057 fixes your test case for me. Do you mind verifying that it fixes your original issue?

That CL passes for the original package where I hit the issue. I've kicked off a test across the full codebase, but it'll need overnight to complete.

@mdempsky
Copy link
Member

@somersf Great, thank you for the confirmation. I've submitted the fix. If your overnight testing reveals any further issues, please let us know.

@golang golang locked and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants