Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gc_marker.cc ASSERT failure #26706

Closed
johnmccutchan opened this issue Jun 14, 2016 · 8 comments
Closed

gc_marker.cc ASSERT failure #26706

johnmccutchan opened this issue Jun 14, 2016 · 8 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@johnmccutchan
Copy link
Contributor

https://build.chromium.org/p/client.dart/builders/vm-linux-debug-x64-be/builds/10042/steps/checked%20vm%20tests/logs/stdio

FAILED: none-vm-checked debug_x64 lib/mirrors/mirrors_reader_test
Expected: Pass 
Actual: Crash
CommandOutput[vm]:

stderr:
runtime/vm/gc_marker.cc:609: error: expected: AtomicOperations::LoadRelaxed(num_busy_) == 0


Command[vm]: DART_CONFIGURATION=DebugX64 out/DebugX64/dart --enable_asserts --enable_type_checks --ignore-unrecognized-flags --package-root=out/DebugX64/packages/ /mnt/data/b/build/slave/vm-linux-debug-x64-be/build/sdk/tests/lib/mirrors/mirrors_reader_test.dart
Took 0:00:05.694883

Short reproduction command (experimental):
    python tools/test.py --write-debug-log --write-test-outcome-log --copy-coredumps --exclude-suite pkg --checked -t120 lib/mirrors/mirrors_reader_test



===
=== 1 test failed
===
@johnmccutchan
Copy link
Contributor Author

@a-siva @mraleph

@johnmccutchan johnmccutchan added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Jun 14, 2016
@a-siva a-siva self-assigned this Jun 14, 2016
@mraleph
Copy link
Member

mraleph commented Jun 15, 2016

Oh, this is an obvious bug, sorry about this. It's a race: one thread manages to reach an increment marked (2), then another thread checks num_busy_ at (1) and sees an incremented value.

We should just remove the assertion. I originally removed it precisely because it is very hard to make it work in the current setup and then I reinstated it thinking it's fine to do after the barrier - but missed this case.

        barrier_->Sync();
        ASSERT(AtomicOperations::LoadRelaxed(num_busy_) == 0); // (1)

        // Check if we have any pending properties with marked keys.
        // Those might have been marked by another marker.
        more_to_mark = visitor.ProcessPendingWeakProperties();
        if (more_to_mark) {
          // We have more work to do. Notify others.
          AtomicOperations::FetchAndIncrement(num_busy_);  // (2)
        }

        // Wait for all other markers to finish processing their pending
        // weak properties and decide if they need to continue marking.
        // Caveat: we need two barriers here to make this decision in lock step
        // between all markers and the main thread.
        barrier_->Sync();

alternatively we can do:

        barrier_->Sync();
#if defined(DEBUG)
        ASSERT(AtomicOperations::LoadRelaxed(num_busy_) == 0); // (1)
        barrier->Sync();
#endif

(main thread would need to be fixed accordingly).

@fsc8000
Copy link
Contributor

fsc8000 commented Jul 22, 2016

@johnmccutchan
Copy link
Contributor Author

Hit it again:
https://build.chromium.org/p/client.dart/builders/vm-linux-debug-x64-live-reload-be/builds/120/steps/vm%20tests/logs/stdio


FAILED: none-vm debug_x64 lib/mirrors/mirrors_reader_test
Expected: Pass 
Actual: Crash
CommandOutput[vm]:

stderr:
runtime/vm/gc_marker.cc: 613: error: expected: AtomicOperations::LoadRelaxed(num_busy_) == 0


Command[vm]: DART_CONFIGURATION=DebugX64 out/DebugX64/dart --hot-reload-test-mode --ignore-unrecognized-flags --package-root=out/DebugX64/packages/ /b/build/slave/vm-linux-debug-x64-live-reload-be/build/sdk/tests/lib/mirrors/mirrors_reader_test.dart
Took 0:00:21.006147

Short reproduction command (experimental):
    python tools/test.py --write-debug-log --write-test-outcome-log --copy-coredumps --exclude-suite pkg --hot-reload --builder-tag no_ipv6 -t120 lib/mirrors/mirrors_reader_test

@mraleph
Copy link
Member

mraleph commented Jul 29, 2016

This should fix it

https://codereview.chromium.org/2191313002/

@mraleph
Copy link
Member

mraleph commented Aug 1, 2016

Adjusted the assertion to avoid data-race in 59e67cc.

@mraleph mraleph closed this as completed Aug 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

5 participants