Make --enable-heap-profiling=native "work" on Windows.

Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=686208
Review-Url: https://codereview.chromium.org/2707223002
Cr-Commit-Position: refs/heads/master@{#456771}
Committed: https://chromium.googlesource.com/chromium/src/+/93de86905a40d330fd5418a772b7a4ef83491215

[Wez, 2017-02-22 00:03:52]

Description was changed from

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

BUG=
==========

to

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=
==========

[Wez, 2017-02-22 00:03:52]

wez@chromium.org changed reviewers:
+ ajwong@chromium.org

[brettw, 2017-02-22 22:03:30]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-02-22 22:03:31]

https://codereview.chromium.org/2707223002/diff/1/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2707223002/diff/1/base/debug/stack_trace.h#ne...
base/debug/stack_trace.h:26: //#if defined(OS_POSIX) && ( \
We should clean this up before checking it in.

[Wez, 2017-02-23 00:12:22]

https://codereview.chromium.org/2707223002/diff/1/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2707223002/diff/1/base/debug/stack_trace.h#ne...
base/debug/stack_trace.h:26: //#if defined(OS_POSIX) && ( \
On 2017/02/22 22:03:31, brettw (plz ping after 24h) wrote:
> We should clean this up before checking it in.

Yup; this was uploaded mainly to unblock Albert - I'll update and upload a clean
version today.

[Wez, 2017-02-24 00:44:18]

awong: FYI; I'm an idiot who thinks pointers fit in a single byte. *ahem*.

[awong, 2017-03-01 23:42:20]

LGTM

[awong, 2017-03-01 23:43:26]

On 2017/03/01 23:42:20, awong wrote:
> LGTM

As a first pass for stack capture, this seems completely fine.

[Wez, 2017-03-01 23:52:34]

Would you prefer that we avoid setting "HAVE_STACK_FRAME_POINTERS" in
builds other than Release+Static? Otherwise we'll have
--enable-heap-profiling=native "work", but not actually capture anything
much.

WDYT?

On 1 March 2017 at 15:43, <ajwong@chromium.org> wrote:

> On 2017/03/01 23:42:20, awong wrote:
> > LGTM
>
> As a first pass for stack capture, this seems completely fine.
>
> https://codereview.chromium.org/2707223002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awong, 2017-03-02 00:00:08]

I actually think having it "work" for now makes sense. We should find a way to
fix it for other types of builds...

Also, FYI, this isn't clang-clean. There's some unused-function warnings.


On 2017/03/01 23:52:34, Wez wrote:
> Would you prefer that we avoid setting "HAVE_STACK_FRAME_POINTERS" in
> builds other than Release+Static? Otherwise we'll have
> --enable-heap-profiling=native "work", but not actually capture anything
> much.
> 
> WDYT?
> 
> On 1 March 2017 at 15:43, <mailto:ajwong@chromium.org> wrote:
> 
> > On 2017/03/01 23:42:20, awong wrote:
> > > LGTM
> >
> > As a first pass for stack capture, this seems completely fine.
> >
> > https://codereview.chromium.org/2707223002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Wez, 2017-03-02 00:01:17]

OK, I'll fix that up this afternoon and go ahead and land.

On 1 March 2017 at 16:00, <ajwong@chromium.org> wrote:

> I actually think having it "work" for now makes sense. We should find a
> way to
> fix it for other types of builds...
>
> Also, FYI, this isn't clang-clean. There's some unused-function warnings.
>
>
> On 2017/03/01 23:52:34, Wez wrote:
> > Would you prefer that we avoid setting "HAVE_STACK_FRAME_POINTERS" in
> > builds other than Release+Static? Otherwise we'll have
> > --enable-heap-profiling=native "work", but not actually capture anything
> > much.
> >
> > WDYT?
> >
> > On 1 March 2017 at 15:43, <mailto:ajwong@chromium.org> wrote:
> >
> > > On 2017/03/01 23:42:20, awong wrote:
> > > > LGTM
> > >
> > > As a first pass for stack capture, this seems completely fine.
> > >
> > > https://codereview.chromium.org/2707223002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2707223002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Wez, 2017-03-12 07:21:19]

Description was changed from

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=
==========

to

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=686208
==========

[Wez, 2017-03-12 07:22:30]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 07:22:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-12 08:36:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-12 08:36:13]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-03-13 01:35:13]

wez@chromium.org changed reviewers:
+ dcheng@chromium.org

[Wez, 2017-03-13 01:35:14]

dcheng: PTAL. This is a quick patch to un-block native heap profiling on
Windows; we plan to do some cleanup of StackTrace as noted in the TODOs.

[dcheng, 2017-03-13 02:11:21]

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:29: defined(OS_WIN)
Nit: this condition is getting pretty hard to read. Can we break it down to
simplify the conditions, even though that means the HAVE_STACK_FRAME_POINTERS
#define will be redundant?

e.g.

#if defined(OS_POSIX)

#if defined(__i386__) || defined(__x86_64__)
#define HAVE_TRACE_STACK_FRAME_POINTERS 1
#elif defined(__arm__) && !defined(__thumb)
#define HAVE_TRACE_STACK_FRAME_POINTERS 1
#else
#define HAVE_TRACE_STACK_FRAME_POINTERS 0
#endif  // defined(__i386__) || defined(__x86_64__)

#elif defined(OS_WIN)
#define HAVE_TRACE_STACK_FRAME_POINTERS 1
#else
#define HAVE_TRACE_STACK_FRAME_POINTERS 0
#endif  // defined(OS_POSIX)

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace_...
File base/debug/stack_trace_unittest.cc (right):

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_unittest.cc:258: // MSVC (Windows) doesn't generate frame
pointers and can't take the address
I'm a bit confused about the comments vs the GN changes: the GN changes mean we
generate frame pointers, but this comments says we don't...

[Wez, 2017-03-14 05:21:12]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 05:21:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-03-14 05:22:39]

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace.h
File base/debug/stack_trace.h (right):

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:29: defined(OS_WIN)
On 2017/03/13 02:11:21, dcheng wrote:
> Nit: this condition is getting pretty hard to read. Can we break it down to
> simplify the conditions, even though that means the HAVE_STACK_FRAME_POINTERS
> #define will be redundant?
> 
> e.g.
> 
> #if defined(OS_POSIX)
> 
> #if defined(__i386__) || defined(__x86_64__)
> #define HAVE_TRACE_STACK_FRAME_POINTERS 1
> #elif defined(__arm__) && !defined(__thumb)
> #define HAVE_TRACE_STACK_FRAME_POINTERS 1
> #else
> #define HAVE_TRACE_STACK_FRAME_POINTERS 0
> #endif  // defined(__i386__) || defined(__x86_64__)
> 
> #elif defined(OS_WIN)
> #define HAVE_TRACE_STACK_FRAME_POINTERS 1
> #else
> #define HAVE_TRACE_STACK_FRAME_POINTERS 0
> #endif  // defined(OS_POSIX)

Done, although I'm keeping the TODO because I would actually prefer to make
having frame pointers a POSIX-only flag, since we'll never use them under
Windows anyway.

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace_...
File base/debug/stack_trace_unittest.cc (right):

https://codereview.chromium.org/2707223002/diff/60001/base/debug/stack_trace_...
base/debug/stack_trace_unittest.cc:258: // MSVC (Windows) doesn't generate frame
pointers and can't take the address
On 2017/03/13 02:11:21, dcheng wrote:
> I'm a bit confused about the comments vs the GN changes: the GN changes mean
we
> generate frame pointers, but this comments says we don't...

Originally we planned to use frame pointers; this comment is correct for Win64
builds, which don't support frame pointers at all - updated it to correctly
specify why we're skipping these tests.

[commit-bot: I haz the power, 2017-03-14 07:02:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 07:02:19]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-03-14 07:22:42]

LGTM

https://codereview.chromium.org/2707223002/diff/80001/base/debug/stack_trace_...
File base/debug/stack_trace_unittest.cc (right):

https://codereview.chromium.org/2707223002/diff/80001/base/debug/stack_trace_...
base/debug/stack_trace_unittest.cc:258: // Windows x64 binaries cannot be build
with frame pointer, and MSVC doesn't
Nit: build->built

[Wez, 2017-03-14 16:17:32]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-03-14 16:17:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ajwong@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2707223002/#ps100001
(title: "Fix typos")

[commit-bot: I haz the power, 2017-03-14 16:17:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-03-14 18:11:31]

wez@chromium.org changed reviewers:
+ etienneb@chromium.org

[Wez, 2017-03-14 18:11:32]

+etienneb

[commit-bot: I haz the power, 2017-03-14 18:44:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489508252820370,
"parent_rev": "42c4d3b900752d93f6da6fb4f3dac6d367ac5727", "commit_rev":
"93de86905a40d330fd5418a772b7a4ef83491215"}

[commit-bot: I haz the power, 2017-03-14 18:45:01]

Description was changed from

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=686208
==========

to

==========
Make --enable-heap-profiling=native "work" on Windows.

Three small changes to get things "working"[1]:
1. Implement TraceStackFramePointers using StackTrace, under OS_WIN.
2. Add compile flag to enable frame pointers.
3. Tweak the HAVE_TRACE_STACK_FRAME_POINTERS definition.

This enables native heap profiling in Release+Static builds.

[1] Not perfectly, but enough to un-block related work.

BUG=686208

Review-Url: https://codereview.chromium.org/2707223002
Cr-Commit-Position: refs/heads/master@{#456771}
Committed:
https://chromium.googlesource.com/chromium/src/+/93de86905a40d330fd5418a772b7...
==========

[commit-bot: I haz the power, 2017-03-14 18:45:02]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/93de86905a40d330fd5418a772b7...

[DmitrySkiba, 2017-03-14 19:36:13]

On 2017/03/14 18:45:02, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:100001) as
>
https://chromium.googlesource.com/chromium/src/+/93de86905a40d330fd5418a772b7...

TraceStackFramePointers() is supposed to use frame pointers for unwinding (hence
the name). I don't think it's right to break that contract.

The better way is to change AllocationContextTracker::GetContextSnapshot() to
use StackTrace::Addresses when frame pointers are not available
(HAVE_TRACE_STACK_FRAME_POINTERS is not defined).

[Wez, 2017-03-14 19:41:51]

Yes, I agree; we weren't super happy about this either. I'll follow-up with
a cleanup, later this week.

On 14 March 2017 at 12:36, <dskiba@chromium.org> wrote:

> On 2017/03/14 18:45:02, commit-bot: I haz the power wrote:
> > Committed patchset #6 (id:100001) as
> >
> https://chromium.googlesource.com/chromium/src/+/
> 93de86905a40d330fd5418a772b7a4ef83491215
>
> TraceStackFramePointers() is supposed to use frame pointers for unwinding
> (hence
> the name). I don't think it's right to break that contract.
>
> The better way is to change AllocationContextTracker::GetContextSnapshot()
> to
> use StackTrace::Addresses when frame pointers are not available
> (HAVE_TRACE_STACK_FRAME_POINTERS is not defined).
>
> https://codereview.chromium.org/2707223002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.