Issue 2869463002: Better stack trace support

Issue 2869463002: Better stack trace support (Closed)

Created:
3 years, 7 months ago by vsm

Modified:
3 years, 7 months ago

Reviewers:
Jennifer Messerly, Jacob

CC:
dev-compiler+reviews_dartlang.org

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Better stack trace support - Caches stack trace objects (package:stack_trace appears to rely on this) - Better support for stack traces on primitives R=jmesserly@google.com Committed: https://github.com/dart-lang/sdk/commit/36356c28c103048337e1b63c7ffd1f5459c2d1e5

Patch Set 2 : some more fixes #

Total comments: 20

Patch Set 3 : Address comments #

Created: 3 years, 7 months ago

(Patch set is too large to download)

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+77 lines, -36 lines)			Patch
M	pkg/dev_compiler/lib/sdk/ddc_sdk.sum	View	1 2	Binary file		0 comments	Download
M	pkg/dev_compiler/test/browser/language_tests.js	View	1 2	2 chunks	+0 lines, -4 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart	View	1 2	1 chunk	+59 lines, -26 lines	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/utils.dart	View	1 2	1 chunk	+1 line, -1 line	0 comments	Download
M	pkg/dev_compiler/tool/input_sdk/private/js_helper.dart	View	1 2	2 chunks	+12 lines, -5 lines	0 comments	Download

Messages

Total messages: 11 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Jennifer Messerly

Functionality wise I think this looks great! various comments related to DDC SDK impl code ...

3 years, 7 months ago (2017-05-05 23:59:16 UTC) #6

vsm

PTAL https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right): https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart#newcode91 pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:91: throwCyclicInitializationError([message]) => JS( On 2017/05/05 23:59:15, Jennifer Messerly ...

3 years, 7 months ago (2017-05-08 17:07:45 UTC) #7

