battleblow / openjdk-jdk11u

BSD port of OpenJDK 11
GNU General Public License v2.0
9 stars 8 forks source link

Hotspot posix: build fix #42

Closed devnexen closed 5 years ago

devnexen commented 5 years ago

NetBSD does not define this constant, here to 16kb.

devnexen commented 5 years ago

Sure make sense np.

battleblow commented 5 years ago

FWIW, my NetBSD branch had this defined as MINSIGSTKSZ rather than putting in a hardcoded literal. On the NetBSD 8.0/amd64 VM that ends up being 8kb rather than 16kb.

For FreeBSD, at least, this is how PTHREAD_STACK_MIN is ultimately defined as well. I'm not sure about OpenBSD.

battleblow commented 5 years ago

My only other NetBSD source change was to address problems with the definition of static_assert in the bundled version of harfbuzz. The current change is just a hack to #undef it on NetBSD so that the internal definition is used. The most important make change I had was to disable some more gcc warnings in CompileJvm.gmk

krytarowski commented 5 years ago

The right thing to do on NetBSD is to not assume any minimal size as it does not exist, it's a runtime flexible value depending on stack guard, cpu, kernel..

krytarowski commented 5 years ago

We deliberately do not define it and portable code shall not make any assumptions that it exists, it's an optional part of POSIX.

battleblow commented 5 years ago

Maybe so, but we have to pick something to use on NetBSD as the minimum stack size. A bit of research suggests that a way to do this is to use sysconf(_SC_THREAD_STACK_MIN). I'm basing this on https://mail-index.netbsd.org/tech-userlevel/2014/08/15/msg008729.html Unless there are objections I'd like to proceed with that.

krytarowski commented 5 years ago

Oops, good catch. I will ask to remove this value (its return is invalid anyway).

And no, there is no need to enforce some minimal value. A portable code is supposed to not make any assumptions about this.

krytarowski commented 5 years ago

As an example, libpthread(3) sets currently by default the stack guard to 65536 bytes.. it makes the currently hardcoded value questionable.

bsdkurt commented 5 years ago

@battleblow Spotty internet on my trip so just a quick reply on this conversation.

I think we should leave the currently committed compatibility define at 16k (or perhaps larger) for the time being for a few reasons. @krytarowski has indicated that he intends to raise the removal of sysconf(_SC_THREAD_STACK_MIN) so while a good option for NetBSD I think we should wait until that coversation is had on a NetBSD mailing list and resolved. While PTHREAD_STACK_MIN is an optional value in POSIX the realiality is that either a constant or sysctl should be provided by the OS so that application developers don't have to write code that probes for the minimum value by attempting to set via pthread_attr_ setstacksize() failing and increasing the value until success. The comment about stack guard size is orthogonal to the min value as POSIX defines it as additional space:

If a thread's stack is created with guard protection, the implementation allocates extra memory at the overflow end of the stack as a buffer against stack overflow of the stack pointer.

I'm thinking we can leave it as is and when NetBSD settles on PTHREAD_STACK_MIN or sysconf(_SC_THREAD_STACK_MIN) it can be adjusted at that time.

@krytarowski it would be great if you could link back the conversation on sysconf(_SC_THREAD_STACK_MIN) so we know the direction NetBSD will be taking.

krytarowski commented 5 years ago

I will prompt the developers, maybe there will be a change of the politics and we will define PTHREAD_STACK_MIN as sysconf(_SC_THREAD_STACK_MIN)... we will see.

krytarowski commented 5 years ago

OK, go for sysconf(_SC_THREAD_STACK_MIN). It looks like some people changed their mind. Whenever we will need to tune it, we will be able to do it inside the distribution.

krytarowski commented 5 years ago
#ifndef PTHREAD_STACK_MIN
#ifdef _SC_THREAD_STACK_MIN
#define PTHREAD_STACK_MIN sysconf(_SC_THREAD_STACK_MIN)
#else
#define PTHREAD_STACK_MIN (4 * 4096)
#endif
#endif

or similar.

battleblow commented 5 years ago

Thanks Kamil! Appreciate you getting us guidance on how to do this correctly for NetBSD. David has gone ahead and made a change to do that.