Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 3 months ago by JaideepBajwa Modified:
4 years, 3 months ago Reviewers:
rmcilroy CC:
v8-reviews_googlegroups.com, JoranSiu, john.yan, michael_dawson, MTBrandyberry Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionAIX: Disable backtrace API call
Backtrace() and backtrace_symbols() API are not supported on
AIX and there are no user-mode equivalent API for the same.
For now, disabling the call to those API.
Currently this is preventing V8 to build on AIX.
This debug functionality was added in
https://codereview.chromium.org/2248393002
R=rmcilroy@chromium.org
BUG=
LOG=N
Committed: https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29
Cr-Commit-Position: refs/heads/master@{#39356}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Created: 4 years, 3 months ago
Messages
Total messages: 15 (2 generated)
PTAL
https://codereview.chromium.org/2324453002/diff/1/src/base/debug/stack_trace_... File src/base/debug/stack_trace_posix.cc (right): https://codereview.chromium.org/2324453002/diff/1/src/base/debug/stack_trace_... src/base/debug/stack_trace_posix.cc:149: // equivalent user-mode API. For now disabling the call to the API No need for the comment (there are probably other platforms in a similar situation and this comment will get stale when #ifdefing out other platforms). https://codereview.chromium.org/2324453002/diff/1/src/base/debug/stack_trace_... src/base/debug/stack_trace_posix.cc:150: #if !V8_OS_AIX Could you instead ifdef out the whole of ProcessBacktrace and the calls to it (similarly to how __ulibc__ is treated in the chromium version of the file: https://cs.chromium.org/chromium/src/base/debug/stack_trace_posix.cc?type=cs&... https://codereview.chromium.org/2324453002/diff/1/src/base/debug/stack_trace_... src/base/debug/stack_trace_posix.cc:364: // equivalent user-mode API. For now disabling the call to the API No need for the comment https://codereview.chromium.org/2324453002/diff/1/src/base/debug/stack_trace_... src/base/debug/stack_trace_posix.cc:365: #if V8_OS_AIX Could you make this #if !V8_OS_AIX and swap the branches around.
PTAL. I've updated as suggested. Thank you.
LGTM, thanks.
The CQ bit was checked by bjaideep@ca.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== AIX: Disable backtrace API call Backtrace() and backtrace_symbols() API are not supported on AIX and there are no user-mode equivalent API for the same. For now, disabling the call to those API. Currently this is preventing V8 to build on AIX. This debug functionality was added in https://codereview.chromium.org/2248393002 R=rmcilroy@chromium.org BUG= LOG=N ========== to ========== AIX: Disable backtrace API call Backtrace() and backtrace_symbols() API are not supported on AIX and there are no user-mode equivalent API for the same. For now, disabling the call to those API. Currently this is preventing V8 to build on AIX. This debug functionality was added in https://codereview.chromium.org/2248393002 R=rmcilroy@chromium.org BUG= LOG=N Committed: https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 Cr-Commit-Position: refs/heads/master@{#39356} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 Cr-Commit-Position: refs/heads/master@{#39356}
Message was sent while issue was closed.
On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > Cr-Commit-Position: refs/heads/master@{#39356} Can this be merged back to V8 5.4?
Message was sent while issue was closed.
On 2016/09/12 19:07:48, ofrobots wrote: > On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > > Cr-Commit-Position: refs/heads/master@{#39356} > > Can this be merged back to V8 5.4? I've opened a CL to merge this to 5.4. https://codereview.chromium.org/2333773002/
Message was sent while issue was closed.
On 2016/09/12 19:07:48, ofrobots wrote: > On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > > Cr-Commit-Position: refs/heads/master@{#39356} > > Can this be merged back to V8 5.4? Actually I will be following up with a patch that fixes the other platforms that are also broken (I'm taking over the CL https://codereview.chromium.org/2321433003/) . I can take care of the merge back of this together with that.
Message was sent while issue was closed.
On 2016/09/12 20:31:24, ofrobots wrote: > On 2016/09/12 19:07:48, ofrobots wrote: > > On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > > > Patchset 2 (id:??) landed as > > > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > > > Cr-Commit-Position: refs/heads/master@{#39356} > > > > Can this be merged back to V8 5.4? > > Actually I will be following up with a patch that fixes the other platforms that > are also broken (I'm taking over the CL > https://codereview.chromium.org/2321433003/) . I can take care of the merge back > of this together with that. Oh I see that you already opened a bug for the merge. That's fine too.
Message was sent while issue was closed.
On 2016/09/12 20:32:29, ofrobots wrote: > On 2016/09/12 20:31:24, ofrobots wrote: > > On 2016/09/12 19:07:48, ofrobots wrote: > > > On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > > > > Patchset 2 (id:??) landed as > > > > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > > > > Cr-Commit-Position: refs/heads/master@{#39356} > > > > > > Can this be merged back to V8 5.4? > > > > Actually I will be following up with a patch that fixes the other platforms > that > > are also broken (I'm taking over the CL > > https://codereview.chromium.org/2321433003/) . I can take care of the merge > back > > of this together with that. > > Oh I see that you already opened a bug for the merge. That's fine too. Ok, actually the merge of this CL is not committed yet. I think it would be better to close that merge CL with a ref to https://codereview.chromium.org/2321433003/ (that way the history of 5.4 will be cleaner).
Message was sent while issue was closed.
On 2016/09/12 21:18:39, JaideepBajwa wrote: > On 2016/09/12 20:32:29, ofrobots wrote: > > On 2016/09/12 20:31:24, ofrobots wrote: > > > On 2016/09/12 19:07:48, ofrobots wrote: > > > > On 2016/09/12 15:32:25, commit-bot: I haz the power wrote: > > > > > Patchset 2 (id:??) landed as > > > > > https://crrev.com/bc752a0977bf823765aa298fe64670df91e79a29 > > > > > Cr-Commit-Position: refs/heads/master@{#39356} > > > > > > > > Can this be merged back to V8 5.4? > > > > > > Actually I will be following up with a patch that fixes the other platforms > > that > > > are also broken (I'm taking over the CL > > > https://codereview.chromium.org/2321433003/) . I can take care of the merge > > back > > > of this together with that. > > > > Oh I see that you already opened a bug for the merge. That's fine too. > > Ok, actually the merge of this CL is not committed yet. I think it would be > better to close that merge CL with a ref to > https://codereview.chromium.org/2321433003/ (that way the history of 5.4 will be > cleaner). It turns out that we also need to backport https://codereview.chromium.org/2292973002. It is a mess :(. Feel free to close your merge CL. I can do all of these in one shot. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
