From 64d30f848bbd91cc123195a0777a6bde3da71690 Mon Sep 17 00:00:00 2001 From: Vyacheslav Egorov Date: Wed, 10 Nov 2021 14:50:23 +0000 Subject: [PATCH] [vm] Fix symbolization of future listener frame. For future listener frames async unwinding produces frames with `pc_offset` set to 0, because this frames correspond to closures which will be invoked once the future completes. Such frames are different from most other frames which correspond to real call or yield locations within native code and have non-zero `pc_offset`. Because there is no call-/yield- site at 0 compiler does not emit any source location information into CodeSourceMap and consequently runtime can't produce much beyond file name in the stack trace for these special frames. To make matters worse ordering of code objects in the AOT snapshot could affect symbolization of such frames - sometimes resulting in an arbitrary token position being attached to the symbolized frame. This CL adds more special handling of these frames into StackTrace::ToCString and changes compiler to emit a dummy source position descriptor for the start of every closure function. This descriptor is encoded as ChangePosition function.token_pos() AdvancePC 0 in the CodeSourceMap. This entry is then handled specially when generating DWARF. TEST=vm/dart{,_2}/causal_stacks/async_throws_stack_lazy{,_lazy_non_symbolic}_test Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-product-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-dwarf-linux-product-x64-try Change-Id: I80740dd2c3cb03d6c03c35e1b3bde2cacbe033e0 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/219781 Commit-Queue: Slava Egorov Reviewed-by: Tess Strickland Reviewed-by: Clement Skau --- .../tests/vm/dart/causal_stacks/utils.dart | 2 +- .../tests/vm/dart_2/causal_stacks/utils.dart | 2 +- runtime/vm/code_descriptors.cc | 8 ++++ runtime/vm/code_descriptors.h | 1 + .../compiler/backend/flow_graph_compiler.cc | 47 +++++++++++++++++++ .../vm/compiler/backend/flow_graph_compiler.h | 2 + .../backend/flow_graph_compiler_arm64.cc | 32 ------------- .../backend/flow_graph_compiler_ia32.cc | 18 ------- .../backend/flow_graph_compiler_x64.cc | 27 ----------- runtime/vm/dwarf.cc | 43 ++++++++++++++++- runtime/vm/object.cc | 19 ++++++-- 11 files changed, 118 insertions(+), 83 deletions(-) diff --git a/runtime/tests/vm/dart/causal_stacks/utils.dart b/runtime/tests/vm/dart/causal_stacks/utils.dart index c8b377461ac..077e9bd01ea 100644 --- a/runtime/tests/vm/dart/causal_stacks/utils.dart +++ b/runtime/tests/vm/dart/causal_stacks/utils.dart @@ -850,7 +850,7 @@ Future doTestsLazy([String? debugInfoFilename]) async { r'^$', r'^#1 asyncStarThrowAsync \(.*/utils.dart:126(:5)?\)$', r'^$', - r'^#2 listenAsyncStarThrowAsync. \(.+/utils.dart(:0)?\)$', + r'^#2 listenAsyncStarThrowAsync. \(.+/utils.dart:132(:56)?\)$', r'^$', ]; await doTestAwait( diff --git a/runtime/tests/vm/dart_2/causal_stacks/utils.dart b/runtime/tests/vm/dart_2/causal_stacks/utils.dart index cd46b037f95..a17cade3871 100644 --- a/runtime/tests/vm/dart_2/causal_stacks/utils.dart +++ b/runtime/tests/vm/dart_2/causal_stacks/utils.dart @@ -852,7 +852,7 @@ Future doTestsLazy([String debugInfoFilename]) async { r'^$', r'^#1 asyncStarThrowAsync \(.*/utils.dart:128(:5)?\)$', r'^$', - r'^#2 listenAsyncStarThrowAsync. \(.+/utils.dart(:0)?\)$', + r'^#2 listenAsyncStarThrowAsync. \(.+/utils.dart:134(:56)?\)$', r'^$', ]; await doTestAwait( diff --git a/runtime/vm/code_descriptors.cc b/runtime/vm/code_descriptors.cc index dd8bc1d8b6d..87625f36b6a 100644 --- a/runtime/vm/code_descriptors.cc +++ b/runtime/vm/code_descriptors.cc @@ -417,6 +417,14 @@ void CodeSourceMapBuilder::StartInliningInterval( } } +void CodeSourceMapBuilder::WriteFunctionEntrySourcePosition( + const InstructionSource& source) { + ASSERT(written_pc_offset_ == 0 && buffered_pc_offset_ == 0); + ASSERT(stream_.bytes_written() == 0); + WriteChangePosition(source.token_pos); + WriteAdvancePC(0); +} + void CodeSourceMapBuilder::BeginCodeSourceRange( int32_t pc_offset, const InstructionSource& source) { diff --git a/runtime/vm/code_descriptors.h b/runtime/vm/code_descriptors.h index ac29be74348..bd0d35802ab 100644 --- a/runtime/vm/code_descriptors.h +++ b/runtime/vm/code_descriptors.h @@ -246,6 +246,7 @@ class CodeSourceMapBuilder : public ZoneAllocated { void NoteNullCheck(int32_t pc_offset, const InstructionSource& source, intptr_t name_index); + void WriteFunctionEntrySourcePosition(const InstructionSource& source); // If source is from an inlined call, returns the token position of the // original call in the root function, otherwise the source's token position. diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.cc b/runtime/vm/compiler/backend/flow_graph_compiler.cc index d8ce910906e..88106b27f08 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler.cc @@ -560,6 +560,53 @@ bool FlowGraphCompiler::IsPeephole(Instruction* instr) const { return false; } +void FlowGraphCompiler::EmitFunctionEntrySourcePositionDescriptorIfNeeded() { + // When unwinding async stacks we might produce frames which correspond + // to future listeners which are going to be called when the future completes. + // These listeners are not yet called and thus their frame pc_offset is set + // to 0 - which does not actually correspond to any call- or yield- site + // inside the code object. Nevertheless we would like to be able to + // produce proper position information for it when symbolizing the stack. + // To achieve that in AOT mode (where we don't actually have + // |Function::token_pos| available) we instead emit an artificial descriptor + // at the very beginning of the function. + if (FLAG_precompiled_mode && flow_graph().function().IsClosureFunction()) { + code_source_map_builder_->WriteFunctionEntrySourcePosition( + InstructionSource(flow_graph().function().token_pos())); + } +} + +void FlowGraphCompiler::CompileGraph() { + InitCompiler(); + +#if !defined(TARGET_ARCH_IA32) + // For JIT we have multiple entrypoints functionality which moved the frame + // setup into the [TargetEntryInstr] (which will set the constant pool + // allowed bit to true). Despite this we still have to set the + // constant pool allowed bit to true here as well, because we can generate + // code for [CatchEntryInstr]s, which need the pool. + assembler()->set_constant_pool_allowed(true); +#endif + + EmitFunctionEntrySourcePositionDescriptorIfNeeded(); + VisitBlocks(); + +#if defined(DEBUG) + assembler()->Breakpoint(); +#endif + + if (!skip_body_compilation()) { +#if !defined(TARGET_ARCH_IA32) + ASSERT(assembler()->constant_pool_allowed()); +#endif + GenerateDeferredCode(); + } + + for (intptr_t i = 0; i < indirect_gotos_.length(); ++i) { + indirect_gotos_[i]->ComputeOffsetTable(this); + } +} + void FlowGraphCompiler::VisitBlocks() { CompactBlocks(); if (compiler::Assembler::EmittingComments()) { diff --git a/runtime/vm/compiler/backend/flow_graph_compiler.h b/runtime/vm/compiler/backend/flow_graph_compiler.h index 7669e782304..98488ae55f7 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler.h +++ b/runtime/vm/compiler/backend/flow_graph_compiler.h @@ -559,6 +559,8 @@ class FlowGraphCompiler : public ValueObject { void VisitBlocks(); + void EmitFunctionEntrySourcePositionDescriptorIfNeeded(); + // Bail out of the flow graph compiler. Does not return to the caller. void Bailout(const char* reason); diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc b/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc index fdbf099aab6..233821af1f3 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_arm64.cc @@ -372,38 +372,6 @@ void FlowGraphCompiler::EmitPrologue() { EndCodeSourceRange(PrologueSource()); } -// Input parameters: -// LR: return address. -// SP: address of last argument. -// FP: caller's frame pointer. -// PP: caller's pool pointer. -// R4: arguments descriptor array. -void FlowGraphCompiler::CompileGraph() { - InitCompiler(); - - // For JIT we have multiple entrypoints functionality which moved the frame - // setup into the [TargetEntryInstr] (which will set the constant pool - // allowed bit to true). Despite this we still have to set the - // constant pool allowed bit to true here as well, because we can generate - // code for [CatchEntryInstr]s, which need the pool. - __ set_constant_pool_allowed(true); - - VisitBlocks(); - -#if defined(DEBUG) - __ brk(0); -#endif - - if (!skip_body_compilation()) { - ASSERT(assembler()->constant_pool_allowed()); - GenerateDeferredCode(); - } - - for (intptr_t i = 0; i < indirect_gotos_.length(); ++i) { - indirect_gotos_[i]->ComputeOffsetTable(this); - } -} - void FlowGraphCompiler::EmitCallToStub(const Code& stub) { ASSERT(!stub.IsNull()); if (CanPcRelativeCall(stub)) { diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc b/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc index 0b232ddd619..d613572dd38 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_ia32.cc @@ -459,24 +459,6 @@ void FlowGraphCompiler::EmitPrologue() { EndCodeSourceRange(PrologueSource()); } -void FlowGraphCompiler::CompileGraph() { - InitCompiler(); - - ASSERT(!block_order().is_empty()); - VisitBlocks(); - - if (!skip_body_compilation()) { -#if defined(DEBUG) - __ int3(); -#endif - GenerateDeferredCode(); - } - - for (intptr_t i = 0; i < indirect_gotos_.length(); ++i) { - indirect_gotos_[i]->ComputeOffsetTable(this); - } -} - void FlowGraphCompiler::EmitCallToStub(const Code& stub) { if (stub.InVMIsolateHeap()) { __ CallVmStub(stub); diff --git a/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc b/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc index 573add6f8d6..3abdf84e10f 100644 --- a/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc +++ b/runtime/vm/compiler/backend/flow_graph_compiler_x64.cc @@ -378,33 +378,6 @@ void FlowGraphCompiler::EmitPrologue() { EndCodeSourceRange(PrologueSource()); } -void FlowGraphCompiler::CompileGraph() { - InitCompiler(); - - // We have multiple entrypoints functionality which moved the frame - // setup into the [FunctionEntryInstr] (which will set the constant pool - // allowed bit to true). Despite this we still have to set the - // constant pool allowed bit to true here as well, because we can generate - // code for [CatchEntryInstr]s, which need the pool. - __ set_constant_pool_allowed(true); - - ASSERT(!block_order().is_empty()); - VisitBlocks(); - -#if defined(DEBUG) - __ int3(); -#endif - - if (!skip_body_compilation()) { - ASSERT(assembler()->constant_pool_allowed()); - GenerateDeferredCode(); - } - - for (intptr_t i = 0; i < indirect_gotos_.length(); ++i) { - indirect_gotos_[i]->ComputeOffsetTable(this); - } -} - void FlowGraphCompiler::EmitCallToStub(const Code& stub) { ASSERT(!stub.IsNull()); if (CanPcRelativeCall(stub)) { diff --git a/runtime/vm/dwarf.cc b/runtime/vm/dwarf.cc index 4f9884882ca..d661bd74cdb 100644 --- a/runtime/vm/dwarf.cc +++ b/runtime/vm/dwarf.cc @@ -717,6 +717,26 @@ void Dwarf::WriteLineNumberProgramFromCodeSourceMaps( function_stack.Clear(); token_positions.Clear(); + // CodeSourceMap might start in the following way: + // + // ChangePosition function.token_pos() + // AdvancePC 0 + // ChangePosition x + // AdvancePC y + // + // This entry is emitted to ensure correct symbolization of + // function listener frames produced by async unwinding. + // (See EmitFunctionEntrySourcePositionDescriptorIfNeeded). + // Directly interpreting this sequence would cause us to emit + // multiple with the same pc into line number table and different + // position information. To avoid this will make an adjustment for + // the second record we emit: if position x is a synthetic one we will + // simply drop the second record, if position x is real then we will + // emit row with a slightly adjusted PC (by 1 byte). This would not + // affect symbolization (you can't have a call that is 1 byte long) + // but will avoid line number table entries with the same PC. + bool function_entry_position_was_emitted = false; + int32_t current_pc_offset = 0; function_stack.Add(&root_function); token_positions.Add(kNoDwarfPositionInfo); @@ -740,9 +760,30 @@ void Dwarf::WriteLineNumberProgramFromCodeSourceMaps( const intptr_t file = LookupScript(script); const intptr_t line = token_positions.Last().line(); const intptr_t column = token_positions.Last().column(); - writer->EmitRow(file, line, column, asm_name, current_pc_offset); + intptr_t pc_offset_adjustment = 0; + bool should_emit = true; + + // If we are at the function entry and have already emitted a row + // then adjust current_pc_offset to avoid duplicated entries. + // See the comment below which explains why this code is here. + if (current_pc_offset == 0 && function_entry_position_was_emitted) { + pc_offset_adjustment = 1; + // Ignore synthetic positions. Function entry position gives + // more information anyway. + should_emit = !(line == 0 && column == 0); + } + + if (should_emit) { + writer->EmitRow(file, line, column, asm_name, + current_pc_offset + pc_offset_adjustment); + } current_pc_offset += arg1; + if (arg1 == 0) { // Special case of AdvancePC 0. + ASSERT(current_pc_offset == 0); + ASSERT(!function_entry_position_was_emitted); + function_entry_position_was_emitted = true; + } break; } case CodeSourceMapOps::kPushFunction: { diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc index e272c5cd096..fd8822cb0e8 100644 --- a/runtime/vm/object.cc +++ b/runtime/vm/object.cc @@ -25851,6 +25851,15 @@ const char* StackTrace::ToCString() const { // A visible frame ends any gap we might be in. in_gap = false; + // Zero pc_offset can only occur in the frame produced by the async + // unwinding and it corresponds to the next future listener in the + // chain. This function is not yet called (it will be called when + // the future completes) hence pc_offset is set to 0. This frame + // is very different from other frames which have pc_offsets + // corresponding to call- or yield-sites in the generated code and + // should be handled specially. + const bool is_future_listener = pc_offset == 0; + #if defined(DART_PRECOMPILED_RUNTIME) // When printing non-symbolic frames, we normally print call // addresses, not return addresses, by subtracting one from the PC to @@ -25861,7 +25870,6 @@ const char* StackTrace::ToCString() const { // is invoked with the value of the resolved future. Thus, we must // report the return address, as returning a value before the closure // payload will cause failures to decode the frame using DWARF info. - const bool is_future_listener = pc_offset == 0; const uword call_addr = is_future_listener ? pc : pc - 1; if (FLAG_dwarf_stack_traces_mode) { @@ -25887,7 +25895,11 @@ const char* StackTrace::ToCString() const { } #endif - if (code.is_optimized() && stack_trace.expand_inlined()) { + if (code.is_optimized() && stack_trace.expand_inlined() && + (FLAG_precompiled_mode || !is_future_listener)) { + // Note: In AOT mode EmitFunctionEntrySourcePositionDescriptorIfNeeded + // will take care of emitting a descriptor that would allow us to + // symbolize stack frame with 0 offset. code.GetInlinedFunctionsAtReturnAddress(pc_offset, &inlined_functions, &inlined_token_positions); ASSERT(inlined_functions.length() >= 1); @@ -25901,7 +25913,8 @@ const char* StackTrace::ToCString() const { continue; } - auto const pos = code.GetTokenIndexOfPC(pc); + auto const pos = is_future_listener ? function.token_pos() + : code.GetTokenIndexOfPC(pc); PrintSymbolicStackFrame(zone, &buffer, function, pos, frame_index); frame_index++; }