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

Issue 11362048: GTTF: Make Linux stack dump signal handler async-signal safe. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by Paweł Hajdan Jr.
Modified:
1 year, 5 months ago
CC:
chromium-reviews_chromium.org, erikwright+watch_chromium.org, Markus (顧孟勤)
Visibility:
Public.

Description

GTTF: Make Linux stack dump signal handler async-signal safe.

This also re-enables in-process stack dumping on Linux.

BUG=101155
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167714

Patch Set 1 #

Total comments: 13

Patch Set 2 : fixes #

Total comments: 2

Patch Set 3 : fixes #

Total comments: 12

Patch Set 4 : fixes #

Patch Set 5 : fixes #

Total comments: 7

Patch Set 6 : fixes #

Patch Set 7 : trybot debugging ios & android #

Total comments: 2

Patch Set 8 : fixes #

Patch Set 9 : hopefully the ultimate android fix #

Total comments: 4

Patch Set 10 : shess' comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+336 lines, -66 lines) Lint Patch
M base/base.gyp View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments ? errors Download
M base/debug/debugger_posix.cc View 1 3 chunks +15 lines, -2 lines 0 comments 0 errors Download
M base/debug/stack_trace.h View 1 2 3 4 5 7 1 chunk +14 lines, -0 lines 0 comments 0 errors Download
M base/debug/stack_trace_posix.cc View 1 2 3 4 5 6 7 8 9 8 chunks +208 lines, -60 lines 0 comments 5 errors Download
M base/debug/stack_trace_unittest.cc View 1 2 3 4 5 6 7 5 chunks +95 lines, -4 lines 0 comments 1 errors Download
Commit:

Messages

Total messages: 26
Paweł Hajdan Jr.
Please review: Jim: entire CL Julien: is there some way for me to share code ...
1 year, 5 months ago #1
jln (OOO-til-April-22)
On 2012/11/01 22:54:46, Paweł Hajdan Jr. wrote: > Please review: > > Jim: entire CL ...
1 year, 5 months ago #2
jar
I didn't have time to go though the rest of the code, and stopped after ...
1 year, 5 months ago #3
Paweł Hajdan Jr.
On 2012/11/02 01:17:48, Julien Tinnes wrote: > Excellent to see this! sandbox/linux/sandbox-bpf can use base, ...
1 year, 5 months ago #4
jln (OOO-til-April-22)
On 2012/11/06 17:58:45, Paweł Hajdan Jr. wrote: > On 2012/11/02 01:17:48, Julien Tinnes wrote: > ...
1 year, 5 months ago #5
jar
Much better... and I just realized something else to make it much much simpler. You ...
1 year, 5 months ago #6
Paweł Hajdan Jr.
PTAL
1 year, 5 months ago #7
jar
https://codereview.chromium.org/11362048/diff/5/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/11362048/diff/5/base/debug/stack_trace_posix.cc#newcode256 base/debug/stack_trace_posix.cc:256: if (i < 0) { This should only be ...
1 year, 5 months ago #8
Paweł Hajdan Jr.
https://codereview.chromium.org/11362048/diff/5/base/debug/stack_trace_unittest.cc File base/debug/stack_trace_unittest.cc (right): https://codereview.chromium.org/11362048/diff/5/base/debug/stack_trace_unittest.cc#newcode161 base/debug/stack_trace_unittest.cc:161: EXPECT_EQ("-9223372036854775808", On 2012/11/07 00:32:07, jar wrote: > On a ...
1 year, 5 months ago #9
shess
Sorry about the late and nit-picky comments. This feels kind of excessive, like maybe rather ...
1 year, 5 months ago #10
jar
Windows is only built in 32 bit mode, and hence pointers will only be 32 ...
1 year, 5 months ago #11
jar
FYI: Linux is built and shipped both as a 32 and 64 bit binary. I'd ...
1 year, 5 months ago #12
Paweł Hajdan Jr.
PTAL I'm trying to keep itoa_r general, so if it needs to be re-used later ...
1 year, 5 months ago #13
shess
https://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc#newcode64 base/debug/stack_trace_posix.cc:64: #endif // defined(USE_SYMBOLIZE) I might be missing something, but ...
1 year, 5 months ago #14
Paweł Hajdan Jr.
http://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): http://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc#newcode64 base/debug/stack_trace_posix.cc:64: #endif // defined(USE_SYMBOLIZE) On 2012/11/07 17:58:07, shess wrote: > ...
1 year, 5 months ago #15
shess
https://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/11362048/diff/1006/base/debug/stack_trace_posix.cc#newcode64 base/debug/stack_trace_posix.cc:64: #endif // defined(USE_SYMBOLIZE) On 2012/11/07 19:45:11, Paweł Hajdan Jr. ...
1 year, 5 months ago #16
Paweł Hajdan Jr.
On 2012/11/07 20:29:26, shess wrote: > This is general-purpose code which the signal handler happens ...
1 year, 5 months ago #17
shess
On 2012/11/07 21:13:20, Paweł Hajdan Jr. wrote: > On 2012/11/07 20:29:26, shess wrote: > > ...
1 year, 5 months ago #18
jar
Commenting only on the itoa (which looks much much better now), and leaving it to ...
1 year, 5 months ago #19
Paweł Hajdan Jr.
PTAL This restores some of the older logic for demangling. http://codereview.chromium.org/11362048/diff/24001/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): http://codereview.chromium.org/11362048/diff/24001/base/debug/stack_trace_posix.cc#newcode249 ...
1 year, 5 months ago #20
Paweł Hajdan Jr.
On 2012/11/07 21:48:24, shess wrote: > As noted on the bug, this is in a ...
1 year, 5 months ago #21
jar
itoa looks nice, safe, and clean. The tests for the function also look great... so ...
1 year, 5 months ago #22
shess
I still don't feel convinced that the problem you are solving is an actual problem ...
1 year, 5 months ago #23
Paweł Hajdan Jr.
Thank you for the review! https://codereview.chromium.org/11362048/diff/10004/base/debug/stack_trace_posix.cc File base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/11362048/diff/10004/base/debug/stack_trace_posix.cc#newcode185 base/debug/stack_trace_posix.cc:185: strncat(buf, "Received signal ", ...
1 year, 5 months ago #24
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/phajdan.jr@chromium.org/11362048/13013
1 year, 5 months ago #25
I haz the power (commit-bot)
1 year, 5 months ago #26
Change committed as 167714
Sign in to reply to this message.

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