Make NoReloadScope thread safe

Make NoReloadScope thread safe

Fixes #27366

BUG=
R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/d11363889160ff9301e6b34e1a2b9538c387e0f2

[Cutch, 2016-09-16 14:18:20]

Description was changed from

==========
Make NoReloadScope thread safe

Fixes #27366

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

to

==========
Make NoReloadScope thread safe

Fixes #27366

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

[Cutch, 2016-09-16 14:18:20]

johnmccutchan@google.com changed reviewers:
+ fschneider@google.com, rmacnak@google.com

[Florian Schneider, 2016-09-16 16:22:41]

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/atomic.h
File runtime/vm/atomic.h (right):

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/atomic.h#newcode37
runtime/vm/atomic.h:37: static intptr_t FetchAndDecrement(intptr_t* p);
Not needed? If we need both variants, I'd make this a template function (see
LoadRelaxed)

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:826: uintptr_t no_reload_scope_depth_;  // we can only
reload when this is 0.
What is the reason for using uintptr_t?

[Cutch, 2016-09-16 16:45:31]

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/atomic.h
File runtime/vm/atomic.h (right):

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/atomic.h#newcode37
runtime/vm/atomic.h:37: static intptr_t FetchAndDecrement(intptr_t* p);
On 2016/09/16 16:22:41, Florian Schneider wrote:
> Not needed? If we need both variants, I'd make this a template function (see
> LoadRelaxed)

Oops. I meant to delete this.

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/2344193002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:826: uintptr_t no_reload_scope_depth_;  // we can only
reload when this is 0.
On 2016/09/16 16:22:41, Florian Schneider wrote:
> What is the reason for using uintptr_t?

Because the AtomicOperations I need are typed for uintptr_t and not intptr_t.

[Cutch, 2016-09-16 17:50:00]

As discussed offline I've implemented the signed atomic operations.

PTAL.

[Florian Schneider, 2016-09-16 18:01:37]

Lgtm.

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test.cc
File runtime/vm/atomic_test.cc (right):

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test....
runtime/vm/atomic_test.cc:31: EXPECT_EQ(static_cast<uintptr_t>(-42),
intptr_t

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test....
runtime/vm/atomic_test.cc:33: EXPECT_EQ(static_cast<uintptr_t>(-41), v);
intptr_t?

Or remove these static_casts. They seem redundant.

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test....
runtime/vm/atomic_test.cc:39: EXPECT_EQ(static_cast<uintptr_t>(-42),
Same as above.

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test....
runtime/vm/atomic_test.cc:41: EXPECT_EQ(static_cast<uintptr_t>(-43), v);
Same as above.

[Cutch, 2016-09-16 18:07:51]

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test.cc
File runtime/vm/atomic_test.cc (right):

https://codereview.chromium.org/2344193002/diff/40001/runtime/vm/atomic_test....
runtime/vm/atomic_test.cc:31: EXPECT_EQ(static_cast<uintptr_t>(-42),
On 2016/09/16 18:01:36, Florian Schneider wrote:
> intptr_t

Done here and elsewhere.

[Cutch, 2016-09-16 18:18:41]

Description was changed from

==========
Make NoReloadScope thread safe

Fixes #27366

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

to

==========
Make NoReloadScope thread safe

Fixes #27366

BUG=
R=fschneider@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d11363889160ff9301e6b34e1a2b9538c387e0f2
==========

[Cutch, 2016-09-16 18:18:43]

Committed patchset #4 (id:60001) manually as
d11363889160ff9301e6b34e1a2b9538c387e0f2 (presubmit successful).