From dfcb7b6799b7c1e2c8d65e857d8afede185471d8 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 26 Oct 2021 09:31:09 -0400 Subject: [PATCH] Revert "Fix incorrect source file name for inlined frames" This reverts commit 54d878abcb61623a71e5c2b5bb251e7f7fc8563d. 54d878abcb61 changed the dump_syms format incompatibly. This must be redone in a multi-step process: the processor must be made to understand the old and new formats simultaneously and the processor service must be rebuilt and run with that update before dump_syms output can change to use the new format. Bug: chromium:1263390 Change-Id: I5b6f8aff8ea2916b2c07ac6a74b569fa27db51b9 Reviewed-on: https://chromium-review.googlesource.com/c/breakpad/breakpad/+/3244775 Reviewed-by: Joshua Peraza --- src/common/dwarf_cu_to_module.cc | 36 +++--- src/common/module.cc | 42 ++++-- src/common/module.h | 39 ++---- .../processor/basic_source_line_resolver.h | 22 ++-- .../processor/source_line_resolver_base.h | 3 +- .../source_line_resolver_interface.h | 3 +- .../processor/stack_frame_symbolizer.h | 3 +- src/processor/basic_source_line_resolver.cc | 111 ++++++++-------- .../basic_source_line_resolver_types.h | 14 +- .../basic_source_line_resolver_unittest.cc | 120 +++++++++--------- src/processor/fast_source_line_resolver.cc | 2 +- .../fast_source_line_resolver_types.h | 2 +- src/processor/source_line_resolver_base.cc | 2 +- .../source_line_resolver_base_types.h | 11 +- src/processor/stack_frame_symbolizer.cc | 2 +- src/processor/stackwalk_common.cc | 2 +- src/processor/stackwalker.cc | 8 +- .../linux_inline.sym | 15 +-- 18 files changed, 214 insertions(+), 223 deletions(-) diff --git a/src/common/dwarf_cu_to_module.cc b/src/common/dwarf_cu_to_module.cc index 82131e7b..04d19479 100644 --- a/src/common/dwarf_cu_to_module.cc +++ b/src/common/dwarf_cu_to_module.cc @@ -254,6 +254,9 @@ struct DwarfCUToModule::CUContext { // A map of function pointers to the its forward specification DIE's offset. map spec_function_offsets; + + // From file index to vector of subprogram's offset in this CU. + map> inline_origins; }; // Information about the context of a particular DIE. This is for @@ -558,8 +561,7 @@ class DwarfCUToModule::InlineHandler : public GenericDIEHandler { DwarfForm high_pc_form_; // DW_AT_high_pc can be length or address. DwarfForm ranges_form_; // DW_FORM_sec_offset or DW_FORM_rnglistx uint64_t ranges_data_; // DW_AT_ranges - int call_site_line_; // DW_AT_call_line - int call_site_file_id_; // DW_AT_call_file + int call_site_line_; int inline_nest_level_; // A vector of inlines in the same nest level. It's owned by its parent // function/inline. At Finish(), add this inline into the vector. @@ -587,9 +589,6 @@ void DwarfCUToModule::InlineHandler::ProcessAttributeUnsigned( case DW_AT_call_line: call_site_line_ = data; break; - case DW_AT_call_file: - call_site_file_id_ = data; - break; default: GenericDIEHandler::ProcessAttributeUnsigned(attr, form, data); break; @@ -662,8 +661,8 @@ void DwarfCUToModule::InlineHandler::Finish() { cu_context_->file_context->module_->inline_origin_map .GetOrCreateInlineOrigin(specification_offset_, name_); unique_ptr in( - new Module::Inline(origin, ranges, call_site_line_, call_site_file_id_, - inline_nest_level_, std::move(child_inlines_))); + new Module::Inline(origin, ranges, call_site_line_, inline_nest_level_, + std::move(child_inlines_))); inlines_.push_back(std::move(in)); } @@ -680,6 +679,7 @@ class DwarfCUToModule::FuncHandler: public GenericDIEHandler { high_pc_form_(DW_FORM_addr), ranges_form_(DW_FORM_sec_offset), ranges_data_(0), + decl_file_data_(UINT64_MAX), inline_(false), handle_inline_(handle_inline) {} @@ -700,7 +700,9 @@ class DwarfCUToModule::FuncHandler: public GenericDIEHandler { uint64_t low_pc_, high_pc_; // DW_AT_low_pc, DW_AT_high_pc DwarfForm high_pc_form_; // DW_AT_high_pc can be length or address. DwarfForm ranges_form_; // DW_FORM_sec_offset or DW_FORM_rnglistx - uint64_t ranges_data_; // DW_AT_ranges + uint64_t ranges_data_; // DW_AT_ranges + // DW_AT_decl_file, value of UINT64_MAX means undefined. + uint64_t decl_file_data_; bool inline_; vector> child_inlines_; bool handle_inline_; @@ -725,6 +727,9 @@ void DwarfCUToModule::FuncHandler::ProcessAttributeUnsigned( ranges_data_ = data; ranges_form_ = form; break; + case DW_AT_decl_file: + decl_file_data_ = data; + break; default: GenericDIEHandler::ProcessAttributeUnsigned(attr, form, data); break; @@ -852,7 +857,8 @@ void DwarfCUToModule::FuncHandler::Finish() { // Only keep track of DW_TAG_subprogram which have the attributes we are // interested. - if (handle_inline_ && (!empty_range || inline_)) { + if (handle_inline_ && + (!empty_range || inline_ || decl_file_data_ != UINT64_MAX)) { StringView name = name_.empty() ? name_omitted : name_; uint64_t offset = specification_offset_ != 0 ? specification_offset_ : offset_; @@ -860,6 +866,8 @@ void DwarfCUToModule::FuncHandler::Finish() { offset); cu_context_->file_context->module_->inline_origin_map .GetOrCreateInlineOrigin(offset_, name); + if (decl_file_data_ != UINT64_MAX) + cu_context_->inline_origins[decl_file_data_].push_back(offset_); } } @@ -1442,12 +1450,10 @@ void DwarfCUToModule::AssignLinesToFunctions() { } void DwarfCUToModule::AssignFilesToInlines() { - // Assign File* to Inlines inside this CU. - auto assignFile = [this](unique_ptr& in) { - in->call_site_file = files_[in->call_site_file_id]; - }; - for (auto func : cu_context_->functions) { - Module::Inline::InlineDFS(func->inlines, assignFile); + for (auto iter : files_) { + cu_context_->file_context->module_->inline_origin_map + .AssignFilesToInlineOrigins(cu_context_->inline_origins[iter.first], + iter.second); } } diff --git a/src/common/module.cc b/src/common/module.cc index 381dd5d5..3945e2dd 100644 --- a/src/common/module.cc +++ b/src/common/module.cc @@ -97,6 +97,17 @@ void Module::InlineOriginMap::SetReference(uint64_t offset, references_[offset] = specification_offset; } +void Module::InlineOriginMap::AssignFilesToInlineOrigins( + const vector& inline_origin_offsets, + Module::File* file) { + for (uint64_t offset : inline_origin_offsets) + if (references_.find(offset) != references_.end()) { + auto origin = inline_origins_.find(references_[offset]); + if (origin != inline_origins_.end()) + origin->second->file = file; + } +} + Module::Module(const string& name, const string& os, const string& architecture, const string& id, const string& code_id /* = "" */) : @@ -265,18 +276,13 @@ void Module::AssignSourceIds( line_it != func->lines.end(); ++line_it) line_it->file->source_id = 0; } - - // Also mark all files cited by inline callsite by setting each one's source + // Also mark all files cited by inline functions by setting each one's source // id to zero. - auto markInlineFiles = [](unique_ptr& in) { + for (InlineOrigin* origin : inline_origins) // There are some artificial inline functions which don't belong to // any file. Those will have file id -1. - if (in->call_site_file) - in->call_site_file->source_id = 0; - }; - for (auto func : functions_) { - Inline::InlineDFS(func->inlines, markInlineFiles); - } + if (origin->file) + origin->file->source_id = 0; // Finally, assign source ids to those files that have been marked. // We could have just assigned source id numbers while traversing @@ -290,6 +296,15 @@ void Module::AssignSourceIds( } } +static void InlineDFS( + vector>& inlines, + std::function&)> const& forEach) { + for (unique_ptr& in : inlines) { + forEach(in); + InlineDFS(in->child_inlines, forEach); + } +} + void Module::CreateInlineOrigins( set& inline_origins) { // Only add origins that have file and deduplicate origins with same name and @@ -302,7 +317,7 @@ void Module::CreateInlineOrigins( in->origin = *it; }; for (Function* func : functions_) - Module::Inline::InlineDFS(func->inlines, addInlineOrigins); + InlineDFS(func->inlines, addInlineOrigins); int next_id = 0; for (InlineOrigin* origin : inline_origins) { origin->id = next_id++; @@ -366,7 +381,8 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) { } // Write out inline origins. for (InlineOrigin* origin : inline_origins) { - stream << "INLINE_ORIGIN " << origin->id << " " << origin->name << "\n"; + stream << "INLINE_ORIGIN " << origin->id << " " << origin->getFileID() + << " " << origin->name << "\n"; if (!stream.good()) return ReportError(); } @@ -391,12 +407,12 @@ bool Module::Write(std::ostream& stream, SymbolData symbol_data) { auto write_inline = [&](unique_ptr& in) { stream << "INLINE "; stream << in->inline_nest_level << " " << in->call_site_line << " " - << in->getCallSiteFileID() << " " << in->origin->id << hex; + << in->origin->id << hex; for (const Range& r : in->ranges) stream << " " << (r.address - load_address_) << " " << r.size; stream << dec << "\n"; }; - Module::Inline::InlineDFS(func->inlines, write_inline); + InlineDFS(func->inlines, write_inline); if (!stream.good()) return ReportError(); diff --git a/src/common/module.h b/src/common/module.h index bfd344dd..c5e0abfc 100644 --- a/src/common/module.h +++ b/src/common/module.h @@ -38,7 +38,6 @@ #ifndef COMMON_LINUX_MODULE_H__ #define COMMON_LINUX_MODULE_H__ -#include #include #include #include @@ -132,7 +131,7 @@ class Module { }; struct InlineOrigin { - explicit InlineOrigin(StringView name) : id(-1), name(name) {} + explicit InlineOrigin(StringView name): id(-1), name(name), file(nullptr) {} // A unique id for each InlineOrigin object. INLINE records use the id to // refer to its INLINE_ORIGIN record. @@ -140,6 +139,10 @@ class Module { // The inlined function's name. StringView name; + + File* file; + + int getFileID() const { return file ? file->source_id : -1; } }; // A inlined call site. @@ -147,14 +150,11 @@ class Module { Inline(InlineOrigin* origin, const vector& ranges, int call_site_line, - int call_site_file_id, int inline_nest_level, vector> child_inlines) : origin(origin), ranges(ranges), call_site_line(call_site_line), - call_site_file_id(call_site_file_id), - call_site_file(nullptr), inline_nest_level(inline_nest_level), child_inlines(std::move(child_inlines)) {} @@ -165,29 +165,10 @@ class Module { int call_site_line; - // The id is only meanful inside a CU. It's only used for looking up real - // File* after scanning a CU. - int call_site_file_id; - - File* call_site_file; - int inline_nest_level; // A list of inlines which are children of this inline. vector> child_inlines; - - int getCallSiteFileID() const { - return call_site_file ? call_site_file->source_id : -1; - } - - static void InlineDFS( - vector>& inlines, - std::function&)> const& forEach) { - for (std::unique_ptr& in : inlines) { - forEach(in); - InlineDFS(in->child_inlines, forEach); - } - } }; typedef map InlineOriginByOffset; @@ -201,7 +182,9 @@ class Module { // value of its DW_AT_specification or equals to offset if // DW_AT_specification doesn't exist in that DIE. void SetReference(uint64_t offset, uint64_t specification_offset); - + void AssignFilesToInlineOrigins( + const vector& inline_origin_offsets, + File* file); ~InlineOriginMap() { for (const auto& iter : inline_origins_) { delete iter.second; @@ -278,8 +261,10 @@ class Module { }; struct InlineOriginCompare { - bool operator()(const InlineOrigin* lhs, const InlineOrigin* rhs) const { - return lhs->name < rhs->name; + bool operator() (const InlineOrigin* lhs, const InlineOrigin* rhs) const { + if (lhs->getFileID() == rhs->getFileID()) + return lhs->name < rhs->name; + return lhs->getFileID() < rhs->getFileID(); } }; diff --git a/src/google_breakpad/processor/basic_source_line_resolver.h b/src/google_breakpad/processor/basic_source_line_resolver.h index e9459c77..fe76ad4d 100644 --- a/src/google_breakpad/processor/basic_source_line_resolver.h +++ b/src/google_breakpad/processor/basic_source_line_resolver.h @@ -98,28 +98,28 @@ class SymbolParseHelper { char** filename); // out // Parses a |inline_origin_line| declaration. Returns true on success. - // Format: INLINE_ORIGIN . + // Format: INLINE_ORIGIN . // Notice, that this method modifies the input |inline_origin_line| which is - // why it can't be const. On success, and are - // stored in |*origin_id| and |*name|. No allocation is + // why it can't be const. On success, , and are + // stored in |*origin_id|, |*file_id|, and |*name|. No allocation is // done, |*name| simply points inside |inline_origin_line|. static bool ParseInlineOrigin(char* inline_origin_line, // in long* origin_id, // out + long* file_id, // out char** name); // out // Parses a |inline| declaration. Returns true on success. - // Format: INLINE - // [
]+ - // Notice, that this method modifies the input - // |inline| which is why it can't be const. On success, , - // , and are stored in - // |*inline_nest_level|, |*call_site_line|, |*call_site_file_id| and - // |*origin_id|, and all pairs of (
, ) are added into ranges. + // Format: INLINE
+ // .... + // Notice, that this method modifies the input |inline| + // which is why it can't be const. On success, , + // and are stored in |*inline_nest_level|, + // |*call_site_line|, and |*origin_id|, and all pairs of (
, ) + // are added into ranges . static bool ParseInline( char* inline_line, // in long* inline_nest_level, // out long* call_site_line, // out - long* call_site_file_id, // out long* origin_id, // out std::vector>* ranges); // out diff --git a/src/google_breakpad/processor/source_line_resolver_base.h b/src/google_breakpad/processor/source_line_resolver_base.h index ba68798a..2d2e4b35 100644 --- a/src/google_breakpad/processor/source_line_resolver_base.h +++ b/src/google_breakpad/processor/source_line_resolver_base.h @@ -41,7 +41,6 @@ #ifndef GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_BASE_H__ #define GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_BASE_H__ -#include #include #include #include @@ -87,7 +86,7 @@ class SourceLineResolverBase : public SourceLineResolverInterface { virtual bool IsModuleCorrupt(const CodeModule* module); virtual void FillSourceLineInfo( StackFrame* frame, - std::deque>* inlined_frames); + std::vector>* inlined_frames); virtual WindowsFrameInfo* FindWindowsFrameInfo(const StackFrame* frame); virtual CFIFrameInfo* FindCFIFrameInfo(const StackFrame* frame); diff --git a/src/google_breakpad/processor/source_line_resolver_interface.h b/src/google_breakpad/processor/source_line_resolver_interface.h index 2614f65e..7880c922 100644 --- a/src/google_breakpad/processor/source_line_resolver_interface.h +++ b/src/google_breakpad/processor/source_line_resolver_interface.h @@ -34,7 +34,6 @@ #ifndef GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_INTERFACE_H__ #define GOOGLE_BREAKPAD_PROCESSOR_SOURCE_LINE_RESOLVER_INTERFACE_H__ -#include #include #include #include @@ -99,7 +98,7 @@ class SourceLineResolverInterface { // inlined_frames in an order from outermost frame to inner most frame. virtual void FillSourceLineInfo( StackFrame* frame, - std::deque>* inlined_frames) = 0; + std::vector>* inlined_frames) = 0; // If Windows stack walking information is available covering // FRAME's instruction address, return a WindowsFrameInfo structure diff --git a/src/google_breakpad/processor/stack_frame_symbolizer.h b/src/google_breakpad/processor/stack_frame_symbolizer.h index 91ef64b5..0e2c6088 100644 --- a/src/google_breakpad/processor/stack_frame_symbolizer.h +++ b/src/google_breakpad/processor/stack_frame_symbolizer.h @@ -35,7 +35,6 @@ #ifndef GOOGLE_BREAKPAD_PROCESSOR_STACK_FRAME_SYMBOLIZER_H__ #define GOOGLE_BREAKPAD_PROCESSOR_STACK_FRAME_SYMBOLIZER_H__ -#include #include #include #include @@ -83,7 +82,7 @@ class StackFrameSymbolizer { const CodeModules* unloaded_modules, const SystemInfo* system_info, StackFrame* stack_frame, - std::deque>* inlined_frames); + std::vector>* inlined_frames); virtual WindowsFrameInfo* FindWindowsFrameInfo(const StackFrame* frame); diff --git a/src/processor/basic_source_line_resolver.cc b/src/processor/basic_source_line_resolver.cc index 0db0d735..bbaa1331 100644 --- a/src/processor/basic_source_line_resolver.cc +++ b/src/processor/basic_source_line_resolver.cc @@ -50,11 +50,10 @@ #include "processor/tokenize.h" -using std::deque; -using std::make_pair; using std::map; -using std::unique_ptr; using std::vector; +using std::make_pair; +using std::unique_ptr; namespace google_breakpad { @@ -238,43 +237,42 @@ bool BasicSourceLineResolver::Module::LoadMapFromMemory( return true; } -void BasicSourceLineResolver::Module::ConstructInlineFrames( +int BasicSourceLineResolver::Module::ConstructInlineFrames( StackFrame* frame, MemAddr address, const RangeMap>& inlines, - deque>* inlined_frames) const { + vector>* inlined_frames) const { linked_ptr in; MemAddr inline_base; if (!inlines.RetrieveRange(address, &in, &inline_base, nullptr, nullptr)) - return; + return -1; auto origin = inline_origins_.find(in->origin_id); if (origin == inline_origins_.end()) - return; + return -1; - // Update parent frame's source line and source file. - frame->source_line = in->call_site_line; - auto file = files_.find(in->call_site_file_id); - if (file != files_.end()) { - frame->source_file_name = file->second; - } - - // Create a child frame of `frame`. - StackFrame child_frame = StackFrame(*frame); - child_frame.function_name = origin->second->name; + StackFrame new_frame = StackFrame(*frame); + new_frame.function_name = origin->second->name; // Use the starting adress of the inlined range as inlined function base. - child_frame.function_base = child_frame.module->base_address() + inline_base; - child_frame.trust = StackFrame::FRAME_TRUST_INLINE; - ConstructInlineFrames(&child_frame, address, in->child_inlines, - inlined_frames); - // Add child_frame after ConstructInlineFrames so that the innermost frame is - // the first frame inside inlined_frames. - inlined_frames->push_back( - unique_ptr(new StackFrame(child_frame))); + new_frame.function_base = new_frame.module->base_address() + inline_base; + auto it = files_.find(origin->second->source_file_id); + if (it != files_.end()) + new_frame.source_file_name = it->second; + + new_frame.trust = StackFrame::FRAME_TRUST_INLINE; + // Must add frames before calling ConstructInlineFrames to get correct order. + int current_idx = inlined_frames->size(); + inlined_frames->push_back(unique_ptr(new StackFrame(new_frame))); + int source_line = ConstructInlineFrames(&new_frame, address, + in->child_inlines, inlined_frames); + if (source_line != -1) { + (*inlined_frames)[current_idx]->source_line = source_line; + } + return in->call_site_line; } void BasicSourceLineResolver::Module::LookupAddress( StackFrame* frame, - deque>* inlined_frames) const { + vector>* inlined_frames) const { MemAddr address = frame->instruction - frame->module->base_address(); // First, look for a FUNC record that covers address. Use @@ -308,13 +306,10 @@ void BasicSourceLineResolver::Module::LookupAddress( // Check if this is inlined function call. if (inlined_frames) { - int source_line = frame->source_line; - string source_file_name = frame->source_file_name; - ConstructInlineFrames(frame, address, func->inlines, inlined_frames); - if (!inlined_frames->empty()) { - // Update the inner most frame's source line and source file name. - inlined_frames->front()->source_line = source_line; - inlined_frames->front()->source_file_name = source_file_name; + int source_line = + ConstructInlineFrames(frame, address, func->inlines, inlined_frames); + if (source_line != -1) { + frame->source_line = source_line; } } } else if (public_symbols_.Retrieve(address, @@ -421,10 +416,12 @@ bool BasicSourceLineResolver::Module::ParseFile(char* file_line) { bool BasicSourceLineResolver::Module::ParseInlineOrigin( char* inline_origin_line) { long origin_id; + long source_file_id; char* origin_name; if (SymbolParseHelper::ParseInlineOrigin(inline_origin_line, &origin_id, - &origin_name)) { - inline_origins_.insert(make_pair(origin_id, new InlineOrigin(origin_name))); + &source_file_id, &origin_name)) { + inline_origins_.insert( + make_pair(origin_id, new InlineOrigin(source_file_id, origin_name))); return true; } return false; @@ -434,14 +431,12 @@ linked_ptr BasicSourceLineResolver::Module::ParseInline(char* inline_line) { long inline_nest_level; long call_site_line; - long call_site_file_id; long origin_id; vector> ranges; if (SymbolParseHelper::ParseInline(inline_line, &inline_nest_level, - &call_site_line, &call_site_file_id, - &origin_id, &ranges)) { - return linked_ptr(new Inline(inline_nest_level, call_site_line, - call_site_file_id, origin_id, ranges)); + &call_site_line, &origin_id, &ranges)) { + return linked_ptr( + new Inline(inline_nest_level, call_site_line, origin_id, ranges)); } return linked_ptr(); } @@ -641,12 +636,13 @@ bool SymbolParseHelper::ParseFile(char* file_line, long* index, // static bool SymbolParseHelper::ParseInlineOrigin(char* inline_origin_line, long* origin_id, + long* file_id, char** name) { - // INLINE_ORIGIN + // INLINE_ORIGIN assert(strncmp(inline_origin_line, "INLINE_ORIGIN ", 14) == 0); inline_origin_line += 14; // skip prefix vector tokens; - if (!Tokenize(inline_origin_line, kWhitespace, 2, &tokens)) { + if (!Tokenize(inline_origin_line, kWhitespace, 3, &tokens)) { return false; } @@ -657,7 +653,15 @@ bool SymbolParseHelper::ParseInlineOrigin(char* inline_origin_line, return false; } - *name = tokens[1]; + *file_id = strtol(tokens[1], &after_number, 10); + // If the file id is -1, it might be an artificial function that doesn't have + // file id. So, we consider -1 as a valid special case. + if (!IsValidAfterNumber(after_number) || + *file_id < -1 | *origin_id == std::numeric_limits::max()) { + return false; + } + + *name = tokens[2]; if (!*name) { return false; } @@ -670,19 +674,18 @@ bool SymbolParseHelper::ParseInline( char* inline_line, long* inline_nest_level, long* call_site_line, - long* call_site_file_id, long* origin_id, vector>* ranges) { - // INLINE - // [
]+ + // INLINE
+ // ... assert(strncmp(inline_line, "INLINE ", 7) == 0); inline_line += 7; // skip prefix vector tokens; Tokenize(inline_line, kWhitespace, std::numeric_limits::max(), &tokens); - // The length of the vector should be at least 6 and an even number. - if (tokens.size() < 6 || tokens.size() % 2 != 0) + // The length of the vector should be at least 5 and an odd number. + if (tokens.size() < 5 && tokens.size() % 2 == 0) return false; char* after_number; @@ -698,21 +701,13 @@ bool SymbolParseHelper::ParseInline( return false; } - *call_site_file_id = strtol(tokens[2], &after_number, 10); - // If the file id is -1, it might be an artificial function that doesn't have - // file id. So, we consider -1 as a valid special case. - if (!IsValidAfterNumber(after_number) || *call_site_file_id < -1 || - *call_site_file_id == std::numeric_limits::max()) { - return false; - } - - *origin_id = strtol(tokens[3], &after_number, 10); + *origin_id = strtol(tokens[2], &after_number, 10); if (!IsValidAfterNumber(after_number) || *origin_id < 0 || *origin_id == std::numeric_limits::max()) { return false; } - for (size_t i = 4; i < tokens.size();) { + for (size_t i = 3; i < tokens.size();) { MemAddr address = strtoull(tokens[i++], &after_number, 16); if (!IsValidAfterNumber(after_number) || address == std::numeric_limits::max()) { diff --git a/src/processor/basic_source_line_resolver_types.h b/src/processor/basic_source_line_resolver_types.h index 60c06756..482176f4 100644 --- a/src/processor/basic_source_line_resolver_types.h +++ b/src/processor/basic_source_line_resolver_types.h @@ -37,7 +37,6 @@ #ifndef PROCESSOR_BASIC_SOURCE_LINE_RESOLVER_TYPES_H__ #define PROCESSOR_BASIC_SOURCE_LINE_RESOLVER_TYPES_H__ -#include #include #include @@ -109,18 +108,15 @@ class BasicSourceLineResolver::Module : public SourceLineResolverBase::Module { // with the result. virtual void LookupAddress( StackFrame* frame, - std::deque>* inlined_frame) const; + std::vector>* inlined_frame) const; - // Construct inlined frames for frame. Return true on success. - // If successfully construct inlined frames for `frame`, `inline_frames` will - // be filled with lined frames and frame->source_file_name and - // frame->source_line will be update to represents the outermost frame. - // If failed, `inline_frames` will be empty and frame remains unchanged. - virtual void ConstructInlineFrames( + // Construct inlined frame for frame and return inlined function call site + // source line. If failed to construct inlined frame, return -1. + virtual int ConstructInlineFrames( StackFrame* frame, MemAddr address, const RangeMap>& inlines, - std::deque>* inline_frames) const; + std::vector>* inline_frames) const; // If Windows stack walking information is available covering ADDRESS, // return a WindowsFrameInfo structure describing it. If the information diff --git a/src/processor/basic_source_line_resolver_unittest.cc b/src/processor/basic_source_line_resolver_unittest.cc index 8db56217..914f0963 100644 --- a/src/processor/basic_source_line_resolver_unittest.cc +++ b/src/processor/basic_source_line_resolver_unittest.cc @@ -421,40 +421,40 @@ TEST_F(TestBasicSourceLineResolver, TestLoadAndResolveInlines) { "linux_inline.sym")); ASSERT_TRUE(resolver.HasModule(&module)); StackFrame frame; - std::deque> inlined_frames; + std::vector> inlined_frames; frame.instruction = 0x161b6; frame.module = &module; // main frame. resolver.FillSourceLineInfo(&frame, &inlined_frames); ASSERT_EQ(frame.function_name, "main"); ASSERT_EQ(frame.function_base, 0x15b30U); - ASSERT_EQ(frame.source_file_name, "a.cpp"); + ASSERT_EQ(frame.source_file_name, "linux_inline.cpp"); ASSERT_EQ(frame.source_line, 42); ASSERT_EQ(frame.source_line_base, 0x161b6U); ASSERT_EQ(inlined_frames.size(), 3UL); // Inlined frames inside main frame. - ASSERT_EQ(inlined_frames[2]->function_name, "foo()"); - ASSERT_EQ(inlined_frames[2]->function_base, 0x15b45U); - ASSERT_EQ(inlined_frames[2]->source_file_name, "b.cpp"); - ASSERT_EQ(inlined_frames[2]->source_line, 39); - ASSERT_EQ(inlined_frames[2]->source_line_base, 0x161b6U); - ASSERT_EQ(inlined_frames[2]->trust, StackFrame::FRAME_TRUST_INLINE); + ASSERT_EQ(inlined_frames[0]->function_name, "foo()"); + ASSERT_EQ(inlined_frames[0]->function_base, 0x15b45U); + ASSERT_EQ(inlined_frames[0]->source_file_name, "linux_inline.cpp"); + ASSERT_EQ(inlined_frames[0]->source_line, 39); + ASSERT_EQ(inlined_frames[0]->source_line_base, 0x161b6U); + ASSERT_EQ(inlined_frames[0]->trust, StackFrame::FRAME_TRUST_INLINE); ASSERT_EQ(inlined_frames[1]->function_name, "bar()"); ASSERT_EQ(inlined_frames[1]->function_base, 0x15b72U); - ASSERT_EQ(inlined_frames[1]->source_file_name, "c.cpp"); + ASSERT_EQ(inlined_frames[1]->source_file_name, "linux_inline.cpp"); ASSERT_EQ(inlined_frames[1]->source_line, 32); ASSERT_EQ(inlined_frames[1]->source_line_base, 0x161b6U); ASSERT_EQ(inlined_frames[1]->trust, StackFrame::FRAME_TRUST_INLINE); - ASSERT_EQ(inlined_frames[0]->function_name, "func()"); - ASSERT_EQ(inlined_frames[0]->function_base, 0x15b83U); - ASSERT_EQ(inlined_frames[0]->source_file_name, "linux_inline.cpp"); - ASSERT_EQ(inlined_frames[0]->source_line, 27); - ASSERT_EQ(inlined_frames[0]->source_line_base, 0x161b6U); - ASSERT_EQ(inlined_frames[0]->trust, StackFrame::FRAME_TRUST_INLINE); + ASSERT_EQ(inlined_frames[2]->function_name, "func()"); + ASSERT_EQ(inlined_frames[2]->function_base, 0x15b83U); + ASSERT_EQ(inlined_frames[2]->source_file_name, "linux_inline.cpp"); + ASSERT_EQ(inlined_frames[2]->source_line, 27); + ASSERT_EQ(inlined_frames[2]->source_line_base, 0x161b6U); + ASSERT_EQ(inlined_frames[2]->trust, StackFrame::FRAME_TRUST_INLINE); } // Test parsing of valid FILE lines. The format is: @@ -781,15 +781,25 @@ TEST(SymbolParseHelper, ParsePublicSymbolInvalid) { } // Test parsing of valid INLINE_ORIGIN lines. The format is: -// INLINE_ORIGIN +// INLINE_ORIGIN TEST(SymbolParseHelper, ParseInlineOriginValid) { long origin_id; + long file_id; char* name; - char kTestLine[] = "INLINE_ORIGIN 1 function name"; - ASSERT_TRUE( - SymbolParseHelper::ParseInlineOrigin(kTestLine, &origin_id, &name)); + char kTestLine[] = "INLINE_ORIGIN 1 1 function name"; + ASSERT_TRUE(SymbolParseHelper::ParseInlineOrigin(kTestLine, &origin_id, + &file_id, &name)); EXPECT_EQ(1, origin_id); + EXPECT_EQ(1, file_id); + EXPECT_EQ("function name", string(name)); + + // -1 is a file id, which is used when the function is artifical. + char kTestLine1[] = "INLINE_ORIGIN 0 -1 function name"; + ASSERT_TRUE(SymbolParseHelper::ParseInlineOrigin(kTestLine1, &origin_id, + &file_id, &name)); + EXPECT_EQ(0, origin_id); + EXPECT_EQ(-1, file_id); EXPECT_EQ("function name", string(name)); } @@ -797,27 +807,28 @@ TEST(SymbolParseHelper, ParseInlineOriginValid) { // INLINE_ORIGIN TEST(SymbolParseHelper, ParseInlineOriginInvalid) { long origin_id; + long file_id; char* name; // Test missing function name. - char kTestLine[] = "INLINE_ORIGIN 1"; - ASSERT_FALSE( - SymbolParseHelper::ParseInlineOrigin(kTestLine, &origin_id, &name)); + char kTestLine[] = "INLINE_ORIGIN 1 1"; + ASSERT_FALSE(SymbolParseHelper::ParseInlineOrigin(kTestLine, &origin_id, + &file_id, &name)); // Test bad origin id. - char kTestLine1[] = "INLINE_ORIGIN x1 function name"; - ASSERT_FALSE( - SymbolParseHelper::ParseInlineOrigin(kTestLine1, &origin_id, &name)); + char kTestLine1[] = "INLINE_ORIGIN x1 1 function name"; + ASSERT_FALSE(SymbolParseHelper::ParseInlineOrigin(kTestLine1, &origin_id, + &file_id, &name)); // Test large origin id. - char kTestLine2[] = "INLINE_ORIGIN 123123123123123123123123 function name"; - ASSERT_FALSE( - SymbolParseHelper::ParseInlineOrigin(kTestLine2, &origin_id, &name)); + char kTestLine2[] = "INLINE_ORIGIN 123123123123123123123123 1 function name"; + ASSERT_FALSE(SymbolParseHelper::ParseInlineOrigin(kTestLine2, &origin_id, + &file_id, &name)); // Test negative origin id. - char kTestLine3[] = "INLINE_ORIGIN -1 function name"; - ASSERT_FALSE( - SymbolParseHelper::ParseInlineOrigin(kTestLine3, &origin_id, &name)); + char kTestLine3[] = "INLINE_ORIGIN -1 1 function name"; + ASSERT_FALSE(SymbolParseHelper::ParseInlineOrigin(kTestLine3, &origin_id, + &file_id, &name)); } // Test parsing of valid INLINE lines. The format is: @@ -825,31 +836,26 @@ TEST(SymbolParseHelper, ParseInlineOriginInvalid) { TEST(SymbolParseHelper, ParseInlineValid) { long inline_nest_level; long call_site_line; - long call_site_file_id; long origin_id; std::vector> ranges; - char kTestLine[] = "INLINE 0 1 2 3 4 5"; + char kTestLine[] = "INLINE 0 1 2 3 4"; ASSERT_TRUE(SymbolParseHelper::ParseInline( - kTestLine, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine, &inline_nest_level, &call_site_line, &origin_id, &ranges)); EXPECT_EQ(0, inline_nest_level); EXPECT_EQ(1, call_site_line); - EXPECT_EQ(2, call_site_file_id); - EXPECT_EQ(3, origin_id); - EXPECT_EQ(0x4ULL, ranges[0].first); - EXPECT_EQ(0x5ULL, ranges[0].second); + EXPECT_EQ(2, origin_id); + EXPECT_EQ(0x3ULL, ranges[0].first); + EXPECT_EQ(0x4ULL, ranges[0].second); ranges.clear(); // Test hex and discontinuous ranges. - char kTestLine1[] = "INLINE 0 1 2 3 a b 1a 1b"; + char kTestLine1[] = "INLINE 0 1 2 a b 1a 1b"; ASSERT_TRUE(SymbolParseHelper::ParseInline( - kTestLine1, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine1, &inline_nest_level, &call_site_line, &origin_id, &ranges)); EXPECT_EQ(0, inline_nest_level); EXPECT_EQ(1, call_site_line); - EXPECT_EQ(2, call_site_file_id); - EXPECT_EQ(3, origin_id); + EXPECT_EQ(2, origin_id); EXPECT_EQ(0xaULL, ranges[0].first); EXPECT_EQ(0xbULL, ranges[0].second); EXPECT_EQ(0x1aULL, ranges[1].first); @@ -861,39 +867,33 @@ TEST(SymbolParseHelper, ParseInlineValid) { TEST(SymbolParseHelper, ParseInlineInvalid) { long inline_nest_level; long call_site_line; - long call_site_file_id; long origin_id; std::vector> ranges; // Test negative inline_nest_level. - char kTestLine[] = "INLINE -1 1 2 3 4 5"; + char kTestLine[] = "INLINE -1 1 2 3 4"; ASSERT_FALSE(SymbolParseHelper::ParseInline( - kTestLine, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine, &inline_nest_level, &call_site_line, &origin_id, &ranges)); // Test negative call_site_line. - char kTestLine1[] = "INLINE 0 -1 2 3 4 5"; + char kTestLine1[] = "INLINE 0 -1 2 3 4"; ASSERT_FALSE(SymbolParseHelper::ParseInline( - kTestLine1, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine1, &inline_nest_level, &call_site_line, &origin_id, &ranges)); // Test negative origin_id. - char kTestLine2[] = "INLINE 0 1 2 -3 4 5"; + char kTestLine2[] = "INLINE 0 1 -2 3 4"; ASSERT_FALSE(SymbolParseHelper::ParseInline( - kTestLine2, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine2, &inline_nest_level, &call_site_line, &origin_id, &ranges)); // Test missing ranges. - char kTestLine3[] = "INLINE 0 1 2 -2"; + char kTestLine3[] = "INLINE 0 1 -2"; ASSERT_FALSE(SymbolParseHelper::ParseInline( - kTestLine3, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine3, &inline_nest_level, &call_site_line, &origin_id, &ranges)); // Test missing size for range. - char kTestLine4[] = "INLINE 0 1 -2 3 4"; + char kTestLine4[] = "INLINE 0 1 -2 3"; ASSERT_FALSE(SymbolParseHelper::ParseInline( - kTestLine4, &inline_nest_level, &call_site_line, &call_site_file_id, - &origin_id, &ranges)); + kTestLine4, &inline_nest_level, &call_site_line, &origin_id, &ranges)); } } // namespace diff --git a/src/processor/fast_source_line_resolver.cc b/src/processor/fast_source_line_resolver.cc index 31786460..029f21f4 100644 --- a/src/processor/fast_source_line_resolver.cc +++ b/src/processor/fast_source_line_resolver.cc @@ -64,7 +64,7 @@ bool FastSourceLineResolver::ShouldDeleteMemoryBufferAfterLoadModule() { void FastSourceLineResolver::Module::LookupAddress( StackFrame* frame, - std::deque>* inlined_frames) const { + vector>* inlined_frames) const { MemAddr address = frame->instruction - frame->module->base_address(); // First, look for a FUNC record that covers address. Use diff --git a/src/processor/fast_source_line_resolver_types.h b/src/processor/fast_source_line_resolver_types.h index 2d1bcfcb..aa02fc11 100644 --- a/src/processor/fast_source_line_resolver_types.h +++ b/src/processor/fast_source_line_resolver_types.h @@ -119,7 +119,7 @@ class FastSourceLineResolver::Module: public SourceLineResolverBase::Module { // with the result. virtual void LookupAddress( StackFrame* frame, - std::deque>* inlined_frames) const; + std::vector>* inlined_frames) const; // Loads a map from the given buffer in char* type. virtual bool LoadMapFromMemory(char* memory_buffer, diff --git a/src/processor/source_line_resolver_base.cc b/src/processor/source_line_resolver_base.cc index cea1a46d..16c82224 100644 --- a/src/processor/source_line_resolver_base.cc +++ b/src/processor/source_line_resolver_base.cc @@ -297,7 +297,7 @@ bool SourceLineResolverBase::IsModuleCorrupt(const CodeModule* module) { void SourceLineResolverBase::FillSourceLineInfo( StackFrame* frame, - std::deque>* inlined_frames) { + std::vector>* inlined_frames) { if (frame->module) { ModuleMap::const_iterator it = modules_->find(frame->module->code_file()); if (it != modules_->end()) { diff --git a/src/processor/source_line_resolver_base_types.h b/src/processor/source_line_resolver_base_types.h index 44968bf6..3e3afd0e 100644 --- a/src/processor/source_line_resolver_base_types.h +++ b/src/processor/source_line_resolver_base_types.h @@ -40,7 +40,6 @@ #include -#include #include #include #include @@ -71,7 +70,10 @@ class SourceLineResolverBase::AutoFileCloser { }; struct SourceLineResolverBase::InlineOrigin { - explicit InlineOrigin(const string& name) : name(name) {} + InlineOrigin(int32_t source_file_id, const string& name) + : source_file_id(source_file_id), name(name) {} + + int32_t source_file_id; string name; }; @@ -80,18 +82,15 @@ struct SourceLineResolverBase::Inline { using InlineRanges = std::vector>; Inline(int32_t inline_nest_level, int32_t call_site_line, - int32_t call_site_file_id, int32_t origin_id, InlineRanges inline_ranges) : inline_nest_level(inline_nest_level), call_site_line(call_site_line), - call_site_file_id(call_site_file_id), origin_id(origin_id), inline_ranges(inline_ranges) {} int32_t inline_nest_level; int32_t call_site_line; - int32_t call_site_file_id; int32_t origin_id; InlineRanges inline_ranges; RangeMap> child_inlines; @@ -175,7 +174,7 @@ class SourceLineResolverBase::Module { // with the result. virtual void LookupAddress( StackFrame* frame, - std::deque>* inlined_frames) const = 0; + std::vector>* inlined_frames) const = 0; // If Windows stack walking information is available covering ADDRESS, // return a WindowsFrameInfo structure describing it. If the information diff --git a/src/processor/stack_frame_symbolizer.cc b/src/processor/stack_frame_symbolizer.cc index a460bc9e..6490ca90 100644 --- a/src/processor/stack_frame_symbolizer.cc +++ b/src/processor/stack_frame_symbolizer.cc @@ -58,7 +58,7 @@ StackFrameSymbolizer::SymbolizerResult StackFrameSymbolizer::FillSourceLineInfo( const CodeModules* unloaded_modules, const SystemInfo* system_info, StackFrame* frame, - std::deque>* inlined_frames) { + std::vector>* inlined_frames) { assert(frame); const CodeModule* module = NULL; diff --git a/src/processor/stackwalk_common.cc b/src/processor/stackwalk_common.cc index c1e17084..856a6a66 100644 --- a/src/processor/stackwalk_common.cc +++ b/src/processor/stackwalk_common.cc @@ -218,7 +218,7 @@ static void PrintStackContents(const string& indent, modules->GetModuleForAddress(pointee_frame.instruction); // Try to look up the function name. - std::deque> inlined_frames; + vector> inlined_frames; if (pointee_frame.module) resolver->FillSourceLineInfo(&pointee_frame, &inlined_frames); diff --git a/src/processor/stackwalker.cc b/src/processor/stackwalker.cc index cb383119..d4897d4c 100644 --- a/src/processor/stackwalker.cc +++ b/src/processor/stackwalker.cc @@ -138,7 +138,7 @@ bool Stackwalker::Walk( // frame_pointer fields. The frame structure comes from either the // context frame (above) or a caller frame (below). - std::deque> inlined_frames; + vector> inlined_frames; // Resolve the module information, if a module map was provided. StackFrameSymbolizer::SymbolizerResult symbolizer_result = frame_symbolizer_->FillSourceLineInfo(modules_, unloaded_modules_, @@ -174,10 +174,10 @@ bool Stackwalker::Walk( default: break; } - // Add all nested inlined frames belonging to this frame from left to right. + // Add all nested inlined frames belonging to this frame in reverse order. while (!inlined_frames.empty()) { - stack->frames_.push_back(inlined_frames.front().release()); - inlined_frames.pop_front(); + stack->frames_.push_back(inlined_frames.back().release()); + inlined_frames.pop_back(); } // Add the frame to the call stack. Relinquish the ownership claim // over the frame, because the stack now owns it. diff --git a/src/processor/testdata/symbols/linux_inline/BBA6FA10B8AAB33D00000000000000000/linux_inline.sym b/src/processor/testdata/symbols/linux_inline/BBA6FA10B8AAB33D00000000000000000/linux_inline.sym index db797142..775640f0 100644 --- a/src/processor/testdata/symbols/linux_inline/BBA6FA10B8AAB33D00000000000000000/linux_inline.sym +++ b/src/processor/testdata/symbols/linux_inline/BBA6FA10B8AAB33D00000000000000000/linux_inline.sym @@ -1,16 +1,13 @@ MODULE Linux x86_64 BBA6FA10B8AAB33D00000000000000000 linux_inline INFO CODE_ID 10FAA6BBAAB83DB3 FILE 0 linux_inline.cpp -FILE 1 a.cpp -FILE 2 b.cpp -FILE 3 c.cpp -INLINE_ORIGIN 0 bar() -INLINE_ORIGIN 1 foo() -INLINE_ORIGIN 2 func() +INLINE_ORIGIN 0 0 bar() +INLINE_ORIGIN 1 0 foo() +INLINE_ORIGIN 2 0 func() FUNC 15b30 6cf 0 main -INLINE 0 42 1 1 15b45 6b1 -INLINE 1 39 2 0 15b72 684 -INLINE 2 32 3 2 15b83 673 +INLINE 0 42 1 15b45 6b1 +INLINE 1 39 0 15b72 684 +INLINE 2 32 2 15b83 673 15b30 15 41 0 15b45 11 36 0 15b56 a 37 0