Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(74)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11883023: Collect debugging info for catch clauses (Closed)

Created:
7 years, 11 months ago by hausner
Modified:
7 years, 11 months ago
Reviewers:
siva
CC:
reviews_dartlang.org
Visibility:
Public.

Description

Collect debugging info for catch clauses Collect info about try-statement nesting, and a list of handled types in each catch clause. This will be used by the debugger to determine whether an exception is handled by any handler on the stack. Committed: https://code.google.com/p/dart/source/detail?r=17100

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Total comments: 23

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+195 lines, -81 lines) Patch
M runtime/vm/ast.h View 1 2 3 3 chunks +5 lines, -0 lines 0 comments Download
M runtime/vm/code_descriptors.h View 1 2 3 3 chunks +20 lines, -4 lines 0 comments Download
M runtime/vm/flow_graph_builder.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M runtime/vm/flow_graph_compiler.cc View 1 2 3 1 chunk +7 lines, -2 lines 0 comments Download
M runtime/vm/flow_graph_compiler_ia32.h View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M runtime/vm/flow_graph_compiler_x64.h View 1 2 3 1 chunk +4 lines, -1 line 0 comments Download
M runtime/vm/intermediate_language.h View 1 2 3 3 chunks +6 lines, -1 line 0 comments Download
M runtime/vm/intermediate_language.cc View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M runtime/vm/object.h View 1 2 3 3 chunks +16 lines, -28 lines 0 comments Download
M runtime/vm/object.cc View 1 2 3 3 chunks +83 lines, -25 lines 0 comments Download
M runtime/vm/object_test.cc View 1 2 3 2 chunks +11 lines, -6 lines 0 comments Download
M runtime/vm/parser.cc View 1 2 3 3 chunks +15 lines, -5 lines 0 comments Download
M runtime/vm/raw_object.h View 1 2 3 1 chunk +15 lines, -3 lines 0 comments Download
M runtime/vm/raw_object.cc View 1 2 3 2 chunks +5 lines, -4 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
hausner
7 years, 11 months ago (2013-01-14 23:22:52 UTC) #1
hausner
Trying this again.
7 years, 11 months ago (2013-01-15 19:10:49 UTC) #2
siva
lgtm https://chromiumcodereview.appspot.com/11883023/diff/1003/runtime/vm/object.cc File runtime/vm/object.cc (right): https://chromiumcodereview.appspot.com/11883023/diff/1003/runtime/vm/object.cc#newcode6895 runtime/vm/object.cc:6895: ASSERT(index < Length()); ASSERT(index >= 0 && index ...
7 years, 11 months ago (2013-01-15 22:04:12 UTC) #3
hausner
7 years, 11 months ago (2013-01-15 22:44:52 UTC) #4
Message was sent while issue was closed. 
Thank you.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6895: ASSERT(index < Length());
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length())

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6905: ASSERT(index < Length());
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length())

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6906: RawExceptionHandlers::HandlerInfo* data =
&raw_ptr()->data_[index];
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(info != NULL);

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6914: ASSERT(index < Length());
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length())

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6920: ASSERT(index < Length());
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length());

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6926: const Array& handled_types) const {
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length());

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6933: RawArray* ExceptionHandlers::GetHandledTypes(intptr_t
index) const {
On 2013/01/15 22:04:12, siva wrote:
> ASSERT(index >= 0 && index < Length());

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6946: ASSERT(Object::exception_handlers_class() !=
Class::null());
On 2013/01/15 22:04:12, siva wrote:
> You should probably still retain the check as an ASSERT?

Done, but to be an effective security check it has to be checked in non-debug
builds as well.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.cc#newco...
runtime/vm/object.cc:6957: result.set_handled_types_data(handled_types_data);
On 2013/01/15 22:04:12, siva wrote:
> Pull the result.set_handled_types_data code outside the NoGCScope. At some
point
> StorePointer may trigger a GC.
> 
> I would also pull the handled_types_data declaration down to just before the
> set_handled_types_data(...) code.

Done.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/object.h#newcod...
runtime/vm/object.h:2407: 
On 2013/01/15 22:04:12, siva wrote:
> I think you still need the kMaxElements constant to ensure that we don't
create
> too many of these entries (this was a security check that Todd had added).

Done. Picked a large but otherwise arbitrary number as the upper limit.

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/11883023/diff/1003/runtime/vm/parser.cc#newco...
runtime/vm/parser.cc:6076: handler_types.Add(*exception_param.type);
On 2013/01/15 22:04:12, siva wrote:
> We should also be ok to not collect any more catch clause types from this
point
> on right?
> 
> e.g:
> try {
>   ...
>   ....
> }
> on type1 catch() { ... }
> catch() { ... }
> on type2 catch() { ... }
> 
> I know this code looks weird but I assume is still legal.

The compiler prints an error message if there are any more catch clauses after a
catch clause that catches type 'dynamic'. So, essentially we can never have a
case where we collect more types since this has to be the last catch clause we
see.

Powered by Google App Engine
This is Rietveld 408576698