Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(975)

Issue 11195045: Pass the SIGPROF signal on to previously registered signal handler. (Closed)

Created:
8 years, 2 months ago by willchan no longer on Chromium
Modified:
7 years, 11 months ago
CC:
v8-dev, ojan, jln (very slow on Chromium)
Visibility:
Public.

Description

Pass the SIGPROF signal on to previously registered signal handler. This enables the google-perftools SIGPROF signal handler to continue to work properly. BUG=none Committed: https://code.google.com/p/v8/source/detail?r=12889

Patch Set 1 #

Total comments: 1

Patch Set 2 : Address comment. #

Patch Set 3 : Compiles now. #

Total comments: 7

Patch Set 4 : fixes as per markus #

Patch Set 5 : Handle SIG_DFL & SIG_IGN #

Total comments: 6

Patch Set 6 : Address latest comments. #

Patch Set 7 : Add newlines. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -56 lines) Patch
M src/platform-linux.cc View 1 2 3 4 5 6 6 chunks +97 lines, -56 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
willchan no longer on Chromium
This is my first time sending a patch to V8. I'm probably doing something silly, ...
8 years, 2 months ago (2012-10-18 18:28:08 UTC) #1
danno
https://codereview.chromium.org/11195045/diff/1/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/1/src/platform-linux.cc#newcode80 src/platform-linux.cc:80: static void (*g_old_signal_handler)(int, siginfo_t *, void *) = NULL; ...
8 years, 2 months ago (2012-10-19 07:43:16 UTC) #2
willchan no longer on Chromium
Sorry for the delay, I think I've address the comment now.
8 years, 1 month ago (2012-10-31 21:58:12 UTC) #3
danno
lgtm, I'll land this for you.
8 years, 1 month ago (2012-11-02 10:35:52 UTC) #4
danno
Hold on, although the patch applies cleanly, the call of SignalSender::CallOldSignalHandler isn't declared before it's ...
8 years, 1 month ago (2012-11-02 10:40:15 UTC) #5
willchan no longer on Chromium
OK, I think this should work for real this time.
8 years, 1 month ago (2012-11-02 21:25:18 UTC) #6
danno
lgtm, I'll land this
8 years, 1 month ago (2012-11-07 17:19:30 UTC) #7
Michael Starzinger
Two Webkit tests started crashing with this change on both (32 bit and 64 bit) ...
8 years, 1 month ago (2012-11-07 19:57:37 UTC) #8
willchan no longer on Chromium
https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1073 src/platform-linux.cc:1073: if (signal_handler_installed_ && old_signal_handler_.sa_sigaction) old_signal_handler_.sa_sigaction is 0x1 in the ...
8 years ago (2012-12-12 22:28:37 UTC) #9
Markus (顧孟勤)
https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1074 src/platform-linux.cc:1074: old_signal_handler_.sa_sigaction(signal, info, context); If you want to do this ...
8 years ago (2012-12-12 22:58:17 UTC) #10
danno
https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1073 src/platform-linux.cc:1073: if (signal_handler_installed_ && old_signal_handler_.sa_sigaction) Go ahead and re-open this ...
8 years ago (2012-12-13 00:00:19 UTC) #11
willchan no longer on Chromium
Reopened. I'll hopefully upload a patch later today. https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/9001/src/platform-linux.cc#newcode1073 src/platform-linux.cc:1073: if ...
8 years ago (2012-12-13 00:15:57 UTC) #12
willchan no longer on Chromium
OK Markus, I think this is ready for a look. It passes that webkit test ...
8 years ago (2012-12-13 02:44:12 UTC) #13
Markus (顧孟勤)
https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1036 src/platform-linux.cc:1036: static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context); Our ...
8 years ago (2012-12-13 08:49:45 UTC) #14
Sven Panne
https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1036 src/platform-linux.cc:1036: static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context); On ...
8 years ago (2012-12-13 09:34:54 UTC) #15
Markus (顧孟勤)
https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc File src/platform-linux.cc (right): https://codereview.chromium.org/11195045/diff/18002/src/platform-linux.cc#newcode1036 src/platform-linux.cc:1036: static void ProfilerSignalHandler(int signal, siginfo_t* info, void* context); Ah, ...
8 years ago (2012-12-13 09:56:50 UTC) #16
willchan no longer on Chromium
OK, I think this is ready for another look!
8 years ago (2012-12-13 23:48:40 UTC) #17
Markus (顧孟勤)
lgtm I like it. Thank you. And sorry for being so anal. I should probably ...
8 years ago (2012-12-14 00:44:15 UTC) #18
willchan no longer on Chromium
I don't know what we do for unit testing for this code in v8. I'm ...
8 years ago (2012-12-14 21:13:17 UTC) #19
danno
8 years ago (2012-12-20 17:31:02 UTC) #20
We have (a lot of) C++ based tests of the V8 API in src/test/cctest/.  You
should be able to add something src/test/cctest/test-platform-linux.cc that sets
up a signal handler, initializes V8 and verifies that the call through works
properly. It would be great if you added something like this before we land this
CL.

Powered by Google App Engine
This is Rietveld 408576698