PTAL

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right):

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:91:
throwCyclicInitializationError([message]) => JS(
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> I know this is the style of all these helpers but ... fyi ... I think this
could
> be written simply as:
> 
> 
> throwCyclicInitializationError([String message]) {
>   if (_trapRuntimeErrors) JS('', 'debugger');
>   throw new CyclicInitializationError(message);
> }
> 
> 
> ... only thing to watch out for in these is to avoid implicit casts for the
most
> part.

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
(right):

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:620: //
Store a JS error for an exception.  For non-primitives, we store as an expando.
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> should be triple slash doc comment?
> 
> also long line

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:624: Map
_primitiveErrorCache = null;
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> `= null` should not be required.
> 
> also since this is top-level, you can initialize it here, you'll get laziness
> for free from Dart.

Removed the null.  We're generating eagerly though if I initialize here &
failing at runtime.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:625: var
_maxErrorCache = 10;
On 2017/05/05 23:59:16, Jennifer Messerly wrote:
> this is a const right? (or at least a final)

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:627:
_isJsError(exception) {
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> bool return type?

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:676:
stackPrint(exception) => JS(
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> this isn't new code, but it's another place I'd try and refactor the "JS" call
> 
> stackPrint(exception) {
>   var error = recordJsError(exception);
>   JS('', 'console.log(#.stack ? #.stack : 'No stack trace for: ' + #)', error,
> error, error);
> }

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:683:
stackTrace(exception) => getTraceFromException(exception);
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> hmmm. I see why you had to do this (you need to create _StackTrace from
> js_helepr) ... but it looks odd as the reader.
> 
> Maybe worth a comment?

Done.

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
File pkg/dev_compiler/tool/input_sdk/private/js_helper.dart (right):

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:496: return
JS('StackTrace', 'dart.stackTrace(#)', error);
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> question ... i've seen this pattern too in dart:mirrors ... can we not just:
> 
> import 'dart:_runtime' as dart;
> 
>     return dart.stackTrace(error);
> 
> ... as long as we're importing 'dart:_runtime' from our own private SDK code
> (such as this file, or our mirrors patch) it should be fine to do that.
> 
> Bonus: better static checking of our SDK implementations :D
> 
> ... last but not least I think `dart.stackTrace` is equivalent to
> getTraceFromException, below

Oh nice - the typed call works!  

In this case though, calling getTraceFromException direct is the right call.

We should switch to typed calls though - we may also need better static typing
to avoid additional casts though (we're avoiding some since we trust JS).

https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:643: //=>
JS('StackTrace', 'dart.stackTrace(#)', exception);
On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> remove this comment?

doh!

vsm

On 2017/05/08 17:07:45, vsm wrote: > PTAL > > https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart > File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right): > ...

3 years, 7 months ago (2017-05-09 17:55:17 UTC) #8

On 2017/05/08 17:07:45, vsm wrote:
> PTAL
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart (right):
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/errors.dart:91:
> throwCyclicInitializationError([message]) => JS(
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > I know this is the style of all these helpers but ... fyi ... I think this
> could
> > be written simply as:
> > 
> > 
> > throwCyclicInitializationError([String message]) {
> >   if (_trapRuntimeErrors) JS('', 'debugger');
> >   throw new CyclicInitializationError(message);
> > }
> > 
> > 
> > ... only thing to watch out for in these is to avoid implicit casts for the
> most
> > part.
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> File pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart
> (right):
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:620: //
> Store a JS error for an exception.  For non-primitives, we store as an
expando.
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > should be triple slash doc comment?
> > 
> > also long line
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:624: Map
> _primitiveErrorCache = null;
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > `= null` should not be required.
> > 
> > also since this is top-level, you can initialize it here, you'll get
laziness
> > for free from Dart.
> 
> Removed the null.  We're generating eagerly though if I initialize here &
> failing at runtime.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:625: var
> _maxErrorCache = 10;
> On 2017/05/05 23:59:16, Jennifer Messerly wrote:
> > this is a const right? (or at least a final)
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:627:
> _isJsError(exception) {
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > bool return type?
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:676:
> stackPrint(exception) => JS(
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > this isn't new code, but it's another place I'd try and refactor the "JS"
call
> > 
> > stackPrint(exception) {
> >   var error = recordJsError(exception);
> >   JS('', 'console.log(#.stack ? #.stack : 'No stack trace for: ' + #)',
error,
> > error, error);
> > }
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/ddc_runtime/operations.dart:683:
> stackTrace(exception) => getTraceFromException(exception);
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > hmmm. I see why you had to do this (you need to create _StackTrace from
> > js_helepr) ... but it looks odd as the reader.
> > 
> > Maybe worth a comment?
> 
> Done.
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> File pkg/dev_compiler/tool/input_sdk/private/js_helper.dart (right):
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:496: return
> JS('StackTrace', 'dart.stackTrace(#)', error);
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > question ... i've seen this pattern too in dart:mirrors ... can we not just:
> > 
> > import 'dart:_runtime' as dart;
> > 
> >     return dart.stackTrace(error);
> > 
> > ... as long as we're importing 'dart:_runtime' from our own private SDK code
> > (such as this file, or our mirrors patch) it should be fine to do that.
> > 
> > Bonus: better static checking of our SDK implementations :D
> > 
> > ... last but not least I think `dart.stackTrace` is equivalent to
> > getTraceFromException, below
> 
> Oh nice - the typed call works!  
> 
> In this case though, calling getTraceFromException direct is the right call.
> 
> We should switch to typed calls though - we may also need better static typing
> to avoid additional casts though (we're avoiding some since we trust JS).
> 
>
https://codereview.chromium.org/2869463002/diff/10001/pkg/dev_compiler/tool/i...
> pkg/dev_compiler/tool/input_sdk/private/js_helper.dart:643: //=>
> JS('StackTrace', 'dart.stackTrace(#)', exception);
> On 2017/05/05 23:59:15, Jennifer Messerly wrote:
> > remove this comment?
> 
> doh!

ping

vsm

3 years, 7 months ago (2017-05-09 21:59:00 UTC) #11

Message was sent while issue was closed.

Committed patchset #3 (id:20001) manually as
36356c28c103048337e1b63c7ffd1f5459c2d1e5 (presubmit successful).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages