[LLDB] Add a StackFrameRecognizer for the Darwin specific abort_with_payload… #101365
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesThis is used by various system routines (the capabilities checker and dyld to name a few) to add extra color to an abort. This patch adds a frame recognizer so people can easily see the details, and also adds the information to the ExtendedCrashInformation dictionary. I also had to rework how the dictionary is held; previously it was created on demand, but that was inconvenient since it meant all the entries had to be produced at that same time. That didn't work for the recognizer. Patch is 26.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/101365.diff 10 Files Affected:
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index c8475db8ae160..ffff84bbd8a52 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -2569,6 +2569,18 @@ void PruneThreadPlans();
/// A StructuredDataSP object which, if non-empty, will contain the
/// information related to the process.
virtual StructuredData::DictionarySP GetMetadata() { return nullptr; }
+
+ /// Fetch extended crash information held by the process. This will never be
+ /// an empty shared pointer, it will always have a dict, though it may be
+ /// empty.
+ StructuredData::DictionarySP GetExtendedCrashInfoDict() {
+ return m_crash_info_dict_sp;
+ }
+
+ void ResetExtendedCrashInfoDict() {
+ // StructuredData::Dictionary is add only, so we have to make a new one:
+ m_crash_info_dict_sp.reset(new StructuredData::Dictionary());
+ }
size_t AddImageToken(lldb::addr_t image_ptr);
@@ -3185,6 +3197,10 @@ void PruneThreadPlans();
/// Per process source file cache.
SourceManager::SourceFileCache m_source_file_cache;
+
+ /// A repository for extra crash information, consulted in
+ /// GetExtendedCrashInformation.
+ StructuredData::DictionarySP m_crash_info_dict_sp;
size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size,
uint8_t *buf) const;
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
index 6fb5bbfbe417b..398b44c293614 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp
@@ -856,21 +856,38 @@ PlatformDarwin::ParseVersionBuildDir(llvm::StringRef dir) {
}
llvm::Expected<StructuredData::DictionarySP>
-PlatformDarwin::FetchExtendedCrashInformation(Process &process) {
- StructuredData::DictionarySP extended_crash_info =
- std::make_shared<StructuredData::Dictionary>();
-
- StructuredData::ArraySP annotations = ExtractCrashInfoAnnotations(process);
- if (annotations && annotations->GetSize())
- extended_crash_info->AddItem("Crash-Info Annotations", annotations);
-
- StructuredData::DictionarySP app_specific_info =
- ExtractAppSpecificInfo(process);
- if (app_specific_info && app_specific_info->GetSize())
- extended_crash_info->AddItem("Application Specific Information",
- app_specific_info);
+PlatformDarwin::
+FetchExtendedCrashInformation(Process &process) {
+ static constexpr llvm::StringLiteral crash_info_key("Crash-Info Annotations");
+ static constexpr llvm::StringLiteral asi_info_key("Application Specific Information");
+
+ // We cache the information we find in the process extended info dict:
+ StructuredData::DictionarySP process_dict_sp
+ = process.GetExtendedCrashInfoDict();
+ StructuredData::Array *annotations = nullptr;
+ StructuredData::ArraySP new_annotations_sp;
+ if (!process_dict_sp->GetValueForKeyAsArray(crash_info_key, annotations)) {
+ new_annotations_sp = ExtractCrashInfoAnnotations(process);
+ if (new_annotations_sp && new_annotations_sp->GetSize()) {
+ process_dict_sp->AddItem(crash_info_key, new_annotations_sp);
+ annotations = new_annotations_sp.get();
+ }
+ }
- return extended_crash_info->GetSize() ? extended_crash_info : nullptr;
+ StructuredData::Dictionary *app_specific_info;
+ StructuredData::DictionarySP new_app_specific_info_sp;
+ if (!process_dict_sp->GetValueForKeyAsDictionary(asi_info_key, app_specific_info))
+ {
+ new_app_specific_info_sp = ExtractAppSpecificInfo(process);
+ if (new_app_specific_info_sp && new_app_specific_info_sp->GetSize()) {
+ process_dict_sp->AddItem(asi_info_key, new_app_specific_info_sp);
+ app_specific_info = new_app_specific_info_sp.get();
+ }
+ }
+
+ // Now get anything else that was in the process info dict, and add it to the
+ // return here:
+ return process_dict_sp->GetSize() ? process_dict_sp : nullptr;
}
StructuredData::ArraySP
diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp
new file mode 100644
index 0000000000000..7f2aecd148379
--- /dev/null
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp
@@ -0,0 +1,201 @@
+ //===-- AbortWithPayloadFrameRecognizer.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 "AbortWithPayloadFrameRecognizer.h"
+
+#include "lldb/Core/Value.h"
+#include "lldb/Core/ValueObjectConstResult.h"
+#include "lldb/Target/ABI.h"
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StackFrame.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/Utility/StructuredData.h"
+
+#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace lldb_private {
+void RegisterAbortWithPayloadFrameRecognizer(Process *process) {
+ // There are two user-level API's that this recognizer captures,
+ // abort_with_reason and abort_with_payload. But they both call the private
+ // __abort_with_payload, the abort_with_reason call fills in a null payload.
+ static ConstString module_name("libsystem_kernel.dylib");
+ static ConstString sym_name("__abort_with_payload");
+
+ if (!process)
+ return;
+ ConstString sym_arr[1]= {sym_name};
+
+ process->GetTarget().GetFrameRecognizerManager().AddRecognizer(
+ std::make_shared<AbortWithPayloadFrameRecognizer>(),
+ module_name, sym_arr,
+ /*first_instruction_only*/ false);
+}
+
+RecognizedStackFrameSP
+AbortWithPayloadFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
+ // We have two jobs:
+ // 1) to add the data passed to abort_with_payload to the
+ // ExtraCrashInformation dictionary.
+ // 2) To make up faux arguments for this frame.
+ static constexpr llvm::StringLiteral namespace_key("namespace");
+ static constexpr llvm::StringLiteral code_key("code");
+ static constexpr llvm::StringLiteral payload_addr_key("payload_addr");
+ static constexpr llvm::StringLiteral payload_size_key("payload_size");
+ static constexpr llvm::StringLiteral reason_key("reason");
+ static constexpr llvm::StringLiteral flags_key("flags");
+ static constexpr llvm::StringLiteral info_key("abort_with_payload");
+
+ // We are fetching the data from registers.
+ Thread *thread = frame_sp->GetThread().get();
+ Process *process = thread->GetProcess().get();
+
+ // FIXME: Add logging for these errors
+ if (!thread)
+ return {};
+
+ TypeSystemClangSP scratch_ts_sp =
+ ScratchTypeSystemClang::GetForTarget(process->GetTarget());
+ if (!scratch_ts_sp)
+ return {};
+
+ // The abort_with_payload signature is:
+ // abort_with_payload(uint32_t reason_namespace, uint64_t reason_code,
+ // void* payload, uint32_t payload_size,
+ // const char* reason_string, uint64_t reason_flags);
+
+ ValueList arg_values;
+ Value input_value_32;
+ Value input_value_64;
+ Value input_value_void_ptr;
+ Value input_value_char_ptr;
+
+ CompilerType clang_void_ptr_type =
+ scratch_ts_sp->GetBasicType(eBasicTypeVoid).GetPointerType();
+ CompilerType clang_char_ptr_type =
+ scratch_ts_sp->GetBasicType(eBasicTypeChar).GetPointerType();
+ CompilerType clang_uint64_type =
+ scratch_ts_sp->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint,
+ 64);
+ CompilerType clang_uint32_type =
+ scratch_ts_sp->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint,
+ 32);
+ CompilerType clang_char_star_type =
+ scratch_ts_sp->GetBuiltinTypeForEncodingAndBitSize(lldb::eEncodingUint,
+ 64);
+
+ input_value_32.SetValueType(Value::ValueType::Scalar);
+ input_value_32.SetCompilerType(clang_uint32_type);
+ input_value_64.SetValueType(Value::ValueType::Scalar);
+ input_value_64.SetCompilerType(clang_uint64_type);
+ input_value_void_ptr.SetValueType(Value::ValueType::Scalar);
+ input_value_void_ptr.SetCompilerType(clang_void_ptr_type);
+ input_value_char_ptr.SetValueType(Value::ValueType::Scalar);
+ input_value_char_ptr.SetCompilerType(clang_char_ptr_type);
+
+ arg_values.PushValue(input_value_32);
+ arg_values.PushValue(input_value_64);
+ arg_values.PushValue(input_value_void_ptr);
+ arg_values.PushValue(input_value_32);
+ arg_values.PushValue(input_value_char_ptr);
+ arg_values.PushValue(input_value_64);
+
+ lldb::ABISP abi_sp = process->GetABI();
+ bool success = abi_sp->GetArgumentValues(*thread, arg_values);
+ if (!success)
+ return {};
+
+ Value *cur_value;
+ StackFrame *frame = frame_sp.get();
+ ValueObjectListSP arguments_sp = ValueObjectListSP(new ValueObjectList());
+
+ auto add_to_arguments = [&](llvm::StringRef name, Value *value, bool dynamic)
+ {
+ ValueObjectSP cur_valobj_sp = ValueObjectConstResult::Create(frame, *value,
+ ConstString(name));
+ cur_valobj_sp = ValueObjectRecognizerSynthesizedValue::Create(
+ *cur_valobj_sp, eValueTypeVariableArgument);
+ ValueObjectSP dyn_valobj_sp;
+ if (dynamic) {
+ dyn_valobj_sp = cur_valobj_sp->GetDynamicValue(eDynamicDontRunTarget);
+ if (dyn_valobj_sp)
+ cur_valobj_sp = dyn_valobj_sp;
+ }
+ arguments_sp->Append(cur_valobj_sp);
+ };
+
+ // Decode the arg_values:
+
+ uint32_t namespace_val = 0;
+ cur_value = arg_values.GetValueAtIndex(0);
+ add_to_arguments(namespace_key, cur_value, false);
+ namespace_val = cur_value->GetScalar().UInt(namespace_val);
+
+ uint32_t code_val = 0;
+ cur_value = arg_values.GetValueAtIndex(1);
+ add_to_arguments(code_key, cur_value, false);
+ code_val = cur_value->GetScalar().UInt(code_val);
+
+ lldb::addr_t payload_addr = LLDB_INVALID_ADDRESS;
+ cur_value = arg_values.GetValueAtIndex(2);
+ add_to_arguments(payload_addr_key, cur_value, true);
+ payload_addr = cur_value->GetScalar().ULongLong(payload_addr);
+
+ uint32_t payload_size = 0;
+ cur_value = arg_values.GetValueAtIndex(3);
+ add_to_arguments(payload_size_key, cur_value, false);
+ payload_size = cur_value->GetScalar().UInt(payload_size);
+
+ lldb::addr_t reason_addr = LLDB_INVALID_ADDRESS;
+ cur_value = arg_values.GetValueAtIndex(4);
+ add_to_arguments(reason_key, cur_value, false);
+ reason_addr = cur_value->GetScalar().ULongLong(payload_addr);
+
+ // For the reason string, we want the string not the address, so fetch that.
+ std::string reason_string;
+ Status error;
+ size_t str_len = process->ReadCStringFromMemory(reason_addr, reason_string, error);
+ if (error.Fail())
+ return {};
+
+ uint32_t flags_val = 0;
+ cur_value = arg_values.GetValueAtIndex(5);
+ add_to_arguments(flags_key, cur_value, false);
+ flags_val = cur_value->GetScalar().UInt(flags_val);
+
+ // Okay, we've gotten all the argument values, now put then in a
+ // StructuredData, and add that to the Process ExtraCrashInformation:
+ StructuredData::DictionarySP abort_dict_sp(new StructuredData::Dictionary());
+ abort_dict_sp->AddIntegerItem(namespace_key, namespace_val);
+ abort_dict_sp->AddIntegerItem(code_key, code_val);
+ abort_dict_sp->AddIntegerItem(payload_addr_key, payload_addr);
+ abort_dict_sp->AddIntegerItem(payload_size_key, payload_size);
+ abort_dict_sp->AddStringItem(reason_key, reason_string);
+ abort_dict_sp->AddIntegerItem(flags_key, flags_val);
+
+ // This will overwrite any information in the dictionary already.
+ // But we can only crash on abort_with_payload once, so that shouldn't matter.
+ process->GetExtendedCrashInfoDict()->AddItem(info_key, abort_dict_sp);
+
+ return RecognizedStackFrameSP(new AbortWithPayloadRecognizedStackFrame(frame_sp, arguments_sp));
+}
+
+AbortWithPayloadRecognizedStackFrame::AbortWithPayloadRecognizedStackFrame(
+ lldb::StackFrameSP &frame_sp, ValueObjectListSP &args_sp) :
+ RecognizedStackFrame() {
+ m_arguments = args_sp;
+ m_stop_desc = "abort with payload or reason";
+}
+
+} // namespace lldb_private
+
diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.h b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.h
new file mode 100644
index 0000000000000..25053a4322e7d
--- /dev/null
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.h
@@ -0,0 +1,38 @@
+//===-- AbortWithPayloadFrameRecognizer.h -----------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+#ifndef LLDB_MACOSX_ABORTWITHPAYLOADFRAMERECOGNIZER_H
+#define LLDB_MACOSX_ABORTWITHPAYLOADFRAMERECOGNIZER_H
+
+#include "lldb/Target/Process.h"
+#include "lldb/Target/StackFrameRecognizer.h"
+#include "lldb/Utility/ConstString.h"
+#include "lldb/Utility/FileSpec.h"
+
+#include <tuple>
+
+namespace lldb_private {
+
+void RegisterAbortWithPayloadFrameRecognizer(Process *process);
+
+class AbortWithPayloadRecognizedStackFrame : public RecognizedStackFrame {
+public:
+ AbortWithPayloadRecognizedStackFrame(lldb::StackFrameSP &frame_sp,
+ lldb::ValueObjectListSP &args_sp);
+};
+
+class AbortWithPayloadFrameRecognizer: public StackFrameRecognizer {
+ public:
+ std::string GetName() override {
+ return "abort_with_payload StackFrame Recognizer"; }
+ lldb::RecognizedStackFrameSP
+ RecognizeFrame(lldb::StackFrameSP frame_sp) override;
+
+};
+} // namespace lldb_private
+
+#endif // LLDB_MACOSX_ABORTWITHPAYLOADFRAMERECOGNIZER_H
diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/CMakeLists.txt b/lldb/source/Plugins/SystemRuntime/MacOSX/CMakeLists.txt
index 04d30652ba4b4..17fccdf43c828 100644
--- a/lldb/source/Plugins/SystemRuntime/MacOSX/CMakeLists.txt
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/CMakeLists.txt
@@ -4,6 +4,7 @@ add_lldb_library(lldbPluginSystemRuntimeMacOSX PLUGIN
AppleGetQueuesHandler.cpp
AppleGetThreadItemInfoHandler.cpp
SystemRuntimeMacOSX.cpp
+ AbortWithPayloadFrameRecognizer.cpp
LINK_LIBS
lldbBreakpoint
diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp b/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
index 34ce175920d1e..999b81b49e6e0 100644
--- a/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
@@ -29,6 +29,7 @@
#include "lldb/Utility/StreamString.h"
#include "SystemRuntimeMacOSX.h"
+#include "AbortWithPayloadFrameRecognizer.h"
#include <memory>
@@ -90,7 +91,10 @@ SystemRuntimeMacOSX::SystemRuntimeMacOSX(Process *process)
m_libpthread_offsets(), m_dispatch_tsd_indexes_addr(LLDB_INVALID_ADDRESS),
m_libdispatch_tsd_indexes(),
m_dispatch_voucher_offsets_addr(LLDB_INVALID_ADDRESS),
- m_libdispatch_voucher_offsets() {}
+ m_libdispatch_voucher_offsets() {
+
+ RegisterAbortWithPayloadFrameRecognizer(process);
+}
// Destructor
SystemRuntimeMacOSX::~SystemRuntimeMacOSX() { Clear(true); }
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index d5a639d9beacd..c72852d2afffa 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -477,7 +477,8 @@ Process::Process(lldb::TargetSP target_sp, ListenerSP listener_sp,
m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
m_can_interpret_function_calls(false), m_run_thread_plan_lock(),
- m_can_jit(eCanJITDontKnow) {
+ m_can_jit(eCanJITDontKnow),
+ m_crash_info_dict_sp(new StructuredData::Dictionary()) {
CheckInWithManager();
Log *log = GetLog(LLDBLog::Object);
@@ -3263,6 +3264,10 @@ Status Process::PrivateResume() {
// If signals handing status changed we might want to update our signal
// filters before resuming.
UpdateAutomaticSignalFiltering();
+ // Clear any crash info we accumulated for this stop, but don't do so if we
+ // are running functions; we don't want to wipe out the real stop's info.
+ if (!GetModID().IsLastResumeForUserExpression())
+ ResetExtendedCrashInfoDict();
Status error(WillResume());
// Tell the process it is about to resume before the thread list
diff --git a/lldb/test/API/macosx/abort_with_payload/Makefile b/lldb/test/API/macosx/abort_with_payload/Makefile
new file mode 100644
index 0000000000000..695335e068c0c
--- /dev/null
+++ b/lldb/test/API/macosx/abort_with_payload/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/macosx/abort_with_payload/TestAbortWithPayload.py b/lldb/test/API/macosx/abort_with_payload/TestAbortWithPayload.py
new file mode 100644
index 0000000000000..764f8441382a5
--- /dev/null
+++ b/lldb/test/API/macosx/abort_with_payload/TestAbortWithPayload.py
@@ -0,0 +1,146 @@
+"""
+Test that the FrameRecognizer for __abort_with_payload
+works properly
+"""
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestAbortWithPayload(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_abort_with_payload(self):
+ """There can be many tests in a test case - describe this test here."""
+ self.build()
+ self.abort_with_test(True)
+
+ def test_abort_with_reason(self):
+ """There can be many tests in a test case - describe this test here."""
+ self.build()
+ self.abort_with_test(False)
+
+ def setUp(self):
+ # Call super's setUp().
+ TestBase.setUp(self)
+ self.main_source_file = lldb.SBFileSpec("main.c")
+
+ def abort_with_test(self, with_payload):
+ """If with_payload is True, we test the abort_with_payload call,
+ if false, we test abort_with_reason."""
+ launch_info = lldb.SBLaunchInfo([])
+ if not with_payload:
+ launch_info.SetArguments(["use_reason"], True)
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "Stop here before abort", self.main_source_file,
+ launch_info=launch_info
+ )
+
+ frame = thread.GetFrameAtIndex(0)
+ payload_str_var = frame.FindVariable("payload_string")
+ self.assertSuccess(payload_str_var.GetError(), "Got payload string var")
+ payload_var_addr = payload_str_var.unsigned
+
+ payload_size_var = frame.FindVariable("payload_string_len")
+ self.assertSuccess(payload_size_var.GetError(), "Got payload string len var")
+ payload_size_val = payload_size_var.unsigned
+
+ # Not let it run to crash:
+ process.Continue()
+
+ # At this point we should have stopped at the internal function.
+ # Make sure we selected the right thread:
+ sel_thread = process.GetSelectedThread()
+ self.assertEqual(thread, sel_thread, "Selected the original thread")
+ # Make sure the stop reason is right:
+ self.assertEqual(thread.GetStopDescription(100), "abort with payload or reason", "Description was right")
+ frame_0 = thread.frames[0]
+ self.assertEqual(frame_0.name, "__abort_with_payload", "Frame 0 was right")
+
+ # Now check the recognized argument values and the ExtendedCrashInformation version:
+ options = lldb.SBVariablesOptions()
+ options.SetIncludeRecognizedArguments(True)
+ options.SetIncludeArguments(False)
+ options.SetIncludeLocals(False)
+ options.SetIncludeStatics(False)
+ options.SetIncludeRuntim...
[truncated]
|
You can test this locally with the following command:darker --check --diff -r 80c0e8d572d3b1c34d66faffc42bcfc9432583ec...39afb8e218b8e26f8b21728b6d432faa494e0955 lldb/test/API/macosx/abort_with_payload/TestAbortWithPayload.pyView the diff from darker here.--- TestAbortWithPayload.py 2024-07-31 22:41:31.000000 +0000
+++ TestAbortWithPayload.py 2024-07-31 22:45:09.844700 +0000
@@ -104,12 +104,12 @@
)
# We always stop at __abort_with_payload, regardless of whether the caller
# was abort_with_reason or abort_with_payload or any future API that
# funnels here. Since I don't want to have to know too much about the
- # callers, I just always report what is in the function I've
- #
+ # callers, I just always report what is in the function I've
+ #
# add the payload ones if it is the payload not the reason function.
self.assertEqual(arguments[2].name, "payload_addr")
self.assertEqual(arguments[3].name, "payload_size")
if with_payload:
self.assertEqual(
|
You can test this locally with the following command:git-clang-format --diff 80c0e8d572d3b1c34d66faffc42bcfc9432583ec 39afb8e218b8e26f8b21728b6d432faa494e0955 --extensions h,c,cpp -- lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.h lldb/test/API/macosx/abort_with_payload/main.c lldb/include/lldb/Target/Process.h lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp lldb/source/Target/Process.cppView the diff from clang-format here.diff --git a/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp
index 0d19d63fec..e580ca5188 100644
--- a/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp
+++ b/lldb/source/Plugins/SystemRuntime/MacOSX/AbortWithPayloadFrameRecognizer.cpp
@@ -36,7 +36,7 @@ void RegisterAbortWithPayloadFrameRecognizer(Process *process) {
return;
process->GetTarget().GetFrameRecognizerManager().AddRecognizer(
- std::make_shared<AbortWithPayloadFrameRecognizer>(), module_name,
+ std::make_shared<AbortWithPayloadFrameRecognizer>(), module_name,
sym_name, /*first_instruction_only*/ false);
}
@@ -55,7 +55,7 @@ AbortWithPayloadFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
static constexpr llvm::StringLiteral info_key("abort_with_payload");
Log *log = GetLog(LLDBLog::SystemRuntime);
-
+
if (!frame_sp) {
LLDB_LOG(log, "abort_with_payload recognizer: invalid frame.");
return {};
@@ -197,8 +197,8 @@ AbortWithPayloadFrameRecognizer::RecognizeFrame(lldb::StackFrameSP frame_sp) {
abort_dict_sp->AddStringItem(reason_key, reason_string);
abort_dict_sp->AddIntegerItem(flags_key, flags_val);
- // This will overwrite the abort_with_payload information in the dictionary
- // already. But we can only crash on abort_with_payload once, so that
+ // This will overwrite the abort_with_payload information in the dictionary
+ // already. But we can only crash on abort_with_payload once, so that
// shouldn't matter.
process->GetExtendedCrashInfoDict()->AddItem(info_key, abort_dict_sp);
|
tschuett
changed the title
|
|
||
| if (!process) | ||
| return; | ||
| ConstString sym_arr[1] = {sym_name}; |
There was a problem hiding this comment.
nit: the size of the array is optional if you're not mutating it. Might as well make it const.
There was a problem hiding this comment.
That was a left-over from when I thought I was going to match abort_with_payload and _abort_with_reason, but they don't really work as well, since when you stop you WILL be at the beginning of __abort_with_payload so arguments are still in registers, which is not true of the public functions. I don't need the array at all.
| ConstString sym_arr[1] = {sym_name}; | ||
|
|
||
| process->GetTarget().GetFrameRecognizerManager().AddRecognizer( | ||
| std::make_shared<AbortWithPayloadFrameRecognizer>(), module_name, sym_arr, |
There was a problem hiding this comment.
Suggested change
| std::make_shared<AbortWithPayloadFrameRecognizer>(), module_name, sym_arr, | |
| std::make_shared<AbortWithPayloadFrameRecognizer>(), module_name, {sym_name}, |
This should work as well since you're not re-using the array.
| Thread *thread = frame_sp->GetThread().get(); | ||
| Process *process = thread->GetProcess().get(); | ||
|
|
||
| // FIXME: Add logging for these errors |
There was a problem hiding this comment.
I fixed the FIXME...
| Process *process = thread->GetProcess().get(); | ||
|
|
||
| // FIXME: Add logging for these errors | ||
| if (!thread) |
There was a problem hiding this comment.
You want this check before dereferencing the pointer to get the process.
| add_to_arguments(flags_key, cur_value, false); | ||
| flags_val = cur_value->GetScalar().UInt(flags_val); | ||
|
|
||
| // Okay, we've gotten all the argument values, now put then in a |
There was a problem hiding this comment.
Suggested change
| // Okay, we've gotten all the argument values, now put then in a | |
| // Okay, we've gotten all the argument values, now put them in a |
typo
| abort_dict_sp->AddStringItem(reason_key, reason_string); | ||
| abort_dict_sp->AddIntegerItem(flags_key, flags_val); | ||
|
|
||
| // This will overwrite any information in the dictionary already. |
There was a problem hiding this comment.
Is this comment accurate ? If we call AddItem it will only overwrite existing abort_with_payload item in the dictionary but not the other entries.
|
|
||
| # First check the recognized argument values: | ||
| self.assertEqual(len(arguments), 6, "Got all six values") | ||
| self.runCmd("frame variable") |
There was a problem hiding this comment.
What's the point of this ?
| arguments[1].unsigned, correct_values["code"], "code value correct" | ||
| ) | ||
|
|
||
| # FIXME: I'm always adding these recognized arguments, but that looks wrong. Should only |
| lldb::StackFrameSP &frame_sp, ValueObjectListSP &args_sp) | ||
| : RecognizedStackFrame() { | ||
| m_arguments = args_sp; | ||
| m_stop_desc = "abort with payload or reason"; |
There was a problem hiding this comment.
We could check if the payload is null and report a more accurate stop description, can't we ?
There was a problem hiding this comment.
I can't guarantee someone didn't call abort_with_payload but the payload this time happened to be null. If I really wanted to do this I'd have to crawl the stack here and see what the caller is. I also don't know that there will ever only be two API's (with_payload and with_reason) that funnel to __abort_with_payload. I'm not sure that I need to be more exact about that since the backtrace will show you what happened.
But "abort with reason and/or payload" might be a better string here.
There was a problem hiding this comment.
Yeah, that's sounds like a lot of work for little rewards.
JDevlieghere
approved these changes
Aug 1, 2024
|
FYI, this is failing on x86 and matrix macOS bots: https://green.lab.llvm.org/job/llvm.org/view/LLDB/job/lldb-cmake/4544/execution/node/97/log/?consoleFull @jimingham could you take a look? |
|
This has to read the values of the payload arguments from registers. There are 6 arguments, and x86_64 has 6 argument passing registers, so this should have worked, though I don't know how thoroughly we've tested the $arg variables beyond one or two... Anyway, I'll take a look, if it takes more that a bit, I'll disable the test for x86_64 till I can figure what's up there. |
|
I disabled the x86_64 tests for now. On Rosetta, the abort_with_payload call kills the target without returning control to the debugger, so I can't make that work. I'll have to see what happens on real x86_64 hardware. |
jimingham
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Oct 8, 2024
…payload… (llvm#101365) This is used by various system routines (the capabilities checker and dyld to name a few) to add extra color to an abort. This patch adds a frame recognizer so people can easily see the details, and also adds the information to the ExtendedCrashInformation dictionary. I also had to rework how the dictionary is held; previously it was created on demand, but that was inconvenient since it meant all the entries had to be produced at that same time. That didn't work for the recognizer.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is used by various system routines (the capabilities checker and dyld to name a few) to add extra color to an abort. This patch adds a frame recognizer so people can easily see the details, and also adds the information to the ExtendedCrashInformation dictionary.
I also had to rework how the dictionary is held; previously it was created on demand, but that was inconvenient since it meant all the entries had to be produced at that same time. That didn't work for the recognizer.