JDevlieghere updated this revision to Diff 415643. JDevlieghere marked 2 inline comments as done. JDevlieghere added a comment. Herald added a subscriber: mgorny.
Address code review feedback CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121511/new/ https://reviews.llvm.org/D121511 Files: lldb/include/lldb/Core/Debugger.h lldb/include/lldb/Core/DebuggerEvents.h lldb/source/Core/Debugger.cpp lldb/source/Core/DebuggerEvents.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h lldb/unittests/Core/CMakeLists.txt lldb/unittests/Core/DiagnosticEventTest.cpp
Index: lldb/unittests/Core/DiagnosticEventTest.cpp =================================================================== --- /dev/null +++ lldb/unittests/Core/DiagnosticEventTest.cpp @@ -0,0 +1,144 @@ +//===-- DiagnosticEventTest.cpp -------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "gtest/gtest.h" + +#include "Plugins/Platform/MacOSX/PlatformMacOSX.h" +#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h" +#include "TestingSupport/SubsystemRAII.h" +#include "lldb/Core/Debugger.h" +#include "lldb/Core/DebuggerEvents.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Utility/Broadcaster.h" +#include "lldb/Utility/Event.h" +#include "lldb/Utility/Listener.h" +#include "lldb/Utility/Reproducer.h" + +using namespace lldb; +using namespace lldb_private; +using namespace lldb_private::repro; + +static const constexpr std::chrono::seconds TIMEOUT(0); +static const constexpr size_t DEBUGGERS = 3; + +static std::once_flag debugger_initialize_flag; + +namespace { +class DiagnosticEventTest : public ::testing::Test { +public: + void SetUp() override { + llvm::cantFail(Reproducer::Initialize(ReproducerMode::Off, llvm::None)); + FileSystem::Initialize(); + HostInfo::Initialize(); + PlatformMacOSX::Initialize(); + std::call_once(debugger_initialize_flag, + []() { Debugger::Initialize(nullptr); }); + } + void TearDown() override { + PlatformMacOSX::Terminate(); + HostInfo::Terminate(); + FileSystem::Terminate(); + Reproducer::Terminate(); + } +}; +} // namespace + +TEST_F(DiagnosticEventTest, Warning) { + ArchSpec arch("x86_64-apple-macosx-"); + Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + + Broadcaster &broadcaster = debugger_sp->GetBroadcaster(); + ListenerSP listener_sp = Listener::MakeListener("test-listener"); + + listener_sp->StartListeningForEvents(&broadcaster, + Debugger::eBroadcastBitWarning); + EXPECT_TRUE( + broadcaster.EventTypeHasListeners(Debugger::eBroadcastBitWarning)); + + Debugger::ReportWarning("foo", debugger_sp->GetID()); + + EventSP event_sp; + EXPECT_TRUE(listener_sp->GetEvent(event_sp, TIMEOUT)); + ASSERT_TRUE(event_sp); + + const DiagnosticEventData *data = + DiagnosticEventData::GetEventDataFromEvent(event_sp.get()); + ASSERT_NE(data, nullptr); + EXPECT_EQ(data->GetPrefix(), "warning"); + EXPECT_EQ(data->GetMessage(), "foo"); + + Debugger::Destroy(debugger_sp); +} + +TEST_F(DiagnosticEventTest, Error) { + ArchSpec arch("x86_64-apple-macosx-"); + Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch)); + + DebuggerSP debugger_sp = Debugger::CreateInstance(); + + Broadcaster &broadcaster = debugger_sp->GetBroadcaster(); + ListenerSP listener_sp = Listener::MakeListener("test-listener"); + + listener_sp->StartListeningForEvents(&broadcaster, + Debugger::eBroadcastBitError); + EXPECT_TRUE(broadcaster.EventTypeHasListeners(Debugger::eBroadcastBitError)); + + Debugger::ReportError("bar", debugger_sp->GetID()); + + EventSP event_sp; + EXPECT_TRUE(listener_sp->GetEvent(event_sp, TIMEOUT)); + ASSERT_TRUE(event_sp); + + const DiagnosticEventData *data = + DiagnosticEventData::GetEventDataFromEvent(event_sp.get()); + ASSERT_NE(data, nullptr); + EXPECT_EQ(data->GetPrefix(), "error"); + EXPECT_EQ(data->GetMessage(), "bar"); + + Debugger::Destroy(debugger_sp); +} + +TEST_F(DiagnosticEventTest, MultipleDebuggers) { + ArchSpec arch("x86_64-apple-macosx-"); + Platform::SetHostPlatform(PlatformRemoteMacOSX::CreateInstance(true, &arch)); + + std::vector<DebuggerSP> debuggers; + std::vector<ListenerSP> listeners; + + for (size_t i = 0; i < DEBUGGERS; ++i) { + DebuggerSP debugger = Debugger::CreateInstance(); + ListenerSP listener = Listener::MakeListener("listener"); + + debuggers.push_back(debugger); + listeners.push_back(listener); + + listener->StartListeningForEvents(&debugger->GetBroadcaster(), + Debugger::eBroadcastBitError); + } + + Debugger::ReportError("baz"); + + for (size_t i = 0; i < DEBUGGERS; ++i) { + EventSP event_sp; + EXPECT_TRUE(listeners[i]->GetEvent(event_sp, TIMEOUT)); + ASSERT_TRUE(event_sp); + + const DiagnosticEventData *data = + DiagnosticEventData::GetEventDataFromEvent(event_sp.get()); + ASSERT_NE(data, nullptr); + EXPECT_EQ(data->GetPrefix(), "error"); + EXPECT_EQ(data->GetMessage(), "baz"); + } + + for (size_t i = 0; i < DEBUGGERS; ++i) { + Debugger::Destroy(debuggers[i]); + } +} Index: lldb/unittests/Core/CMakeLists.txt =================================================================== --- lldb/unittests/Core/CMakeLists.txt +++ lldb/unittests/Core/CMakeLists.txt @@ -1,5 +1,6 @@ add_lldb_unittest(LLDBCoreTests CommunicationTest.cpp + DiagnosticEventTest.cpp DumpDataExtractorTest.cpp FormatEntityTest.cpp MangledTest.cpp @@ -13,11 +14,12 @@ LINK_LIBS lldbCore lldbHost - lldbSymbol lldbPluginObjectFileELF lldbPluginObjectFileMachO lldbPluginObjectFilePECOFF + lldbPluginPlatformMacOSX lldbPluginSymbolFileSymtab + lldbSymbol lldbUtilityHelpers LLVMTestingSupport LINK_COMPONENTS Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h =================================================================== --- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h +++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h @@ -433,7 +433,8 @@ std::unique_ptr<NonPointerISACache> m_non_pointer_isa_cache_up; std::unique_ptr<TaggedPointerVendor> m_tagged_pointer_vendor_up; EncodingToTypeSP m_encoding_to_type_sp; - bool m_noclasses_warning_emitted; + std::once_flag m_no_classes_cached_warning; + std::once_flag m_no_expanded_cache_warning; llvm::Optional<std::pair<lldb::addr_t, lldb::addr_t>> m_CFBoolean_values; uint64_t m_realized_class_generation_count; }; Index: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp =================================================================== --- lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp +++ lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp @@ -21,6 +21,7 @@ #include "Plugins/TypeSystem/Clang/TypeSystemClang.h" #include "lldb/Core/Debugger.h" +#include "lldb/Core/DebuggerEvents.h" #include "lldb/Core/Module.h" #include "lldb/Core/PluginManager.h" #include "lldb/Core/Section.h" @@ -665,8 +666,8 @@ m_loaded_objc_opt(false), m_non_pointer_isa_cache_up(), m_tagged_pointer_vendor_up( TaggedPointerVendorV2::CreateInstance(*this, objc_module_sp)), - m_encoding_to_type_sp(), m_noclasses_warning_emitted(false), - m_CFBoolean_values(), m_realized_class_generation_count(0) { + m_encoding_to_type_sp(), m_CFBoolean_values(), + m_realized_class_generation_count(0) { static const ConstString g_gdb_object_getClass("gdb_object_getClass"); m_has_object_getClass = HasSymbol(g_gdb_object_getClass); static const ConstString g_objc_copyRealizedClassList( @@ -2330,32 +2331,27 @@ void AppleObjCRuntimeV2::WarnIfNoClassesCached( SharedCacheWarningReason reason) { - if (m_noclasses_warning_emitted) - return; - if (GetProcess() && !DoesProcessHaveSharedCache(*GetProcess())) { // Simulators do not have the objc_opt_ro class table so don't actually // complain to the user - m_noclasses_warning_emitted = true; return; } Debugger &debugger(GetProcess()->GetTarget().GetDebugger()); - if (auto stream = debugger.GetAsyncOutputStream()) { - switch (reason) { - case SharedCacheWarningReason::eNotEnoughClassesRead: - stream->PutCString("warning: could not find Objective-C class data in " - "the process. This may reduce the quality of type " - "information available.\n"); - m_noclasses_warning_emitted = true; - break; - case SharedCacheWarningReason::eExpressionExecutionFailure: - stream->PutCString("warning: could not execute support code to read " - "Objective-C class data in the process. This may " - "reduce the quality of type information available.\n"); - m_noclasses_warning_emitted = true; - break; - } + switch (reason) { + case SharedCacheWarningReason::eNotEnoughClassesRead: + Debugger::ReportWarning("warning: could not find Objective-C class data in " + "the process. This may reduce the quality of type " + "information available.\n", + debugger.GetID(), &m_no_classes_cached_warning); + break; + case SharedCacheWarningReason::eExpressionExecutionFailure: + Debugger::ReportWarning( + "warning: could not execute support code to read " + "Objective-C class data in the process. This may " + "reduce the quality of type information available.\n", + debugger.GetID(), &m_no_classes_cached_warning); + break; } } @@ -2372,17 +2368,25 @@ Target &target = GetProcess()->GetTarget(); Debugger &debugger = target.GetDebugger(); - if (auto stream = debugger.GetAsyncOutputStream()) { - const char *msg = "read from the shared cache"; - if (PlatformSP platform_sp = target.GetPlatform()) - msg = platform_sp->IsHost() - ? "read from the host's in-memory shared cache" - : "find the on-disk shared cache for this device"; - stream->Printf("warning: libobjc.A.dylib is being read from process " - "memory. This indicates that LLDB could not %s. This will " - "likely reduce debugging performance.\n", - msg); + + std::string buffer; + llvm::raw_string_ostream os(buffer); + + os << "warning: libobjc.A.dylib is being read from process memory. This " + "indicates that LLDB could not "; + if (PlatformSP platform_sp = target.GetPlatform()) { + if (platform_sp->IsHost()) { + os << "read from the host's in-memory shared cache"; + } else { + os << "find the on-disk shared cache for this device"; + } + } else { + os << "read from the shared cache"; } + os << ". This will likely reduce debugging performance.\n"; + + Debugger::ReportWarning(os.str(), debugger.GetID(), + &m_no_expanded_cache_warning); } DeclVendor *AppleObjCRuntimeV2::GetDeclVendor() { Index: lldb/source/Core/DebuggerEvents.cpp =================================================================== --- lldb/source/Core/DebuggerEvents.cpp +++ lldb/source/Core/DebuggerEvents.cpp @@ -10,6 +10,15 @@ using namespace lldb_private; +template <typename T> +static const T *GetEventDataFromEventImpl(const Event *event_ptr) { + if (event_ptr) + if (const EventData *event_data = event_ptr->GetData()) + if (event_data->GetFlavor() == T::GetFlavorString()) + return static_cast<const T *>(event_ptr->GetData()); + return nullptr; +} + ConstString ProgressEventData::GetFlavorString() { static ConstString g_flavor("ProgressEventData"); return g_flavor; @@ -33,9 +42,19 @@ const ProgressEventData * ProgressEventData::GetEventDataFromEvent(const Event *event_ptr) { - if (event_ptr) - if (const EventData *event_data = event_ptr->GetData()) - if (event_data->GetFlavor() == ProgressEventData::GetFlavorString()) - return static_cast<const ProgressEventData *>(event_ptr->GetData()); - return nullptr; + return GetEventDataFromEventImpl<ProgressEventData>(event_ptr); +} + +ConstString DiagnosticEventData::GetFlavorString() { + static ConstString g_flavor("DiagnosticEventData"); + return g_flavor; +} + +ConstString DiagnosticEventData::GetFlavor() const { + return DiagnosticEventData::GetFlavorString(); +} + +const DiagnosticEventData * +DiagnosticEventData::GetEventDataFromEvent(const Event *event_ptr) { + return GetEventDataFromEventImpl<DiagnosticEventData>(event_ptr); } Index: lldb/source/Core/Debugger.cpp =================================================================== --- lldb/source/Core/Debugger.cpp +++ lldb/source/Core/Debugger.cpp @@ -1326,6 +1326,73 @@ } } +static void PrivateReportDiagnostic(Debugger &debugger, + DiagnosticEventData::Type type, + std::string message, + bool debugger_specific) { + uint32_t event_type = 0; + switch (type) { + case DiagnosticEventData::Type::Warning: + event_type = Debugger::eBroadcastBitWarning; + break; + case DiagnosticEventData::Type::Error: + event_type = Debugger::eBroadcastBitError; + break; + } + + Broadcaster &broadcaster = debugger.GetBroadcaster(); + if (!broadcaster.EventTypeHasListeners(event_type)) + return; + EventSP event_sp = std::make_shared<Event>( + event_type, + new DiagnosticEventData(type, std::move(message), debugger_specific)); + broadcaster.BroadcastEvent(event_sp); +} + +void Debugger::ReportDiagnosticImpl(DiagnosticEventData::Type type, + std::string message, + llvm::Optional<lldb::user_id_t> debugger_id, + std::once_flag *once) { + auto ReportDiagnosticLambda = [&]() { + // Check if this progress is for a specific debugger. + if (debugger_id) { + // It is debugger specific, grab it and deliver the event if the debugger + // still exists. + DebuggerSP debugger_sp = FindDebuggerWithID(*debugger_id); + if (debugger_sp) + PrivateReportDiagnostic(*debugger_sp, type, std::move(message), true); + return; + } + // The progress event is not debugger specific, iterate over all debuggers + // and deliver a progress event to each one. + if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) { + std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr); + for (const auto &debugger : *g_debugger_list_ptr) + PrivateReportDiagnostic(*debugger, type, message, false); + } + }; + + if (once) + std::call_once(*once, ReportDiagnosticLambda); + else + ReportDiagnosticLambda(); +} + +void Debugger::ReportWarning(std::string message, + llvm::Optional<lldb::user_id_t> debugger_id, + std::once_flag *once) { + ReportDiagnosticImpl(DiagnosticEventData::Type::Warning, std::move(message), + debugger_id, once); +} + +void Debugger::ReportError(std::string message, + llvm::Optional<lldb::user_id_t> debugger_id, + std::once_flag *once) { + + ReportDiagnosticImpl(DiagnosticEventData::Type::Error, std::move(message), + debugger_id, once); +} + bool Debugger::EnableLog(llvm::StringRef channel, llvm::ArrayRef<const char *> categories, llvm::StringRef log_file, uint32_t log_options, @@ -1605,8 +1672,9 @@ CommandInterpreter::eBroadcastBitAsynchronousOutputData | CommandInterpreter::eBroadcastBitAsynchronousErrorData); - listener_sp->StartListeningForEvents(&m_broadcaster, - Debugger::eBroadcastBitProgress); + listener_sp->StartListeningForEvents( + &m_broadcaster, + eBroadcastBitProgress | eBroadcastBitWarning | eBroadcastBitError); // Let the thread that spawned us know that we have started up and that we // are now listening to all required events so no events get missed @@ -1660,6 +1728,10 @@ } else if (broadcaster == &m_broadcaster) { if (event_type & Debugger::eBroadcastBitProgress) HandleProgressEvent(event_sp); + else if (event_type & Debugger::eBroadcastBitWarning) + HandleDiagnosticEvent(event_sp); + else if (event_type & Debugger::eBroadcastBitError) + HandleDiagnosticEvent(event_sp); } } @@ -1793,6 +1865,15 @@ output->Flush(); } +void Debugger::HandleDiagnosticEvent(const lldb::EventSP &event_sp) { + auto *data = DiagnosticEventData::GetEventDataFromEvent(event_sp.get()); + if (!data) + return; + + StreamSP stream = GetAsyncErrorStream(); + *stream << data->GetPrefix() << ": " << data->GetMessage() << '\n'; +} + bool Debugger::HasIOHandlerThread() { return m_io_handler_thread.IsJoinable(); } bool Debugger::StartIOHandlerThread() { Index: lldb/include/lldb/Core/DebuggerEvents.h =================================================================== --- lldb/include/lldb/Core/DebuggerEvents.h +++ lldb/include/lldb/Core/DebuggerEvents.h @@ -46,6 +46,45 @@ ProgressEventData(const ProgressEventData &) = delete; const ProgressEventData &operator=(const ProgressEventData &) = delete; }; + +class DiagnosticEventData : public EventData { +public: + enum class Type { + Warning, + Error, + }; + DiagnosticEventData(Type type, std::string message, bool debugger_specific) + : m_message(std::move(message)), m_type(type), + m_debugger_specific(debugger_specific) {} + ~DiagnosticEventData() {} + + const std::string &GetMessage() const { return m_message; } + Type GetType() const { return m_type; } + + llvm::StringRef GetPrefix() const { + switch (m_type) { + case Type::Warning: + return "warning"; + case Type::Error: + return "error"; + } + } + + static ConstString GetFlavorString(); + ConstString GetFlavor() const override; + + static const DiagnosticEventData * + GetEventDataFromEvent(const Event *event_ptr); + +protected: + std::string m_message; + Type m_type; + const bool m_debugger_specific; + + DiagnosticEventData(const DiagnosticEventData &) = delete; + const DiagnosticEventData &operator=(const DiagnosticEventData &) = delete; +}; + } // namespace lldb_private #endif // LLDB_CORE_DEBUGGER_EVENTS_H Index: lldb/include/lldb/Core/Debugger.h =================================================================== --- lldb/include/lldb/Core/Debugger.h +++ lldb/include/lldb/Core/Debugger.h @@ -14,6 +14,7 @@ #include <memory> #include <vector> +#include "lldb/Core/DebuggerEvents.h" #include "lldb/Core/FormatEntity.h" #include "lldb/Core/IOHandler.h" #include "lldb/Core/SourceManager.h" @@ -57,7 +58,6 @@ class Stream; class SymbolContext; class Target; -class ProgressEventData; namespace repro { class DataRecorder; @@ -77,6 +77,8 @@ /// Broadcaster event bits definitions. enum { eBroadcastBitProgress = (1 << 0), + eBroadcastBitWarning = (1 << 1), + eBroadcastBitError = (1 << 2), }; static ConstString GetStaticBroadcasterClass(); @@ -375,6 +377,50 @@ return m_broadcaster_manager_sp; } + /// Report warning events. + /// + /// Progress events will be delivered to any debuggers that have listeners + /// for the eBroadcastBitError. + /// + /// \param[in] message + /// The warning message to be reported. + /// + /// \param [in] debugger_id + /// If this optional parameter has a value, it indicates the unique + /// debugger identifier that this progress should be delivered to. If this + /// optional parameter does not have a value, the progress will be + /// delivered to all debuggers. + /// + /// \param [in] once + /// If a pointer is passed to a std::once_flag, then it will be used to + /// ensure the given warning is only broadcast once. + static void + ReportWarning(std::string messsage, + llvm::Optional<lldb::user_id_t> debugger_id = llvm::None, + std::once_flag *once = nullptr); + + /// Report error events. + /// + /// Progress events will be delivered to any debuggers that have listeners + /// for the eBroadcastBitError. + /// + /// \param[in] message + /// The error message to be reported. + /// + /// \param [in] debugger_id + /// If this optional parameter has a value, it indicates the unique + /// debugger identifier that this progress should be delivered to. If this + /// optional parameter does not have a value, the progress will be + /// delivered to all debuggers. + /// + /// \param [in] once + /// If a pointer is passed to a std::once_flag, then it will be used to + /// ensure the given error is only broadcast once. + static void + ReportError(std::string messsage, + llvm::Optional<lldb::user_id_t> debugger_id = llvm::None, + std::once_flag *once = nullptr); + protected: friend class CommandInterpreter; friend class REPL; @@ -413,6 +459,11 @@ uint64_t completed, uint64_t total, llvm::Optional<lldb::user_id_t> debugger_id); + static void ReportDiagnosticImpl(DiagnosticEventData::Type type, + std::string message, + llvm::Optional<lldb::user_id_t> debugger_id, + std::once_flag *once); + void PrintProgress(const ProgressEventData &data); bool StartEventHandlerThread(); @@ -444,6 +495,8 @@ void HandleProgressEvent(const lldb::EventSP &event_sp); + void HandleDiagnosticEvent(const lldb::EventSP &event_sp); + // Ensures two threads don't attempt to flush process output in parallel. std::mutex m_output_flush_mutex; void FlushProcessOutput(Process &process, bool flush_stdout,
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
