gofrontend: RawSockaddr.Data should be uint8 instead of int8 on s390/s390x

[bryanpkc]

gccgo declares the RawSockaddr.Data field as []int8. On s390/s390x, this field should be a []uint8, since the corresponding C field is a char array, and the char type is unsigned by default. This issue is preventing some code from building correctly with both go and gccgo, as previously described in issue #11469.

The same problem also affects RawSockaddrUnix which has a Path field defined as []int8.

[OneOfOne]

Is there an actual instance where it should be int8 instead of uint8?

[bryanpkc]

On platforms where the C char type defaults to being signed (e.g. x86_64), these Go types will need to be int8.

[ianlancetaylor]

See libgo/go/syscall/socket_linux_ppc64x_type.go for the solution to the same problem on ppc64x.

[vogtd]

I can make a patch for gofrontend but I'm a bit out of practice. Is that part of libgo just a copy of the golang sources now? So, is it necessary to make a separate patch for gofrontend?

[bryanpkc]

@vogtd: gofrontend's libgo is not part of golang sources. The patch needs to be made against https://github.com/golang/gofrontend.

[ianlancetaylor]

Well, most of libgo is copied from the gc sources, with some local patches, but the syscall package is almost entirely different.

The important thing for this specific type is that gccgo and gc agree on the type.

[laboger]

Here's the CL for how I fixed it for ppc64/ppc64le:
https://go-review.googlesource.com/#/c/11946/

[bryanpkc]

@laboger syscall.RawSockaddrUnix.Path is declared [108]int8 in ztypes_linux_ppc64.go, even though the corresponding field declared in <linux/un.h> is a char array. Does the Path field need to be changed to [108]uint8, just like RawSockaddr.Data?

[vogtd]

Proposed fix is here:
https://go-review.googlesource.com/#/c/20096/

[vogtd]

Tested within the Gccgo sources on s390x.

[gopherbot]

CL https://golang.org/cl/20096 mentions this issue.

[laboger]

@bryanpkc The original problem with RawSockaddr was found because gccgo and gc from golang had different declarations for the RawSockaddr structure. That caused problems when building Docker because the two compilers were not consistent in their compiling of Go source code. For RawSockaddrUnix.Path, I can see that gc has a special case for powerpc64 to force the structure to be a signed char, and that matches how gccgo declares it.

In syscall/types_linux.go:
struct my_sockaddr_un {
sa_family_t sun_family;
#if defined(__ARM_EABI__) || defined(__powerpc64__)
// on ARM and PPC char is by default unsigned
signed char sun_path[108];
#else
char sun_path[108];
#endif

This forces the declaration of RawSockaddrUnix in gccgo and gc to be the same on powerpc64. Given my past experience with the RawSockaddr change I don't think it is worth changing for powerpc64 because of the compatibility issues it will introduce with the compilers. I'd have to change in both.

Are you asking this because for s390 gccgo declares this as signed but gc declares it unsigned? If so then the above ifdef could be changed to include s390 or you could change gccgo so they match. I think the compilers must be consistent in how they compile Go code.

[bryanpkc]

@vogtd Thanks for the patch. The way I solved the issue with RawSockaddrUnix in gc was by casting the byte array being written to, as shown on line 304 here. Doing this avoids having to write different versions of the function for different platforms. What do you think?

@laboger Yes, what you said is correct. I have checked the history; the code was added for ARM in this commit. Should we keep doing this for all char types on all platforms where the default is not signed? I don't think it makes sense to handle sockaddr.sa_data one way and sockaddr_un.sun_path a different way. @davecheney @rsc, any comment?

[vogtd]

What do I have to do to get that code review going?

[bradfitz]

@ianlancetaylor, ping on https://go-review.googlesource.com/#/c/20096/ (15 days old).

[ianlancetaylor]

Replied on the CL. Sorry for the slow reply; it's been busy.

[mundaym]

I think this issue can be closed. We decided against changing the API in this way (see the discussion on CL 20096).