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

runtime/cgo: x_cgo_getstackbound() broken on SmartOS #67353

Closed
bardo opened this issue May 14, 2024 · 9 comments
Closed

runtime/cgo: x_cgo_getstackbound() broken on SmartOS #67353

bardo opened this issue May 14, 2024 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos
Milestone

Comments

@bardo
Copy link

bardo commented May 14, 2024

Go version

go version go1.22.2 illumos/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/admin/.cache/go-build'
GOENV='/home/admin/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='illumos'
GOINSECURE=''
GOMODCACHE='/home/admin/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='illumos'
GOPATH='/home/admin/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/local/go122'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/local/go122/pkg/tool/illumos_amd64'
GOVCS=''
GOVERSION='go1.22.2'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/admin/code/navidrome/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1040456887=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Applications that rely on x_cgo_getstackbound() (in this case, Navidrome) are broken on SmartOS due to an incorrect macro check.

What did you see happen?

Error:

# runtime/cgo
gcc_stack_unix.c: In function 'x_cgo_getstackbound':
gcc_stack_unix.c:25:2: error: implicit declaration of function 'pthread_getattr_np'; did you mean 'pthread_getname_np'? [-Werror=implicit-function-declaration]
   25 |  pthread_getattr_np(pthread_self(), &attr);  // GNU extension
      |  ^~~~~~~~~~~~~~~~~~
      |  pthread_getname_np

What did you expect to see?

The current implementation of x_cgo_getstackbound() relies on the __illumos__ macro being defined platforms to call pthread_attr_init() as needed:

#if defined(__GLIBC__) || (defined(__sun) && !defined(__illumos__))
// pthread_getattr_np is a GNU extension supported in glibc.
// Solaris is not glibc but does support pthread_getattr_np
// (and the fallback doesn't work...). Illumos does not.
pthread_getattr_np(pthread_self(), &attr); // GNU extension
pthread_attr_getstack(&attr, &addr, &size); // low address
#elif defined(__illumos__)
pthread_attr_init(&attr);
pthread_attr_get_np(pthread_self(), &attr);
pthread_attr_getstack(&attr, &addr, &size); // low address

SmartOS, being Illumos-derived, requires the same treatment, however it does not define the __illumos__ macro, falling into the first branch of the #ifdef and leading to the error.

Test program:

#include <stdio.h>

int main(int argc, char **argv) {
#if defined(__GLIBC__)
        printf("__GLIBC__\n");
#endif

#if defined(__sun)
        printf("__sun\n");
#endif

#if defined(__illumos__)
        printf("__illumos__\n");
#endif

        return 0;
}

Output:

$ gcc test.c -o test
$ ./test
__sun
$ 
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 14, 2024
@ianlancetaylor
Copy link
Contributor

Does SmartOS have a different macro that can be tested with #if?

Would you like to send a patch to fix this? See https://go.dev/doc/contribute. Thanks.

@bardo
Copy link
Author

bardo commented May 14, 2024

Unfortunately I don't know much about SmartOS internals, I just found the issue and I happened to have some time to investigate. I am also unable to contribute a fix directly, even though I will try to find a proper solution.

Looking through the output of cpp -dM /dev/null | sort I see nothing specific. The only platform-related macros I see are:

#define __sun 1
#define __sun__ 1                                    
#define __SVR4 1                                     
#define __svr4__ 1
#define __unix 1
#define __unix__ 1
#define sun 1
#define unix 1

I don't have a machine running another Illumos flavor to check, but I do not expect them to be unique to SmartOS.

@dmitshur
Copy link
Contributor

CC @golang/illumos.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos labels May 14, 2024
@dmitshur dmitshur added this to the Backlog milestone May 14, 2024
@jclulow
Copy link
Contributor

jclulow commented May 14, 2024

G'day!

@bardo The __illumos__ macro is definitely the correct one to be using here. For GCC it's something that gets built into the specific compiler binary. Can you tell me which version of GCC isn't providing the macro for you, and where you obtained it?

Thanks, and sorry for the trouble here!

@bardo
Copy link
Author

bardo commented May 14, 2024

@jclulow no need to apologize at all. In fact, answering your question clarified quite a bit. I was wondering myself what the rationale would be behind not setting __illumos__ on SmartOS, but I just assumed there was a good reason, so I opened the issue here. Turns out, it's always good to challenge assumptions.

The short answer is that this is probably a limitation of the pkgsrc system, coupled with poor maintenance on my side.

The longer answer is that GCC binary comes from the gcc9-9.3.0 package, which indeed is the only GCC version I had installed:

# pkg_alternatives list | grep gcc
gcc9-9.3.0
# pkgin ls | grep ^gcc
gcc10-libs-10.4.0    The GNU Compiler Collection (GCC) support shared libraries
gcc12-libs-12.2.0    The GNU Compiler Collection (GCC) support shared libraries
gcc13-libs-13.2.0    The GNU Compiler Collection (GCC) support shared libraries
gcc9-9.3.0           The GNU Compiler Collection (GCC) - 9.x Release Series
gcc9-libs-9.3.0      The GNU Compiler Collection (GCC) support shared libraries

However, that package is not present in the repositories (anymore).

# pkgin se gcc | grep ^gcc
gcc13-13.2.0 =       The GNU Compiler Collection (GCC) - 13.x Release Series
gcc13-libs-13.2.0 =  The GNU Compiler Collection (GCC) support shared libraries
gcc7-7.5.0nb6 =      The GNU Compiler Collection (GCC) - 7.0 Release Series
gcc7-libs-7.5.0nb7   The GNU Compiler Collection (GCC) support shared libraries
gccmakedep-1.0.4     Create dependencies in Makefiles using gcc

So my reasoning is that it has been removed in favor of newer versions, even though pkgsrc's inability (or sane unwillingness) to do a major version upgrade silently left the outdated version around. I imagine that GCC7 is there just for compatibility reasons.

Curious, I installed both available versions to check for the macro, and indeed:

$ /opt/local/gcc7/bin/cpp -dM /dev/null | grep __illumos__
$ /opt/local/gcc13/bin/cpp -dM /dev/null | grep __illumos__
#define __illumos__ 1
$

Meaning that it has been added somewhere between GCC10 and GCC13. So my specific issue is fixed, however the cgo minimum requirements report GCC4.6, potentially putting GCC7 firmly in scope. Is an issue needed on the SmartOS side or can we trust that a newer GCC will be used when available?

@ianlancetaylor
Copy link
Contributor

I think we can say that the minimum GCC version on SmartOS is GCC 13.

@bardo
Copy link
Author

bardo commented May 15, 2024

Great, feel free to close here, then, and apologies for the noise.

@jclulow
Copy link
Contributor

jclulow commented May 15, 2024

I will investigate and get back to you with some specifics on our end!

@ianlancetaylor
Copy link
Contributor

Thanks.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale May 15, 2024
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. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos
Projects
None yet
Development

No branches or pull requests

5 participants