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: map sized integer types to C equivalents #29878

Open
phst opened this issue Jan 22, 2019 · 13 comments
Open

cmd/cgo: map sized integer types to C equivalents #29878

phst opened this issue Jan 22, 2019 · 13 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@phst
Copy link
Contributor

phst commented Jan 22, 2019

(Removing the template since this is an enhancement proposal)

The type mappings at https://golang.org/cmd/cgo/#hdr-Go_references_to_C don't state which Go types the sized C integer types int8_t etc. are mapped to. In C, these types have the same requirements as the corresponding Go types: they are not allowed to have padding bits and have to use two's complement representation, see https://en.cppreference.com/w/c/types/integer. Therefore, these types are guaranteed to have the same domain and representation as the corresponding Go types. So I propose to always represent these types with each other and formally document that fact. It would reduce the need for casts and would make it more obvious that there's no chance of data loss.

@ianlancetaylor
Copy link
Contributor

Fine with me.

@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jan 22, 2019
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jan 22, 2019
phst added a commit to phst/go that referenced this issue Jan 23, 2019
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer.  This ensures that
they have the same domain and representation as the corresponding Go types.

Fixes golang#29878
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/159258 mentions this issue: cmd/cgo: use C exact-width integer types to represent Go types

@dpinela
Copy link
Contributor

dpinela commented Jan 23, 2019

I'm looking into this, is updating the (*typeConv).Type method in src/cmd/cgo/gcc.go sufficient to implement the equivalence? A quick experiment seems to show that it is.

edit: ninja'd. (And the answer turns out to be no.)

phst added a commit to phst/go that referenced this issue Jan 24, 2019
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer.  This ensures that
they have the same domain and representation as the corresponding Go types.

Fixes golang#29878
phst added a commit to phst/go that referenced this issue Mar 13, 2019
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer.  This ensures that
they have the same domain and representation as the corresponding Go types.

Fixes golang#29878
phst added a commit to phst/go that referenced this issue Mar 13, 2019
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer.  This ensures that
they have the same domain and representation as the corresponding Go types.

Fixes golang#29878
phst added a commit to phst/go that referenced this issue Mar 18, 2019
The exact-width integer types are required to use two’s complement
representation and may not have padding bits, cf. §7.20.1.1/1 in the C11
standard or https://en.cppreference.com/w/c/types/integer.  This ensures that
they have the same domain and representation as the corresponding Go types.

Fixes golang#29878
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/168337 mentions this issue: cmd/cgo: accept __uint8_t as the uint8_t type

gopherbot pushed a commit that referenced this issue Mar 19, 2019
This works around the NetBSD <stdint.h> which defines the type using
"#define" rather than typedef.

Fixes #30918
Updates #29878

Change-Id: I8998eba52139366ae46762bdad5fcae85f9b4027
Reviewed-on: https://go-review.googlesource.com/c/go/+/168337
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ianlancetaylor
Copy link
Contributor

This seemed like a reasonable idea, but it breaks existing code (see #31093) and I don't see any way to fix that without losing good error reporting. So I'm going to roll back the two changes and leave this issue closed. Sorry. Perhaps this would have been a good approach if we had started that way, but we didn't.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/180357 mentions this issue: cmd/cgo: roll back "use C exact-width integer types to represent Go types"

gopherbot pushed a commit that referenced this issue Jun 5, 2019
…ypes"

Roll back CL 159258 and CL 168337. Those changes broke existing
code. I can't see any way to keep existing code working while also
producing good error messages for types like C.ulong (such as the ones
already tested for in misc/cgo/errors).

This is not an exact roll back because parts of the code have changed
since those CLs.

Updates #29878
Fixes #31093

Change-Id: I56fe76c167ff0ab381ed273b9ca4b952402e1434
Reviewed-on: https://go-review.googlesource.com/c/go/+/180357
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Jun 5, 2019

I don't see any way to fix that without losing good error reporting.

Could you elaborate? It seems like we could define ulong, ushort, etc. as type aliases for the corresponding exact-width C types, but I haven't thought through the full implications of that.

(See also #13467.)

@ianlancetaylor
Copy link
Contributor

I tried exactly that--defining them as type aliases--and the effect was that the misc/cgo/error err2.go test failed. It failed because expressions like C.ushort("x") gave an error referring to an error converting to the type uint16, rather than the type C.ushort. I don't think this change gives us enough of an improvement to merit making the error messages that much more confusing.

@phst
Copy link
Contributor Author

phst commented Aug 10, 2019

Can this issue be reopened now that the PR has been reverted?

@ianlancetaylor
Copy link
Contributor

Above I commented that I was going to leave the issue closed, because I don't think there is a way to do this that doesn't break existing code. Do you see a way?

@bcmills
Copy link
Contributor

bcmills commented Aug 12, 2019

It seems to me that the way to do it would be a two-step process:

  1. Improve the diagnostics emitted by the compiler for aliased types (tracked in cmd/compile: errors involving aliased types should use the alias name #21866).

  2. Change cgo to define and use type aliases for all numeric C types (so that, for example, if C's unsigned long type is 64 bits, both _Ctype_ulong and _Ctype_uint64_t are aliases for uint64, not new defined types).

To my knowledge, no one is working on part (1), but that issue is still open. If/when it is addressed, then this issue could likely also be addressed.

@phst
Copy link
Contributor Author

phst commented Aug 15, 2019

Above I commented that I was going to leave the issue closed, because I don't think there is a way to do this that doesn't break existing code. Do you see a way?

Ah sorry, I had overlooked that statement. I don't really see a good way either. However, if #29878 (comment) is true, then this might be an option.

@ianlancetaylor
Copy link
Contributor

OK, reopening.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
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. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants