Remove WTFReportAssertionFailure and WTFReportBacktrace.

Remove WTFReportAssertionFailure and WTFReportBacktrace.

WTF assertion macros are now alised to their base/ counterparts, so
these functions are no longer used.

BUG=596760
Review-Url: https://codereview.chromium.org/2632643004
Cr-Commit-Position: refs/heads/master@{#446937}
Committed: https://chromium.googlesource.com/chromium/src/+/73f8d7ec73f5ff5f7fd7a24862aef82f73f103e7

[Wez, 2017-01-13 21:21:54]

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

[commit-bot: I haz the power, 2017-01-13 21:22:23]

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

[Wez, 2017-01-13 21:22:24]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2017-01-13 21:22:33]

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

[Wez, 2017-01-13 21:22:33]

PTAL

[Wez, 2017-01-13 21:22:38]

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

[commit-bot: I haz the power, 2017-01-13 21:23:15]

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

[esprehn, 2017-01-13 22:06:28]

I think the format that WTF used was a much prettier unmangled trace IIRC? Can
we port some of this logic into base? Also note that WTFReportBacktrace lets you
limit the number of frames to print while base::debug::StackTrace() doesn't make
that easy in a one liner when debugging you need to write a bunch of code. Can
we fix that?

[commit-bot: I haz the power, 2017-01-13 22:59:32]

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

[commit-bot: I haz the power, 2017-01-13 22:59:33]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-01-14 00:14:28]

When I tried calling ASSERT to crash some unit-tests I found that [under
Linux Debug Component build] the WTFReportBacktrace() function was broken,
such that all you get is the addresses, with no function names. The Chrome
backtrace printer gave names.

I'll re-test that, and try under Windows as well, to see if I can repro
anything prettier, though.  Will also check whether it's straightforward to
limit the number of lines in base/ stack traces (seems it should be :).

On 13 January 2017 at 14:06, <esprehn@chromium.org> wrote:

> I think the format that WTF used was a much prettier unmangled trace IIRC?
> Can
> we port some of this logic into base? Also note that WTFReportBacktrace
> lets you
> limit the number of frames to print while base::debug::StackTrace()
> doesn't make
> that easy in a one liner when debugging you need to write a bunch of code.
> Can
> we fix that?
>
> https://codereview.chromium.org/2632643004/
>

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

[Wez, 2017-01-14 00:14:28]

When I tried calling ASSERT to crash some unit-tests I found that [under
Linux Debug Component build] the WTFReportBacktrace() function was broken,
such that all you get is the addresses, with no function names. The Chrome
backtrace printer gave names.

I'll re-test that, and try under Windows as well, to see if I can repro
anything prettier, though.  Will also check whether it's straightforward to
limit the number of lines in base/ stack traces (seems it should be :).

On 13 January 2017 at 14:06, <esprehn@chromium.org> wrote:

> I think the format that WTF used was a much prettier unmangled trace IIRC?
> Can
> we port some of this logic into base? Also note that WTFReportBacktrace
> lets you
> limit the number of frames to print while base::debug::StackTrace()
> doesn't make
> that easy in a one liner when debugging you need to write a bunch of code.
> Can
> we fix that?
>
> https://codereview.chromium.org/2632643004/
>

-- 
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-01-14 03:03:05]

On 2017/01/14 00:14:28, Wez wrote:
> When I tried calling ASSERT to crash some unit-tests I found that [under
> Linux Debug Component build] the WTFReportBacktrace() function was broken,
> such that all you get is the addresses, with no function names. The Chrome
> backtrace printer gave names.

tl;dr: I can't repro even function-name printing from WTFReportBacktrace, let
alone anything pretty. :P

Adding a couple of lines of backtrace output to the WTF Assertions test I added,
I get this:

[19604:19604:0113/184259.145434:628687472469:ERROR:AssertionsTest.cpp(16)] WTF:
1   0x64788e
2   0x6482a0
[19604:19604:0113/184259.145503:628687472535:ERROR:AssertionsTest.cpp(19)] BASE:
#0 0x0000005d71ae base::debug::StackTrace::StackTrace()
#1 0x000000431545 WTF::AssertionsTest_Assertions_Test::TestBody()

I get this in both Debug and Release builds.

Looking at the FrameToNameScope function, which attempts to obtain a name based
on the address, it seems that that is only implemented for Mac OS X and Linux,
and not for Windows.  I'm not sure why it doesn't find any function names,
though.  Could it be confused by address randomization or something?

Is there some backtrace post-processing script which you're thinking off which
produced "pretty" output?

> I'll re-test that, and try under Windows as well, to see if I can repro
> anything prettier, though.  Will also check whether it's straightforward to
> limit the number of lines in base/ stack traces (seems it should be :).

OK, this was indeed straightforward, so done. :)

[Wez, 2017-01-14 03:03:15]

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

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

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

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

FWIW Chromium base/ uses third_party/symbolize to symbolize & unmangle names,
while WTFReportBacktrace, and Chromium built without USE_SYMBOLIZE, uses
abi::__cxa_demangle().  I tried building without symbolize and encountered the
same problem of getting no names at all.

Perhaps the ABI-supplied demangler used to do a better job than
thid_party/symbolize, even though it appears now to be broken?

[commit-bot: I haz the power, 2017-01-14 05:04:05]

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

[commit-bot: I haz the power, 2017-01-14 05:04:05]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-01-20 20:11:48]

tkent: Ping

esprehn: As noted above, I'm unable to repro any pretty traceback functionality
from Blink - if you are able to repro then please give me an example, so I can
take a look.

[tkent, 2017-01-23 00:08:42]

On 2017/01/20 at 20:11:48, wez wrote:
> esprehn: As noted above, I'm unable to repro any pretty traceback
functionality from Blink - if you are able to repro then please give me an
example, so I can take a look.

Probably the issue is specific to Mac?  crbug.com/598886.

[Wez, 2017-01-26 22:31:40]

OK, as of https://codereview.chromium.org/2651893002/ we have symbolizing &
demangling working again on Mac OS X.

PTAL :)

[esprehn, 2017-01-26 22:55:44]

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

[esprehn, 2017-01-26 22:55:45]

lgtm

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

https://codereview.chromium.org/2632643004/diff/20001/base/debug/stack_trace....
base/debug/stack_trace.h:72: StackTrace Truncate(size_t max_entries) const;
This does mean we waste time inside backtrace() filling the array with entries
you don't want if you just print 3 lines, but maybe that's okay.

[tkent, 2017-01-27 00:37:23]

lgtm

[Wez, 2017-01-27 01:29:59]

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

https://codereview.chromium.org/2632643004/diff/20001/base/debug/stack_trace....
base/debug/stack_trace.h:72: StackTrace Truncate(size_t max_entries) const;
On 2017/01/26 22:55:45, esprehn wrote:
> This does mean we waste time inside backtrace() filling the array with entries
> you don't want if you just print 3 lines, but maybe that's okay.

Fair point; let's optimize that if it proves to be valuable?

[Wez, 2017-01-27 01:30:04]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-01-27 01:31:11]

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-01-27 01:45:07]

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

[commit-bot: I haz the power, 2017-01-27 01:45:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Wez, 2017-01-27 01:48:14]

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

[Wez, 2017-01-27 01:48:15]

+dcheng for base/OWNERS for StackTrace changes.

[dcheng, 2017-01-27 09:15:24]

It seems like Truncate() is only used by the test in //base? Am I missing
something?

[esprehn, 2017-01-27 09:21:57]

On 2017/01/27 at 09:15:24, dcheng wrote:
> It seems like Truncate() is only used by the test in //base? Am I missing
something?

It's for debugging locally when you want to instrument the code and print
stacks.

[dcheng, 2017-01-27 19:09:00]

Hmm. What do you think about just adding a size_t max_entries argument to
Print() / ToString()?

[esprehn, 2017-01-27 19:32:49]

On 2017/01/27 at 19:09:00, dcheng wrote:
> Hmm. What do you think about just adding a size_t max_entries argument to
Print() / ToString()?

That's fine for me too. WTF had a BACKTRACE() macro that let you print N entries
in the stack which was super helpful when debugging. As long as we can have a
similarly short debugging one liner in base I'm happy. :)

base::debug::StackTrace().Truncate(3).Print() is already kind of long, if we
could add something like a BACKTRACE() macro that'd be even better.

[Wez, 2017-01-27 19:40:05]

I'm fine with adding a parameter to Print() and/or ToString().

As Elliott previously mentioned, we could instead make it a parameter to
StackTrace() itself, so that it can call the backtrace capture command with
a smaller buffer, which would be a minor optimization.

I have a mild preference for the latter approach.

On 27 January 2017 at 11:32, <esprehn@chromium.org> wrote:

> On 2017/01/27 at 19:09:00, dcheng wrote:
> > Hmm. What do you think about just adding a size_t max_entries argument to
> Print() / ToString()?
>
> That's fine for me too. WTF had a BACKTRACE() macro that let you print N
> entries
> in the stack which was super helpful when debugging. As long as we can
> have a
> similarly short debugging one liner in base I'm happy. :)
>
> base::debug::StackTrace().Truncate(3).Print() is already kind of long, if
> we
> could add something like a BACKTRACE() macro that'd be even better.
>
> https://codereview.chromium.org/2632643004/
>

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

[Wez, 2017-01-27 19:40:05]

I'm fine with adding a parameter to Print() and/or ToString().

As Elliott previously mentioned, we could instead make it a parameter to
StackTrace() itself, so that it can call the backtrace capture command with
a smaller buffer, which would be a minor optimization.

I have a mild preference for the latter approach.

On 27 January 2017 at 11:32, <esprehn@chromium.org> wrote:

> On 2017/01/27 at 19:09:00, dcheng wrote:
> > Hmm. What do you think about just adding a size_t max_entries argument to
> Print() / ToString()?
>
> That's fine for me too. WTF had a BACKTRACE() macro that let you print N
> entries
> in the stack which was super helpful when debugging. As long as we can
> have a
> similarly short debugging one liner in base I'm happy. :)
>
> base::debug::StackTrace().Truncate(3).Print() is already kind of long, if
> we
> could add something like a BACKTRACE() macro that'd be even better.
>
> https://codereview.chromium.org/2632643004/
>

-- 
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-01-27 21:07:26]

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

[commit-bot: I haz the power, 2017-01-27 21:07:59]

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

[Wez, 2017-01-27 21:49:19]

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

[commit-bot: I haz the power, 2017-01-27 21:49:45]

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

[Wez, 2017-01-27 21:51:31]

OK, I've added a |count| parameter to StackTrace().

[esprehn, 2017-01-27 23:16:14]

lgtm

[dcheng, 2017-01-28 01:00:34]

lgtm

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

https://codereview.chromium.org/2632643004/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:57: // |count| will be trimmed to |kMaxTraces|.
Nit: to me, trimmed isn't an operation that applies to integers. I suggest
limited or clamped.

https://codereview.chromium.org/2632643004/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:95: protected:
Why protected?

[commit-bot: I haz the power, 2017-01-28 02:25:30]

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

[commit-bot: I haz the power, 2017-01-28 02:25:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Wez, 2017-01-29 04:25:01]

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

https://codereview.chromium.org/2632643004/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:57: // |count| will be trimmed to |kMaxTraces|.
On 2017/01/28 01:00:33, dcheng wrote:
> Nit: to me, trimmed isn't an operation that applies to integers. I suggest
> limited or clamped.

Agreed; I just copied the wording from the constructor below, for consistency.
Have updated the wording to "limited at most to".

https://codereview.chromium.org/2632643004/diff/60001/base/debug/stack_trace....
base/debug/stack_trace.h:95: protected:
On 2017/01/28 01:00:33, dcheng wrote:
> Why protected?

Oh, strange; no idea where that came from :-/

[Wez, 2017-01-29 04:25:05]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-01-29 04:25:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
esprehn@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2632643004/#ps80001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-01-29 04:25:10]

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-01-29 06:18:17]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485663905434620,
"parent_rev": "f766db29b990d87a3933a27079c2cf14f09aa4b7", "commit_rev":
"73f8d7ec73f5ff5f7fd7a24862aef82f73f103e7"}

[commit-bot: I haz the power, 2017-01-29 06:19:36]

Description was changed from

==========
Remove WTFReportAssertionFailure and WTFReportBacktrace.

WTF assertion macros are now alised to their base/ counterparts, so
these functions are no longer used.

BUG=596760
==========

to

==========
Remove WTFReportAssertionFailure and WTFReportBacktrace.

WTF assertion macros are now alised to their base/ counterparts, so
these functions are no longer used.

BUG=596760

Review-Url: https://codereview.chromium.org/2632643004
Cr-Commit-Position: refs/heads/master@{#446937}
Committed:
https://chromium.googlesource.com/chromium/src/+/73f8d7ec73f5ff5f7fd7a24862ae...
==========

[commit-bot: I haz the power, 2017-01-29 06:19:37]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/73f8d7ec73f5ff5f7fd7a24862ae...