Skip to content

osx: thread id and signal stack support #1373

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.

Sign up for GitHub

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

Merged
mattklein123 merged 4 commits into from
Aug 2, 2017
Merged

osx: thread id and signal stack support #1373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3beb0c5
osx: thread id and signal stack support
zuercher Jul 31, 2017
1dd74dd
switch to pthread_mach_thread_np(pthread_self()) for thread id
zuercher Aug 2, 2017
50efa9f
switch to pthread_threadid_np
zuercher Aug 2, 2017
6b2e1a3
fail with an error when currentThreadId is unimplemented
zuercher Aug 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion source/common/common/thread.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
#include "common/common/thread.h"

#ifdef __linux__
#include <sys/syscall.h>
#elif defined(__APPLE__)
#include <pthread.h>
#endif

#include <functional>

Expand All @@ -21,7 +25,17 @@ Thread::Thread(std::function<void()> thread_routine) : thread_routine_(thread_ro
UNREFERENCED_PARAMETER(rc);
}

int32_t Thread::currentThreadId() { return syscall(SYS_gettid); }
int32_t Thread::currentThreadId() {
#ifdef __linux__
Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: could you either add #else case or use Linux as the fallback? i.e.

#if defined(__linux__)
  return syscall(SYS_gettid);
#elif defined(__APPLE__)
  uint64_t tid;
  pthread_threadid_np(NULL, &tid);
  return tid;
#else
#error "OS not supported."
#endif

or

#ifdef __APPLE__
  uint64_t tid;
  pthread_threadid_np(NULL, &tid);
  return tid;
#else
  return syscall(SYS_gettid);
#endif

otherwise we're going to end with code that doesn't have return value on other OSes.

Either one is fine with me, assuming @mattklein123 doesn't have any objections.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought about this. Agree this would be a bit nicer. Either way the compile will fail so not sure it matters that much. No preference from me about either of these or leaving what we have.

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an #error directive.

return syscall(SYS_gettid);
#elif defined(__APPLE__)
uint64_t tid;
pthread_threadid_np(NULL, &tid);
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use this on all platforms now and lose ifdef? feel free to return uint64_t and lose the static cast if you want.

Copy link
Copy Markdown
Contributor

@PiotrSikora PiotrSikora Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no pthread_threadid_np() on Linux.

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, ok. nevermind me. :)

Copy link
Copy Markdown
Member Author

@zuercher zuercher Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function doesn't exist on Linux.

return static_cast<int32_t>(tid);
#else
#error "Enable and test pthread id retrieval code for you arch in thread.cc"
#endif
}

void Thread::join() {
int rc = pthread_join(thread_id_, nullptr);
Expand Down
13 changes: 13 additions & 0 deletions source/exe/signal_action.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) {
#ifdef REG_RIP
// x86_64
error_pc = reinterpret_cast<void*>(ucontext->uc_mcontext.gregs[REG_RIP]);
#elif defined(__APPLE__) && defined(__x86_64__)
error_pc = reinterpret_cast<void*>(ucontext->uc_mcontext->__ss.__rip);
#else
#warning "Please enable and test PC retrieval code for your arch in signal_action.cc"
// x86 Classic: reinterpret_cast<void*>(ucontext->uc_mcontext.gregs[REG_EIP]);
Expand Down Expand Up @@ -57,13 +59,24 @@ void SignalAction::installSigHandlers() {
}

void SignalAction::removeSigHandlers() {
#if defined(__APPLE__)
// ss_flags contains SS_DISABLE, but Darwin still checks the size, contrary to the man page
if (previous_altstack_.ss_size < MINSIGSTKSZ) {
previous_altstack_.ss_size = MINSIGSTKSZ;
}
#endif
RELEASE_ASSERT(sigaltstack(&previous_altstack_, nullptr) == 0);

int hidx = 0;
for (const auto& sig : FATAL_SIGS) {
RELEASE_ASSERT(sigaction(sig, &previous_handlers_[hidx++], nullptr) == 0);
}
}

#if defined(__APPLE__) && !defined(MAP_STACK)
#define MAP_STACK (0)
#endif

void SignalAction::mapAndProtectStackMemory() {
// Per docs MAP_STACK doesn't actually do anything today but provides a
// library hint that might be used in the future.
Expand Down
6 changes: 5 additions & 1 deletion source/exe/signal_action.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include <signal.h>
#include <unistd.h>

#include <algorithm>

#include "common/common/non_copyable.h"

#include "server/backtrace.h"
Expand Down Expand Up @@ -47,7 +49,9 @@ namespace Envoy {
class SignalAction : NonCopyable {
public:
SignalAction()
: guard_size_(sysconf(_SC_PAGE_SIZE)), altstack_size_(guard_size_ * 4), altstack_(nullptr) {
: guard_size_(sysconf(_SC_PAGE_SIZE)),
altstack_size_(std::max(guard_size_ * 4, static_cast<size_t>(MINSIGSTKSZ))),
altstack_(nullptr) {
mapAndProtectStackMemory();
installSigHandlers();
}
Expand Down