From 7bdeef54a24d3417300f251af891df962b638b9b Mon Sep 17 00:00:00 2001 From: Rene Gollent Date: Fri, 9 Dec 2016 22:57:46 -0500 Subject: [PATCH] Debugger: Rework parts of report generation. Team: - Adjust report generation event to include a final status code for listeners. CliContext,TeamWindow,ReportUserinterface: - Use aforementioned status code to indicate whether report generation succeeded or failed. DebugReportGenerator: - Notify listeners if report generation fails. This may have previously been responsible for some bug reports where it was indicated that the debugger hung without exiting after being asked to save a report from a crashed app. - When dumping disassembly, retrieve it directly if necessary rather than requesting it via the user interface listener. This also fixes the quirk that requesting to save a crash report while looking at the source code of a function would trigger switching it to disassembly visually. - When walking the list of threads to dump, acquire references to all of them before starting. Otherwise, it was potentially possible for a running but not crashed thread to exit while we were generating the report, leaving us with a pointer to a deleted thread. This was most likely the cause of one of the crashes reported in #13082. - When receiving the notification that source code state has changed, clear the waiting function. Otherwise, it was potentially possible for us to get other state change notifications, leading to the data semaphore being released too often. This would then cause later potential waits such as the stack frame memory dump to not actually wait when they should, potentially leading them to dereference objects that weren't yet ready. This fixes another of the crashes in #13802. --- headers/private/debugger/model/Team.h | 8 +- .../user_interface/cli/CliContext.cpp | 11 +- .../gui/team_window/TeamWindow.cpp | 12 +- .../report/ReportUserInterface.cpp | 7 +- .../controllers/DebugReportGenerator.cpp | 138 +++++++++--------- .../controllers/DebugReportGenerator.h | 5 +- src/kits/debugger/model/Team.cpp | 9 +- 7 files changed, 106 insertions(+), 84 deletions(-) diff --git a/headers/private/debugger/model/Team.h b/headers/private/debugger/model/Team.h index 1d83377d48..0af21ee318 100644 --- a/headers/private/debugger/model/Team.h +++ b/headers/private/debugger/model/Team.h @@ -265,7 +265,7 @@ public: // debug report related service methods void NotifyDebugReportChanged( - const char* reportPath); + const char* reportPath, status_t result); // core file related service methods void NotifyCoreFileChanged( @@ -434,13 +434,17 @@ protected: class Team::DebugReportEvent : public Event { public: DebugReportEvent(uint32 type, Team* team, - const char* reportPath); + const char* reportPath, + status_t finalStatus); const char* GetReportPath() const { return fReportPath; } + status_t GetFinalStatus() const { return fFinalStatus; } protected: const char* fReportPath; + status_t fFinalStatus; }; + class Team::CoreFileChangedEvent : public Event { public: CoreFileChangedEvent(uint32 type, Team* team, diff --git a/src/apps/debugger/user_interface/cli/CliContext.cpp b/src/apps/debugger/user_interface/cli/CliContext.cpp index 00946f1ef2..fa287fc03f 100644 --- a/src/apps/debugger/user_interface/cli/CliContext.cpp +++ b/src/apps/debugger/user_interface/cli/CliContext.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2012-2015, Rene Gollent, rene@gollent.com. + * Copyright 2012-2016, Rene Gollent, rene@gollent.com. * Copyright 2012, Ingo Weinhold, ingo_weinhold@gmx.de. * Distributed under the terms of the MIT License. */ @@ -508,8 +508,13 @@ CliContext::ExpressionEvaluated(ExpressionInfo* info, status_t result, void CliContext::DebugReportChanged(const Team::DebugReportEvent& event) { - printf("Successfully saved debug report to %s\n", - event.GetReportPath()); + if (event.GetFinalStatus() == B_OK) { + printf("Successfully saved debug report to %s\n", + event.GetReportPath()); + } else { + fprintf(stderr, "Failed to write debug report: %s\n", strerror( + event.GetFinalStatus())); + } _QueueEvent(new(std::nothrow) Event(EVENT_DEBUG_REPORT_CHANGED)); _SignalInputLoop(EVENT_DEBUG_REPORT_CHANGED); diff --git a/src/apps/debugger/user_interface/gui/team_window/TeamWindow.cpp b/src/apps/debugger/user_interface/gui/team_window/TeamWindow.cpp index 486307ed31..ba5480d157 100644 --- a/src/apps/debugger/user_interface/gui/team_window/TeamWindow.cpp +++ b/src/apps/debugger/user_interface/gui/team_window/TeamWindow.cpp @@ -305,9 +305,16 @@ TeamWindow::MessageReceived(BMessage* message) } case MSG_DEBUG_REPORT_SAVED: { + status_t finalStatus = message->GetInt32("status", B_OK); BString data; - data.SetToFormat("Debug report successfully saved to '%s'", - message->FindString("path")); + if (finalStatus == B_OK) { + data.SetToFormat("Debug report successfully saved to '%s'", + message->FindString("path")); + } else { + data.SetToFormat("Failed to save debug report: '%s'", + strerror(finalStatus)); + } + BAlert *alert = new(std::nothrow) BAlert("Report saved", data.String(), "Close"); if (alert == NULL) @@ -964,6 +971,7 @@ TeamWindow::DebugReportChanged(const Team::DebugReportEvent& event) { BMessage message(MSG_DEBUG_REPORT_SAVED); message.AddString("path", event.GetReportPath()); + message.AddInt32("status", event.GetFinalStatus()); PostMessage(&message); } diff --git a/src/apps/debugger/user_interface/report/ReportUserInterface.cpp b/src/apps/debugger/user_interface/report/ReportUserInterface.cpp index d0dfda0162..fb5f1f3286 100644 --- a/src/apps/debugger/user_interface/report/ReportUserInterface.cpp +++ b/src/apps/debugger/user_interface/report/ReportUserInterface.cpp @@ -237,6 +237,11 @@ ReportUserInterface::ThreadStateChanged(const Team::ThreadEvent& event) void ReportUserInterface::DebugReportChanged(const Team::DebugReportEvent& event) { - printf("Debug report saved to %s\n", event.GetReportPath()); + if (event.GetFinalStatus() == B_OK) + printf("Debug report saved to %s\n", event.GetReportPath()); + else { + fprintf(stderr, "Failed to write debug report: %s\n", strerror( + event.GetFinalStatus())); + } release_sem(fReportSemaphore); } diff --git a/src/kits/debugger/controllers/DebugReportGenerator.cpp b/src/kits/debugger/controllers/DebugReportGenerator.cpp index 0d6ef1023b..72a459d5aa 100644 --- a/src/kits/debugger/controllers/DebugReportGenerator.cpp +++ b/src/kits/debugger/controllers/DebugReportGenerator.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2012-2015, Rene Gollent, rene@gollent.com. + * Copyright 2012-2016, Rene Gollent, rene@gollent.com. * Distributed under the terms of the MIT License. */ @@ -32,6 +32,7 @@ #include "StringUtils.h" #include "SystemInfo.h" #include "Team.h" +#include "TeamDebugInfo.h" #include "Thread.h" #include "Type.h" #include "UiUtils.h" @@ -65,7 +66,6 @@ DebugReportGenerator::DebugReportGenerator(::Team* team, fCurrentBlock(NULL), fBlockRetrievalStatus(B_OK), fTraceWaitingThread(NULL), - fSourceWaitForDisassembly(false), fSourceWaitingFunction(NULL) { fTeam->AddListener(this); @@ -128,9 +128,9 @@ DebugReportGenerator::Create(::Team* team, UserInterfaceListener* listener, status_t -DebugReportGenerator::_GenerateReport(const entry_ref& outputPath) +DebugReportGenerator::_GenerateReport(const char* outputPath) { - BFile file(&outputPath, B_WRITE_ONLY | B_CREATE_FILE | B_ERASE_FILE); + BFile file(outputPath, B_WRITE_ONLY | B_CREATE_FILE | B_ERASE_FILE); status_t result = file.InitCheck(); if (result != B_OK) return result; @@ -155,10 +155,8 @@ DebugReportGenerator::_GenerateReport(const entry_ref& outputPath) if (result != B_OK) return result; - BPath path(&outputPath); - AutoLocker< ::Team> teamLocker(fTeam); - fTeam->NotifyDebugReportChanged(path.Path()); + fTeam->NotifyDebugReportChanged(outputPath, B_OK); return B_OK; } @@ -171,8 +169,12 @@ DebugReportGenerator::MessageReceived(BMessage* message) case MSG_GENERATE_DEBUG_REPORT: { entry_ref ref; - if (message->FindRef("target", &ref) == B_OK) - _GenerateReport(ref); + if (message->FindRef("target", &ref) == B_OK) { + BPath path(&ref); + status_t error = _GenerateReport(path.Path()); + if (error != B_OK) + fTeam->NotifyDebugReportChanged(path.Path(), error); + } break; } @@ -223,17 +225,15 @@ DebugReportGenerator::FunctionSourceCodeChanged(Function* function) { AutoLocker< ::Team> teamLocker(fTeam); if (function == fSourceWaitingFunction) { - function_source_state state; - if (fSourceWaitForDisassembly) - state = function->FirstInstance()->SourceCodeState(); - else - state = function->SourceCodeState(); + function_source_state state = function->SourceCodeState(); switch (state) { case FUNCTION_SOURCE_LOADED: case FUNCTION_SOURCE_SUPPRESSED: case FUNCTION_SOURCE_UNAVAILABLE: { + fSourceWaitingFunction->RemoveListener(this); + fSourceWaitingFunction = NULL; release_sem(fTeamDataSem); // fall through } @@ -455,8 +455,10 @@ DebugReportGenerator::_DumpRunningThreads(BFile& _output) for (ThreadList::ConstIterator it = fTeam->Threads().GetIterator(); (thread = it.Next());) { threads.AddItem(thread); + thread->AcquireReference(); } + status_t error = B_OK; threads.SortItems(&_CompareThreads); for (int32 i = 0; (thread = threads.ItemAt(i)) != NULL; i++) { try { @@ -478,19 +480,21 @@ DebugReportGenerator::_DumpRunningThreads(BFile& _output) // we need to release our lock on the team here // since we might need to block and wait // on the stack trace. - BReference< ::Thread> threadRef(thread); locker.Unlock(); - status_t error = _DumpDebuggedThreadInfo(_output, thread); - if (error != B_OK) - return error; + error = _DumpDebuggedThreadInfo(_output, thread); locker.Lock(); + if (error != B_OK) + break; } } catch (...) { - return B_NO_MEMORY; + error = B_NO_MEMORY; } } - return B_OK; + for (int32 i = 0; (thread = threads.ItemAt(i)) != NULL; i++) + thread->ReleaseReference(); + + return error; } @@ -516,7 +520,7 @@ DebugReportGenerator::_DumpDebuggedThreadInfo(BFile& _output, } while (error == B_INTERRUPTED); if (error != B_OK) - break; + return error; locker.Lock(); } @@ -544,7 +548,6 @@ DebugReportGenerator::_DumpDebuggedThreadInfo(BFile& _output, && functionInstance->SourceCodeState() == FUNCTION_SOURCE_NOT_LOADED) { fSourceWaitingFunction = function; - fSourceWaitForDisassembly = false; fSourceWaitingFunction->AddListener(this); fListener->FunctionSourceCodeRequested(functionInstance); @@ -555,11 +558,9 @@ DebugReportGenerator::_DumpDebuggedThreadInfo(BFile& _output, } while (error == B_INTERRUPTED); if (error != B_OK) - break; + return error; locker.Lock(); - - fSourceWaitingFunction->RemoveListener(this); } } @@ -589,7 +590,8 @@ DebugReportGenerator::_DumpDebuggedThreadInfo(BFile& _output, // only dump the topmost frame if (i == 0) { locker.Unlock(); - error = _DumpFunctionDisassembly(_output, frame->InstructionPointer()); + error = _DumpFunctionDisassembly(_output, + frame->InstructionPointer()); if (error != B_OK) return error; error = _DumpStackFrameMemory(_output, thread->GetCpuState(), @@ -658,55 +660,50 @@ DebugReportGenerator::_DumpFunctionDisassembly(BFile& _output, AutoLocker< ::Team> teamLocker(fTeam); BString data; FunctionInstance* instance = NULL; - Statement* statement = NULL; - status_t error = fTeam->GetStatementAtAddress(instructionPointer, instance, - statement); - if (error != B_OK) { - data.SetToFormat("Unable to retrieve disassembly for IP %#" B_PRIx64 - ": %s\n", instructionPointer, strerror(error)); + Image* image = fTeam->ImageByAddress(instructionPointer); + if (image == NULL) { + data.SetToFormat("\t\t\tUnable to retrieve disassembly for IP %#" + B_PRIx64 ": address not contained in any valid image.\n", + instructionPointer); WRITE_AND_CHECK(_output, data); return B_OK; } + ImageDebugInfo* info = image->GetImageDebugInfo(); + if (info == NULL) { + data.SetToFormat("\t\t\tUnable to retrieve disassembly for IP %#" + B_PRIx64 ": no debug info available for image '%s'.\n", + instructionPointer, image->Name().String()); + WRITE_AND_CHECK(_output, data); + return B_OK; + } + + instance = info->FunctionAtAddress(instructionPointer); + if (instance == NULL) { + data.SetToFormat("\t\t\tUnable to retrieve disassembly for IP %#" + B_PRIx64 ": address does not point to a function.\n", + instructionPointer); + WRITE_AND_CHECK(_output, data); + return B_OK; + } + + Statement* statement = NULL; DisassembledCode* code = instance->GetSourceCode(); - Function* function = instance->GetFunction(); + BReference codeReference; if (code == NULL) { - switch (function->SourceCodeState()) { - case FUNCTION_SOURCE_NOT_LOADED: - case FUNCTION_SOURCE_LOADED: - // FUNCTION_SOURCE_LOADED is included since, if we entered - // here, it implies that the high level source for the - // function has been loaded, but the disassembly has not. - function->AddListener(this); - fSourceWaitingFunction = function; - fSourceWaitForDisassembly = true; - fListener->FunctionSourceCodeRequested(instance, true); - // fall through - case FUNCTION_SOURCE_LOADING: - { - teamLocker.Unlock(); - do { - error = acquire_sem(fTeamDataSem); - } while (error == B_INTERRUPTED); - - if (error != B_OK) - return error; - - teamLocker.Lock(); - fSourceWaitingFunction->RemoveListener(this); - break; - } - default: - return B_OK; + status_t error = fTeam->DebugInfo()->DisassembleFunction(instance, + code); + if (error != B_OK) { + data.SetToFormat("\t\t\tUnable to retrieve disassembly for IP %#" + B_PRIx64 ": %s.\n", instructionPointer, strerror(error)); + WRITE_AND_CHECK(_output, data); + return B_OK; } - if (instance->SourceCodeState() == FUNCTION_SOURCE_UNAVAILABLE) - return B_OK; - - error = fTeam->GetStatementAtAddress(instructionPointer, instance, - statement); - code = instance->GetSourceCode(); - } + codeReference.SetTo(code, true); + statement = code->StatementAtAddress(instructionPointer); + } else + codeReference.SetTo(code); SourceLocation location = statement->StartSourceLocation(); @@ -741,10 +738,12 @@ DebugReportGenerator::_DumpStackFrameMemory(BFile& _output, endAddress = framePointer; } - status_t error; + if (endAddress <= startAddress) + return B_OK; + if (fCurrentBlock == NULL || !fCurrentBlock->Contains(startAddress)) { + status_t error; fListener->InspectRequested(startAddress, this); - error = B_OK; do { error = acquire_sem(fTeamDataSem); } while (error == B_INTERRUPTED); @@ -827,6 +826,7 @@ DebugReportGenerator::_HandleMemoryBlockRetrieved(TeamMemoryBlock* block, } + /*static*/ int DebugReportGenerator::_CompareAreas(const AreaInfo* a, const AreaInfo* b) { diff --git a/src/kits/debugger/controllers/DebugReportGenerator.h b/src/kits/debugger/controllers/DebugReportGenerator.h index b8d50df45c..9eebca4059 100644 --- a/src/kits/debugger/controllers/DebugReportGenerator.h +++ b/src/kits/debugger/controllers/DebugReportGenerator.h @@ -1,5 +1,5 @@ /* - * Copyright 2012-2013, Rene Gollent, rene@gollent.com. + * Copyright 2012-2016, Rene Gollent, rene@gollent.com. * Distributed under the terms of the MIT License. */ #ifndef DEBUG_REPORT_GENERATOR_H @@ -64,7 +64,7 @@ private: virtual void FunctionSourceCodeChanged(Function* function); private: - status_t _GenerateReport(const entry_ref& outputPath); + status_t _GenerateReport(const char* outputPath); status_t _GenerateReportHeader(BFile& _output); status_t _DumpLoadedImages(BFile& _output); status_t _DumpAreas(BFile& _output); @@ -103,7 +103,6 @@ private: TeamMemoryBlock* fCurrentBlock; status_t fBlockRetrievalStatus; ::Thread* fTraceWaitingThread; - bool fSourceWaitForDisassembly; Function* fSourceWaitingFunction; }; diff --git a/src/kits/debugger/model/Team.cpp b/src/kits/debugger/model/Team.cpp index 0d54b0098c..bc769e6917 100644 --- a/src/kits/debugger/model/Team.cpp +++ b/src/kits/debugger/model/Team.cpp @@ -842,12 +842,12 @@ Team::NotifyWatchpointChanged(Watchpoint* watchpoint) void -Team::NotifyDebugReportChanged(const char* reportPath) +Team::NotifyDebugReportChanged(const char* reportPath, status_t result) { for (ListenerList::Iterator it = fListeners.GetIterator(); Listener* listener = it.Next();) { listener->DebugReportChanged(DebugReportEvent( - TEAM_EVENT_DEBUG_REPORT_CHANGED, this, reportPath)); + TEAM_EVENT_DEBUG_REPORT_CHANGED, this, reportPath, result)); } } @@ -1036,10 +1036,11 @@ Team::ConsoleOutputEvent::ConsoleOutputEvent(uint32 type, Team* team, Team::DebugReportEvent::DebugReportEvent(uint32 type, Team* team, - const char* reportPath) + const char* reportPath, status_t finalStatus) : Event(type, team), - fReportPath(reportPath) + fReportPath(reportPath), + fFinalStatus(finalStatus) { }