Chromium Code Reviews
Help | Chromium Project | Sign in
(29)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by willchan
Modified:
1 year, 3 months ago
CC:
v8-dev_googlegroups.com, ojan, jln
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) Lint Patch
M src/platform-linux.cc View 1 2 3 4 5 6 6 chunks +97 lines, -56 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 20
willchan
This is my first time sending a patch to V8. I'm probably doing something silly, ...
1 year, 6 months ago #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; ...
1 year, 6 months ago #2
willchan
Sorry for the delay, I think I've address the comment now.
1 year, 5 months ago #3
danno
lgtm, I'll land this for you.
1 year, 5 months ago #4
danno
Hold on, although the patch applies cleanly, the call of SignalSender::CallOldSignalHandler isn't declared before it's ...
1 year, 5 months ago #5
willchan
OK, I think this should work for real this time.
1 year, 5 months ago #6
danno
lgtm, I'll land this
1 year, 5 months ago #7
Michael Starzinger
Two Webkit tests started crashing with this change on both (32 bit and 64 bit) ...
1 year, 5 months ago #8
willchan
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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #11
willchan
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 ...
1 year, 4 months ago #12
willchan
OK Markus, I think this is ready for a look. It passes that webkit test ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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, ...
1 year, 4 months ago #16
willchan
OK, I think this is ready for another look!
1 year, 4 months ago #17
Markus (顧孟勤)
lgtm I like it. Thank you. And sorry for being so anal. I should probably ...
1 year, 4 months ago #18
willchan
I don't know what we do for unit testing for this code in v8. I'm ...
1 year, 4 months ago #19
danno
1 year, 4 months ago #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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6