Refactor signal handling and replace signal handlers with thread-safe functions #1053
Conversation
…edicated to handling signals of the system
…) and CheckChilds() from TUnixSystem.cxx
gRootDir should only be set in TUnixSystem. Switch call order to insure it is set when needed
|
Starting build on |
|
Build failed on ubuntu14/native. Failing tests: |
|
@zzxuanyuan You can build on MacOS if you have a full version of XCode installed (command line tools only are not enough). If you have CVMFS, you can get some of the externals from there too. Just let me know and I'll show you how to do it. |
|
@zzxuanyuan - any updates on getting this to work on OS X? |
|
@zzxuanyuan any updates on this PR? |
amadio
left a comment
amadio
left a comment
There was a problem hiding this comment.
I think this pull request needs to be changed to avoid ifdefs, as there are too many, and to not add something and change it later, but just add the final version directly. Otherwise, it should be squashed upon merging, as the history here is not useful (e.g. adding file with Author: Fons Rademakers 15/09/95 to then change Author... in a later commit, adding Exit(), changing to gSystem->Exit() then removing the added code in later commit, etc).
| #pragma link C++ class TSubString; | ||
| #pragma link C++ class TSysEvtHandler; | ||
| #pragma link C++ class TSigHandling+; | ||
| #pragma link C++ class TSignalManager+; |
There was a problem hiding this comment.
Please amend the original commit instead of adding it with one name and then renaming.
| fcntl(fd, F_SETFL, flags); | ||
| } | ||
| return -ETIMEDOUT; | ||
| } else { } |
| if (complete == -1) { | ||
| if (errno == EINTR) { continue; } | ||
| else if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) { continue; } | ||
| else { |
There was a problem hiding this comment.
This is not following coding conventions. Please format new code appropriately before committing.
| SignalSafeErrWrite("Unable to pre-allocate executable information"); | ||
| return; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Please remove this #ifdef and use TROOT::GetEtcDir() or equivalent.
| syscall(SYS_execve, "/bin/sh", argv, __environ); | ||
| #else | ||
| execv("/bin/sh", argv); | ||
| #endif |
There was a problem hiding this comment.
Why not use the same for Linux and non-Linux? What's the advantage of this and other similar #ifdefs?
For instance, why not use evecve() from unistd.h? It's a POSIX interface.
core/unix/src/TUnixSigHandling.cxx
Outdated
| SignalSafeErrWrite("(Failed to wait on stack dump output.)\n"); | ||
| Exit(1, kFALSE); | ||
| } else { | ||
| Exit(0, kFALSE); |
There was a problem hiding this comment.
Please just add the final version instead of adding Exit() then changing to gSystem->Exit() in a later commit.
| if (-1 == pipe(gStackTraceHelper.fChildToParent)) | ||
| #else | ||
| if (-1 == pipe2(gStackTraceHelper.fChildToParent, O_CLOEXEC)) | ||
| #endif |
There was a problem hiding this comment.
Please just use pipe() and remove the #ifdef. This is unnecessary complexity.
https://www.usenix.org/legacy/publications/library/proceedings/sa92/spencer.pdf
| ROOT_GLOB_SOURCES(sources ${CMAKE_CURRENT_SOURCE_DIR}/src/*.cxx) | ||
|
|
||
| set(Unix_dict_headers ${CMAKE_CURRENT_SOURCE_DIR}/inc/T*.h PARENT_SCOPE) | ||
| ROOT_GLOB_HEADERS(headers ${CMAKE_CURRENT_SOURCE_DIR}/inc/T*.h) |
There was a problem hiding this comment.
ROOT_GLOB_HEADERS() is not allowed anymore. Please list all files. This is a pre-requisite to solve ROOT-5998.
|
@pcanal is this PR still relevant ? |
|
@pcanal Is there any update on signal handler on MacOS? Do I still need to work on this one? |
|
Converted to draft in order to be able to remove the |
|
Starting build on |
|
Build failed on mac1014/python3. Errors:
|
|
Build failed on ROOT-ubuntu16/nortcxxmod. Errors:
|
|
Build failed on ROOT-fedora31/noimt. Errors:
|
|
Build failed on ROOT-fedora30/cxx14. Errors:
|
|
Build failed on ROOT-performance-centos7-multicore/default. Errors:
|
|
Build failed on ROOT-debian10-i386/cxx14. Errors:
|
|
Build failed on mac1015/cxx17. Errors:
|
|
Build failed on windows10/cxx14. Errors:
|
ferdymercury
requested review from
silverweed
and removed request for
Axel-Naumann and
peremato
11 participants
This is an update of #134 to work compile on MacOS and rename TSigHandling into TSignalManager.
This is a work in progress as the new TSignalManager no longer calls TSystem::StackTrace but is also only implementing support for unix systems.
@zzxuanyuan Could you update the code to support at least MacOS (and attempt to support Windows)?