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

vm crash in 1.24-dev-0.0+ triggered by something in dartdoc #29620

Closed
jcollins-g opened this issue May 15, 2017 · 9 comments
Closed

vm crash in 1.24-dev-0.0+ triggered by something in dartdoc #29620

jcollins-g opened this issue May 15, 2017 · 9 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends.

Comments

@jcollins-g
Copy link
Contributor

Working on a change for dartdoc in this branch: https://github.com/dart-lang/dartdoc/tree/warnings-update

I have encountered a crash in the dart VM, as demoed by Travis: https://travis-ci.org/dart-lang/dartdoc/jobs/231722764

and locally (with some debug prints to show what dartdoc was writing at the time of the crash):

/tmp/sdkdocs/dart-html/DomMatrix/m33.html
Dumping native stack trace for thread 11891
/tmp/sdkdocs/dart-html/DomMatrix/m34.html
/tmp/sdkdocs/dart-html/DomMatrix/m41.html
./tmp/sdkdocs/dart-html/DomMatrix/m42.html
/tmp/sdkdocs/dart-html/DomMatrix/m43.html
  [0x00000000008e130a] dart::Assembler::j(dart::Condition, dart::Label*, bool)
  [0x00000000008e130a] dart::Assembler::j(dart::Condition, dart::Label*, bool)
  [0x00000000009baf34] dart::BinarySmiOpInstr::EmitNativeCode(dart::FlowGraphCompiler*)
  [0x0000000000962729] dart::FlowGraphCompiler::VisitBlocks()
  [0x000000000096e0f5] dart::FlowGraphCompiler::CompileGraph()
  [0x00000000008bd7fc] dart::CompileParsedFunctionHelper::Compile(dart::CompilationPipeline*)
  [0x00000000008be3c6] Unknown symbol
  [0x00000000008bf2a6] dart::Compiler::CompileOptimizedFunction(dart::Thread*, dart::Function const&, long)
  [0x00000000008c0cc6] dart::BackgroundCompiler::Run()
  [0x0000000000bf28bd] dart::ThreadPool::Worker::Loop()
  [0x0000000000bf276a] dart::ThreadPool::Worker::Main(unsigned long)
  [0x0000000000aaa8e9] Unknown symbol
-- End of DumpStackTrace

Process finished with exit code 134 (interrupted by signal 6: SIGABRT)

Running the same code with 1.23 works fine, and working without the change in the warnings-update branch works fine -- however, dartdoc's SDK documentation mode uses different input, of course, for different versions. First broken version is 1.24-dev-0.0.

The crash seems to occur at different places somewhat non-deterministically from the Dart code's perspective but has the same backtrace in all instances; most of the time we're somewhere in the middle of documenting DomMatrix class but we never seem to crash in the exact same place. Nothing I'm doing in the branch seems bad or obviously likely to cause a problem, but without the change, the crash vanishes.

@jcollins-g jcollins-g added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 15, 2017
@jcollins-g
Copy link
Contributor Author

jcollins-g commented May 16, 2017

I've narrowed it down to one bit that seems to cause the crash. Sadly the crash is not reproducible using the dart debugger or I'd be able to get you more data:

https://github.com/dart-lang/dartdoc/blob/warnings-update/lib/src/model.dart#L223

Here, calling accessor.name will sometimes, indeterminately, cause a crash. If I don't call accessor.name, say if I just return null, or if I call other members of accessor like nameLength or enclosingElement, there doesn't seem to be a problem. The crash is clearly data dependent; something in the dart:html library seems to exercise a path that leads to the crash, but since the dart debugger can't reproduce the problem I'm having difficulty localizing it. This code is not part of the delta in the branch, but we are using it more heavily in the branch due to the new documentationFrom method.

@mraleph
Copy link
Member

mraleph commented May 16, 2017

Thanks for looking into this yourself @jcollins-g! Just to give you a bit of background - based on the stack trace, it's a crash in the optimizing JIT compiler - which runs in the background concurrently with the main execution thread and optimizes hot functions. This probably explains why the crash is non-deterministic - because background compiler is inherently racing with the execution... This also explains why you can't reproduce the bug in the debugger - whenever you use debugger VM usually stops background compiler and prevent more functions from being optimized to give you better debugging experience.

From the stack trace I can also guess that one of the most likely reasons for the crash is that we are trying to emit a jump to a NULL deoptimization label.

Could you post the command line / instructions that reproduce the crash most reliably for you? I am away today, but I will take a close look tomorrow.

@mraleph mraleph self-assigned this May 16, 2017
@jcollins-g
Copy link
Contributor Author

Some other things I've tried that all still result in a crash: changing the declaration of accessor to an explicit type (PropertyAccessorElement), not calling replaceFirst on the name but just calling the name in a print statement, running without checked mode.

It is almost certainly a coincidence but it seems strangely linked to #29600. The crash will only occur if the accessor is actually a PropertyAccessorMember rather than a PropertyAccessorElementImpl, and goes away if I repeatedly call baseElement on the member and then call name on the PropertyAccessorElementImpl.

@jcollins-g
Copy link
Contributor Author

jcollins-g commented May 16, 2017

Some basic instructions that should reproduce this quickly:

First, ensure that you have a 1.24+ Linux SDK installed. Then:

git clone git@github.com:dart-lang/dartdoc.git
cd dartdoc
git checkout origin/warnings-update
pub get
dart bin/dartdoc.dart --output /tmp/sdkdocs --sdk-docs --show-progress
[... to crash ...]

@jcollins-g
Copy link
Contributor Author

jcollins-g commented May 16, 2017

@mraleph thanks for looking at this, please let me know if I can be of further help!

@jcollins-g
Copy link
Contributor Author

I've closed the original PR motivating this but left the branch in place for you to reproduce the problem; dart-lang/dartdoc#1428 has the workaround so I can be unblocked while you investigate.

@mraleph
Copy link
Member

mraleph commented May 17, 2017

@jcollins-g thanks!

I have already figured out what the problem is - my original guess was correct. I just need to think a little bit more about how to fix it best.

@mraleph
Copy link
Member

mraleph commented May 18, 2017

CL with a fix is up for review: https://codereview.chromium.org/2891113002/

mraleph added a commit that referenced this issue May 21, 2017
…an deopt.

For BinarySmiOpInstr and ShiftMintOpInstr we use range information to both
decide if instruction can deopt and decide which checks should be emitted
when emitting native code for this instruction.

However because range information is attached to the definition and not
uses it often gets out of sync as we mutate the graph. For example we might
first make a decision that an instruction can't deoptimize based on more
precise range information but then use less precise information in the
backend for deciding which parts of the instruction to emit (because
redifinition or a phi-instruction was removed from the graph). This mismatch
can lead to a crash if less precise information tells backend that one of the
inputs need to be checked - because there is no deoptimization label to jump
to.

This CL is addressing this problem by ensuring that range information is
cached at the use and both ComputeCanDeoptimize() and backend use the same
range information.

Additionally this CL kills overly generic MergedMath instruction and replaces it
with TruncDivModInstr.

BUG=#29620
R=rmacnak@google.com

Review-Url: https://codereview.chromium.org/2891113002 .
@mraleph
Copy link
Member

mraleph commented May 21, 2017

Should be fixed now. Thanks a lot for reporting and providing the reproduction, @jcollins-g!

@mraleph mraleph closed this as completed May 21, 2017
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, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

2 participants