Subzero: Fixed deadlock with _start but no globals

Subzero: Fixed deadlock when _start is first function

It was previously the case that instrumentStart in ASanInstrumentation would block until instrumentGlobals had completed. This was because instrumentStart depends on the global redzones having been inserted. However, instrumentGlobals was not called until the first function was popped off the emit queue, and when _start was the first function, it was not placed on the emit queue until after it had been instrumented and lowered. instrumentStart was waiting for instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=2c9992a596475c09234ee69e66b5ae715ca33124

[tlively, 2016-07-19 07:59:54]

tlively@google.com changed reviewers:
+ kschimpf@google.com, mcgrathr@chromium.org, stichnot@chromium.org

[tlively, 2016-07-19 07:59:54]

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:339: while (!Ctx->getGlobalsReceived()) {
This spinning could be replaced by a condition variable, but I wanted to make
sure this general direction looked good first.

[Jim Stichnoth, 2016-07-19 14:04:13]

Also, please beef up the CL description to explain the deadlock scenario.

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:77: GlobalsMutex.lock();
Can you do this?

  std::unique_lock<std::mutex> _(GlobalsMutex);

Then you can remove the lock() and unlock() calls.

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:339: while (!Ctx->getGlobalsReceived()) {
On 2016/07/19 07:59:54, tlively wrote:
> This spinning could be replaced by a condition variable, but I wanted to make
> sure this general direction looked good first.

Definitely add a TODO about properly using a condition variable.

Also, it would probably be good to do something like
  std::this_thread::yield();
inside the spin loop.

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
File tests_lit/asan_tests/no_globals.ll (right):

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
tests_lit/asan_tests/no_globals.ll:1: ; Check that Subzero can instrument _start
when there are no globals
Can you add a more complete description of why this test is here?  Specifically
about the possibility of deadlock when there are no global initializers and
_start is the first function.

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
tests_lit/asan_tests/no_globals.ll:12: ; DUMP-LABEL: ================
Instrumented CFG ================
Can the tested output be made less specific?

I thought it was a deadlock issue, so would simply
  DUMP: __asan_init
be a sufficient test?

(But be sure to verify that it fails with the old code and succeeds with the CL,
before taking my suggestion...)

I say this because the dump output format isn't strongly guaranteed to be stable
over time.  For example, we might want to change the "Instrumented CFG" string,
or the number of '=' characters, etc.

[tlively, 2016-07-19 22:27:04]

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation.cpp
File src/IceASanInstrumentation.cpp (right):

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:77: GlobalsMutex.lock();
On 2016/07/19 14:04:13, stichnot wrote:
> Can you do this?
> 
>   std::unique_lock<std::mutex> _(GlobalsMutex);
> 
> Then you can remove the lock() and unlock() calls.

Done.

https://codereview.chromium.org/2165493002/diff/1/src/IceASanInstrumentation....
src/IceASanInstrumentation.cpp:339: while (!Ctx->getGlobalsReceived()) {
On 2016/07/19 14:04:13, stichnot wrote:
> On 2016/07/19 07:59:54, tlively wrote:
> > This spinning could be replaced by a condition variable, but I wanted to
make
> > sure this general direction looked good first.
> 
> Definitely add a TODO about properly using a condition variable.
> 
> Also, it would probably be good to do something like
>   std::this_thread::yield();
> inside the spin loop.

Done.

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
File tests_lit/asan_tests/no_globals.ll (right):

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
tests_lit/asan_tests/no_globals.ll:1: ; Check that Subzero can instrument _start
when there are no globals
On 2016/07/19 14:04:13, stichnot wrote:
> Can you add a more complete description of why this test is here? 
Specifically
> about the possibility of deadlock when there are no global initializers and
> _start is the first function.

Done.

https://codereview.chromium.org/2165493002/diff/1/tests_lit/asan_tests/no_glo...
tests_lit/asan_tests/no_globals.ll:12: ; DUMP-LABEL: ================
Instrumented CFG ================
On 2016/07/19 14:04:13, stichnot wrote:
> Can the tested output be made less specific?
> 
> I thought it was a deadlock issue, so would simply
>   DUMP: __asan_init
> be a sufficient test?
> 
> (But be sure to verify that it fails with the old code and succeeds with the
CL,
> before taking my suggestion...)
> 
> I say this because the dump output format isn't strongly guaranteed to be
stable
> over time.  For example, we might want to change the "Instrumented CFG"
string,
> or the number of '=' characters, etc.

Done.

[tlively, 2016-07-19 22:37:32]

Description was changed from

==========
Subzero: Fixed deadlock with _start but no globals

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Subzero: Fixed deadlock with _start but no globals

It was previously the case that instrumentStart in ASanInstrumentation would
block until instrumentGlobals had completed. This was because instrumentStart
depends on the global redzones having been inserted. However, instrumentGlobals
was not called until the first function was popped off the emit queue, and when
_start was the first function, it was not placed on the emit queue until after
it had been instrumented and lowered. instrumentStart was waiting for
instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

[tlively, 2016-07-19 22:38:17]

Description was changed from

==========
Subzero: Fixed deadlock with _start but no globals

It was previously the case that instrumentStart in ASanInstrumentation would
block until instrumentGlobals had completed. This was because instrumentStart
depends on the global redzones having been inserted. However, instrumentGlobals
was not called until the first function was popped off the emit queue, and when
_start was the first function, it was not placed on the emit queue until after
it had been instrumented and lowered. instrumentStart was waiting for
instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Subzero: Fixed deadlock when _start is first function

It was previously the case that instrumentStart in ASanInstrumentation would
block until instrumentGlobals had completed. This was because instrumentStart
depends on the global redzones having been inserted. However, instrumentGlobals
was not called until the first function was popped off the emit queue, and when
_start was the first function, it was not placed on the emit queue until after
it had been instrumented and lowered. instrumentStart was waiting for
instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

[Jim Stichnoth, 2016-07-19 22:39:00]

https://codereview.chromium.org/2165493002/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/2165493002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:619: if (!BuildDefs::minimal() && Instrumentor != NULL)
nullptr

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:122: std::unique_lock<std::mutex>
GlobalsLock(GlobalsSeenMutex);
I think this could be simpler:

void Instrumentation::setHasSeenGlobals() {
  {
    std::unique_lock<std::mutex> _(GlobalsSeenMutex);
    HasSeenGlobals = true;
  }
  GlobalsSeenCV.notify_all();
}

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:130: GlobalsSeenCV.wait(GlobalsLock, [&] { return
HasSeenGlobals; });
Instead of default capture, can you capture "this"?

  GlobalsSeenCV.wait(GlobalsLock, [this] { return HasSeenGlobals; });

[tlively, 2016-07-19 23:20:14]

https://codereview.chromium.org/2165493002/diff/20001/src/IceGlobalContext.h
File src/IceGlobalContext.h (right):

https://codereview.chromium.org/2165493002/diff/20001/src/IceGlobalContext.h#...
src/IceGlobalContext.h:619: if (!BuildDefs::minimal() && Instrumentor != NULL)
On 2016/07/19 22:39:00, stichnot wrote:
> nullptr

Done.

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation.cpp
File src/IceInstrumentation.cpp (right):

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:122: std::unique_lock<std::mutex>
GlobalsLock(GlobalsSeenMutex);
On 2016/07/19 22:39:00, stichnot wrote:
> I think this could be simpler:
> 
> void Instrumentation::setHasSeenGlobals() {
>   {
>     std::unique_lock<std::mutex> _(GlobalsSeenMutex);
>     HasSeenGlobals = true;
>   }
>   GlobalsSeenCV.notify_all();
> }

Done.

https://codereview.chromium.org/2165493002/diff/20001/src/IceInstrumentation....
src/IceInstrumentation.cpp:130: GlobalsSeenCV.wait(GlobalsLock, [&] { return
HasSeenGlobals; });
On 2016/07/19 22:39:00, stichnot wrote:
> Instead of default capture, can you capture "this"?
> 
>   GlobalsSeenCV.wait(GlobalsLock, [this] { return HasSeenGlobals; });

Done.

[Karl, 2016-07-20 17:19:09]

lgtm

[Jim Stichnoth, 2016-07-20 17:36:11]

lgtm

[tlively, 2016-07-20 18:19:20]

Description was changed from

==========
Subzero: Fixed deadlock when _start is first function

It was previously the case that instrumentStart in ASanInstrumentation would
block until instrumentGlobals had completed. This was because instrumentStart
depends on the global redzones having been inserted. However, instrumentGlobals
was not called until the first function was popped off the emit queue, and when
_start was the first function, it was not placed on the emit queue until after
it had been instrumented and lowered. instrumentStart was waiting for
instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
==========

to

==========
Subzero: Fixed deadlock when _start is first function

It was previously the case that instrumentStart in ASanInstrumentation would
block until instrumentGlobals had completed. This was because instrumentStart
depends on the global redzones having been inserted. However, instrumentGlobals
was not called until the first function was popped off the emit queue, and when
_start was the first function, it was not placed on the emit queue until after
it had been instrumented and lowered. instrumentStart was waiting for
instrumentGlobals, which could not happen until instrumentStart completed.

BUG=https://bugs.chromium.org/p/nativeclient/issues/detail?id=4374
R=kschimpf@google.com, stichnot@chromium.org

Committed:
https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a...
==========

[tlively, 2016-07-20 18:19:22]

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