determine thread stack size with pthread API

[damaxwell]

Moves thread stack size computation out of Fiber and into Thread.

Switches to pthread APIs for computing stack size.

[ysbaddaden]

Thanks a lot!

The only bit I'm wary of, is whether the stack addr/size/guard is reported correctly for the main thread, since there isn't a standard and portable pthread_getattr. I investigated and wrote my results below. Only darwin seems to behave incorrectly for the main thread.

• darwin: pthread_get_stackaddr_np seems to be valid, but pthread_get_stacksize_np returns bogus values for the main thread (see https://opensource.apple.com/source/WTF/WTF-7602.1.50/wtf/StackBounds.cpp.auto.html). At least, it used to, and we should be conservative.

• freebsd: pthread_attr_get_np(3) seems correct (see ivmai/bdwgc@49d7fe7).

• linux-gnu: pthread_getattr_np(3) seems correct (per the manpage).

• linux-musl: pthread_getattr_np(3) seems compliant with the GNU extension (see https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_getattr_np.c).

• openbsd: pthread_stackseg_np(3) seems to take care of the main thread (see https://github.com/openbsd/src/blob/b66614995ab119f75167daaa7755b34001836821/lib/librthread/rthread_np.c#L78).

[ysbaddaden]

1. it seems pthread_get_stacksize_np can't be trusted for the main thread on Darwin;
2. maybe harmonize code style (pedantic) to improve readability?
3. some implementations (e.g. linux) report a guard size; could it be useful?

[ysbaddaden]

• Pedantic: can we harmonize the implementation within thread? I use ret (instead of err) and inline the check+raise.
• Leak: if any further call to pthread fails, the pthread_attr_t object is never destroyed (this applies to linux as well).

For example:

ret = LibC.pthread_attr_init(out attr)
raise Errno.new("pthread_attr_init", ret) unless ret == 0

begin
  ret = LibC.pthread_attr_get_np(@th, pointerof(attr))
  raise Errno.new("pthread_attr_get_np", ret) unless ret == 0

  # ...
ensure
  LibC.pthread_attr_destroy(pointerof(attr))
end

[damaxwell]

True enough about that leak. I had seen the same leak also in #6944 in the changes to ConditionVariable#initialize and had assumed that you hadn't used full care to release the resources because if these calls are failing the house is on fire, and maybe the leaky faucet isn't so worrisome. I'll fix this here and submit a separate PR for that other one.

[ysbaddaden]

Oh, I totally missed it! I only overlooked some possible errnos that are very unlikely (per the manpages) and not worth raising.

[damaxwell]

For the requested changes:

1. On Darwin, on the main thread, we fall back to getrlimit.
2. Switched to ret not err and tried to match nearby code style (throw ... unless ret == 0)
3. This was the biggest change. For platforms that do not have an equivalent of pthread_getattr_np we need to cache the thread attributes at the time of creation to access the guard size. The stack is returned as a separate Thread::Stack structure that maintains a pointer, a size, and an optional guard size, which is nil if is unknown (i.e. on the main process thread). If you think this is pedantic, we can simply report a 0 instead. My previous use of Slice(Void) to represent a stack turned out to be a problem because it uses an Int32 to represent its count, and (rarely, but in principle) a stack can have more bytes than that.

The leak was addressed as well. For consistency in the code base, probably ConditionVariable also ought to get fixed; it doesn't look like the errno's that could occur there are much less likely than the errno's addressed here.

[ysbaddaden]

Maybe use a more detailed explanation:

the fiber represents a thread's main stack and we assume it's the currently running thread; this should be fine since a thread should never resume the main fiber of another thread (undefined behavior? chaos?)

[RX14]

Why not pass the thread stack through the Fiber.new constructor. Then we simply have a fiber constructor for creating a fiber with a new stack and a fiber constructor for creating a fiber with an existing stack, and Fiber never has to worry about Thread.current.stack at all.

[straight-shoota]

I also think it's better to properly initialize @stack already in the constructor.

[damaxwell]

We can’t cache @stack in Fiber because it is dynamic in time. What is is at the time of Fiber construction need not be what it is when a segfault occurs later in the run. The stack bottom stays put but the stack size adjusts.

[ysbaddaden]

I did too much low level programming, I guess. It sounded safe enough, but it's better to be really safe.

Is the stack size really changing? We never tweak it, and I'm not sure we ever do.

If so, maybe linking a Fiber to its Thread when it represents a thread stack would be a better idea? We could then have something like @thread.try(&.stack) || @stack.

[damaxwell]

On x86_64-linux-musl at least, the stack sizes reported from pthread_attr_get_np & pthread_attr_getstack on the main thread reflect the current stack allocation, which can be much smaller than the ultimate limit, so it will grow in time. And although the Crystal code base never changes the limits on the main stack, there is nothing to stop a user from calling setrlimit, which will change the main stack size.

Saving the associated thread if the Fiber represents a thread's stack (main thread or otherwise), is the right thing to do.

[damaxwell]

Well, it turned out that it having the Thread know about its Fiber and the Fiber knowing about its Thread didn't mix well with the fact that Thread has a finalizer. Numerous GC Warning: Finalization cycle warnings were generated running the spec suite.

So instead, a Fiber returns an optional Thread::Stack which is Nil if the Fiber is not responsible for maintaining its stack (i.e. the main Fiber for a Thread). A Thread always knows about its own stack and can return it. If you want the current stack, talk to the Crystal::Scheduler, which knows both the current Thread and the current Fiber and can figure out what is the right stack to report.

[RX14]

This is basically fine

[bcardiff]

This looks great. My only input would be the same as #6970 (comment) but can be done later.

@ysbaddaden are you satisfied with the current behaviour?

@damaxwell would you mind rebasing over master?

[damaxwell]

@bcardiff: Rebased.