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: more expansive test coverage #39537

Open
mdempsky opened this issue Jun 11, 2020 · 6 comments
Open

cmd/cgo: more expansive test coverage #39537

mdempsky opened this issue Jun 11, 2020 · 6 comments
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 11, 2020

cgo is very subtle, yet it's lacking in test coverage. For example, misc/cgo/test doesn't have any tests for C.enum_foo or C.struct_bar, and misc/cgo/testgodefs doesn't have any tests for constants (to be addressed by CL 237558).

/cc @ianlancetaylor

@mdempsky mdempsky added this to the Backlog milestone Jun 11, 2020
@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Jun 29, 2020

I want to try to take this issue.
For the first step, I want to implement the test for enum_foo.
Should I add an extra file, test_enum.go, for testing?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 29, 2020

Ideally the test should be added to the existing files misc/cgo/test/test.go and/or misc/cgo/test/testx.go. Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 1, 2020

Change https://golang.org/cl/240697 mentions this issue: cgo: Test Allocating C enum with C.enum_*

@HowJMay
Copy link
Contributor

@HowJMay HowJMay commented Jul 28, 2020

Should I add some tests for union as well?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 30, 2020

@HowJMay Sure. Thanks.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 31, 2020

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

gopherbot pushed a commit that referenced this issue Jul 31, 2020
Consider this test package:

    package p

    // enum E { E0 };
    // union U { long x; };
    // void f(enum E e, union U* up) {}
    import "C"

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

In Go 1.14, cgo translated this to (omitting irrelevant details):

    type _Ctype_union_U [8]byte

    func f() {
    	_Cfunc_f(uint32(_Ciconst_E0), (*[8]byte)(nil))
    }

    func _Cfunc_f(p0 uint32, p1 *[8]byte) (r1 _Ctype_void) { ... }

Notably, _Ctype_union_U was declared as a defined type, but uses were
being rewritten into uses of the underlying type, which matched how
_Cfunc_f was declared.

After CL 230037, cgo started consistently rewriting "C.foo" type
expressions as "_Ctype_foo", which caused it to start emitting:

    type _Ctype_enum_E uint32
    type _Ctype_union_U [8]byte

    func f() {
    	_Cfunc_f(_Ctype_enum_E(_Ciconst_E0), (*_Ctype_union_U)(nil))
    }

    // _Cfunc_f unchanged

Of course, this fails to type-check because _Ctype_enum_E and
_Ctype_union_U are defined types.

This CL changes cgo to emit:

    type _Ctype_enum_E = uint32
    type _Ctype_union_U = [8]byte

    // f unchanged since CL 230037
    // _Cfunc_f still unchanged

It would probably be better to fix this in (*typeConv).loadType so
that cgo generated code uses the _Ctype_foo aliases too. But as it
wouldn't have any effect on actual compilation, it's not worth the
risk of touching it at this point in the release cycle.

Updates #39537.
Fixes #40494.

Change-Id: I88269660b40aeda80a9a9433777601a781b48ac0
Reviewed-on: https://go-review.googlesource.com/c/go/+/246057
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.