[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -831,6 +831,37 @@ bool
lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
return true;
}
+bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+PyObject *implementor, lldb::DebuggerSP debugger,
lldb_private::StructuredDataImpl _impl,
+lldb_private::CommandReturnObject _retobj,
+lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
+
+ PyErr_Cleaner py_err_cleaner(true);
+
+ PythonObject self(PyRefType::Borrowed, implementor);
+ auto pfunc = self.ResolveName("__call__");
+
+ if (!pfunc.IsAllocated())
+return false;
+
+ auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj);
bulbazord wrote:
Why are you doing this here instead of in the invocation of `pfunc` below like
you do for all the other arguments?
https://github.com/llvm/llvm-project/pull/70734
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -831,6 +831,37 @@ bool
lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
return true;
}
+bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+PyObject *implementor, lldb::DebuggerSP debugger,
lldb_private::StructuredDataImpl _impl,
+lldb_private::CommandReturnObject _retobj,
+lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
+
+ PyErr_Cleaner py_err_cleaner(true);
+
+ PythonObject self(PyRefType::Borrowed, implementor);
+ auto pfunc = self.ResolveName("__call__");
+
+ if (!pfunc.IsAllocated())
+return false;
+
+ auto cmd_retobj_arg = SWIGBridge::ToSWIGWrapper(cmd_retobj);
+
+ // FIXME:
+ // I wanted to do something like:
+ // size_t num_elem = args.size();
+ // PythonList my_list(num_elem);
+ // for (const char *elem : args)
+ // my_list.append(PythonString(elem);
+ //
+ // and then pass my_list to the pfunc, but that crashes somewhere
+ // deep in Python for reasons that aren't clear to me.
bulbazord wrote:
Do you have a backtrace or other useful info here? It would be useful to
actually write out what you want the code to be instead of this so future
contributors can reproduce the crash.
https://github.com/llvm/llvm-project/pull/70734
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
https://github.com/bulbazord commented: I did an initial pass over your patch but I didn't read the python tests yet https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -831,6 +831,37 @@ bool
lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject(
return true;
}
+bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject(
+PyObject *implementor, lldb::DebuggerSP debugger,
lldb_private::StructuredDataImpl _impl,
+lldb_private::CommandReturnObject _retobj,
+lldb::ExecutionContextRefSP exe_ctx_ref_sp) {
+
+ PyErr_Cleaner py_err_cleaner(true);
+
+ PythonObject self(PyRefType::Borrowed, implementor);
+ auto pfunc = self.ResolveName("__call__");
+
+ if (!pfunc.IsAllocated())
+return false;
bulbazord wrote:
Can we update the `CommandReturnObject` with useful feedback about what failed?
The bool return value is super opaque.
https://github.com/llvm/llvm-project/pull/70734
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -0,0 +1,315 @@ +""" +This module implements a couple of utility classes to make writing +lldb parsed commands more Pythonic. +The way to use it is to make a class for you command that inherits from ParsedCommandBase. +That will make an LLDBOVParser which you will use for your +option definition, and to fetch option values for the current invocation +of your command. Access to the OV parser is through: + +ParsedCommandBase.get_parser() + +Next, implement setup_command_definition in your new command class, and call: + + self.get_parser().add_option + +to add all your options. The order doesn't matter for options, lldb will sort them +alphabetically for you when it prints help. + +Similarly you can define the arguments with: + + self.get_parser.add_argument + +at present, lldb doesn't do as much work as it should verifying arguments, it pretty +much only checks that commands that take no arguments don't get passed arguments. + +Then implement the execute function for your command as: + +def __call__(self, debugger, args_array, exe_ctx, result): + +The arguments will be in a python array as strings. + +You can access the option values using varname you passed in when defining the option. +If you need to know whether a given option was set by the user or not, you can retrieve +the option definition array with: + + self.get_options_definition() + +look up your element by varname and check the "_value_set" element. + +There are example commands in the lldb testsuite at: + +llvm-project/lldb/test/API/commands/command/script/add/test_commands.py + +FIXME: I should make a convenient wrapper for that. +""" +import inspect +import lldb +import sys + +class LLDBOVParser: +def __init__(self): +self.options_array = [] +self.args_array = [] + +# Some methods to translate common value types. Should return a +# tuple of the value and an error value (True => error) if the +# type can't be converted. +# FIXME: Need a way to push the conversion error string back to lldb. bulbazord wrote: Instead of pushing a conversion error string, why not raise an exception? It's the pythonic way to do things. You won't have to return 2 bools then https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -0,0 +1,315 @@ +""" +This module implements a couple of utility classes to make writing +lldb parsed commands more Pythonic. +The way to use it is to make a class for you command that inherits from ParsedCommandBase. +That will make an LLDBOVParser which you will use for your +option definition, and to fetch option values for the current invocation +of your command. Access to the OV parser is through: + +ParsedCommandBase.get_parser() + +Next, implement setup_command_definition in your new command class, and call: + + self.get_parser().add_option + +to add all your options. The order doesn't matter for options, lldb will sort them +alphabetically for you when it prints help. + +Similarly you can define the arguments with: + + self.get_parser.add_argument + +at present, lldb doesn't do as much work as it should verifying arguments, it pretty +much only checks that commands that take no arguments don't get passed arguments. + +Then implement the execute function for your command as: + +def __call__(self, debugger, args_array, exe_ctx, result): + +The arguments will be in a python array as strings. + +You can access the option values using varname you passed in when defining the option. +If you need to know whether a given option was set by the user or not, you can retrieve +the option definition array with: + + self.get_options_definition() + +look up your element by varname and check the "_value_set" element. + +There are example commands in the lldb testsuite at: + +llvm-project/lldb/test/API/commands/command/script/add/test_commands.py + +FIXME: I should make a convenient wrapper for that. +""" +import inspect +import lldb +import sys + +class LLDBOVParser: +def __init__(self): +self.options_array = [] +self.args_array = [] + +# Some methods to translate common value types. Should return a +# tuple of the value and an error value (True => error) if the +# type can't be converted. +# FIXME: Need a way to push the conversion error string back to lldb. +@staticmethod +def to_bool(in_value): +error = True +value = False +low_in = in_value.lower() +if low_in == "yes" or low_in == "true" or low_in == "1": bulbazord wrote: Might be useful to recognize 'y' as well https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -831,6 +831,37 @@ bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallCommandObject( return true; } +bool lldb_private::python::SWIGBridge::LLDBSwigPythonCallParsedCommandObject( +PyObject *implementor, lldb::DebuggerSP debugger, lldb_private::StructuredDataImpl _impl, bulbazord wrote: Spelling mistake: `implementor` -> `implementer` https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
@@ -0,0 +1,315 @@ +""" +This module implements a couple of utility classes to make writing +lldb parsed commands more Pythonic. +The way to use it is to make a class for you command that inherits from ParsedCommandBase. +That will make an LLDBOVParser which you will use for your +option definition, and to fetch option values for the current invocation +of your command. Access to the OV parser is through: + +ParsedCommandBase.get_parser() + +Next, implement setup_command_definition in your new command class, and call: + + self.get_parser().add_option + +to add all your options. The order doesn't matter for options, lldb will sort them +alphabetically for you when it prints help. + +Similarly you can define the arguments with: + + self.get_parser.add_argument + +at present, lldb doesn't do as much work as it should verifying arguments, it pretty +much only checks that commands that take no arguments don't get passed arguments. + +Then implement the execute function for your command as: + +def __call__(self, debugger, args_array, exe_ctx, result): + +The arguments will be in a python array as strings. + +You can access the option values using varname you passed in when defining the option. +If you need to know whether a given option was set by the user or not, you can retrieve +the option definition array with: + + self.get_options_definition() + +look up your element by varname and check the "_value_set" element. + +There are example commands in the lldb testsuite at: + +llvm-project/lldb/test/API/commands/command/script/add/test_commands.py + +FIXME: I should make a convenient wrapper for that. bulbazord wrote: Convenient wrapper for what? https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to define a Python based command that uses CommandObjectParsed (PR #70734)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70734 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [llvm] [lldb] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
@@ -48,6 +48,7 @@ add_lldb_library(lldbSymbol NO_PLUGIN_DEPENDENCIES lldbHost lldbTarget lldbUtility +LLVMDebuginfod bulbazord wrote: Should this be in `LINK_COMPONENTS`? https://github.com/llvm/llvm-project/pull/70996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
@@ -396,8 +398,22 @@ Symbols::LocateExecutableSymbolFile(const ModuleSpec _spec, } } } - - return LocateExecutableSymbolFileDsym(module_spec); + FileSpec dsym_bundle = LocateExecutableSymbolFileDsym(module_spec); + if (dsym_bundle) +return dsym_bundle; bulbazord wrote: ```suggestion if (FileSpec dsym_bundle = LocateExecutableSymbolFileDsym(module_spec)) return dsym_bundle; ``` https://github.com/llvm/llvm-project/pull/70996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
@@ -396,8 +398,22 @@ Symbols::LocateExecutableSymbolFile(const ModuleSpec
_spec,
}
}
}
-
- return LocateExecutableSymbolFileDsym(module_spec);
+ FileSpec dsym_bundle = LocateExecutableSymbolFileDsym(module_spec);
+ if (dsym_bundle)
+return dsym_bundle;
+
+ // If we didn't find anything by looking locally, let's try Debuginfod.
+ if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
+llvm::object::BuildID build_id(module_uuid.GetBytes());
+llvm::Expected result =
+llvm::getCachedOrDownloadDebuginfo(build_id);
+if (result)
+ return FileSpec(*result);
+// An error is just fine, here...
+consumeError(result.takeError());
bulbazord wrote:
Generally I agree with this but we should probably be careful, if there's an
issue with searching then maybe we'll get hit with thousands of log messages
about the failure.
https://github.com/llvm/llvm-project/pull/70996
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
@@ -4892,6 +4894,21 @@ void TargetProperties::SetDebugUtilityExpression(bool
debug) {
SetPropertyAtIndex(idx, debug);
}
+Args TargetProperties::GetDebugInfoDURLs() const {
+ Args urls;
+ m_collection_sp->GetPropertyAtIndexAsArgs(ePropertyDebugInfoDURLs, urls);
+ return urls;
+}
+
+void TargetProperties::DebugInfoDURLsChangedCallback() {
+ Args urls = GetDebugInfoDURLs();
+ llvm::SmallVector dbginfod_urls;
+ std::transform(urls.begin(), urls.end(), dbginfod_urls.end(),
+ [](const auto ) { return obj.ref(); });
bulbazord wrote:
suggestion: `llvm::transform` will allow you to write just `urls` instead of
`urls.begin(), urls.end()`
https://github.com/llvm/llvm-project/pull/70996
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [llvm] [lldb] DEBUGINFOD based DWP acquisition for LLDB (PR #70996)
https://github.com/bulbazord commented: Out of curiosity, why did you choose the delimiter as ' ' instead of something like ';'? https://github.com/llvm/llvm-project/pull/70996 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Move register info reconfigure into architecture plugin (PR #70950)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70950 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Move register info reconfigure into architecture plugin (PR #70950)
@@ -373,14 +374,8 @@ bool GDBRemoteRegisterContext::WriteRegisterBytes(const RegisterInfo *reg_info, if (dst == nullptr) return false; - // Code below is specific to AArch64 target in SVE or SME state - // If vector granule (vg) register is being written then thread's - // register context reconfiguration is triggered on success. - // We do not allow writes to SVG so it is not mentioned here. - const ArchSpec = process->GetTarget().GetArchitecture(); - bool do_reconfigure_arm64_sve = arch.IsValid() && - arch.GetTriple().isAArch64() && - (strcmp(reg_info->name, "vg") == 0); + bool should_reconfigure_registers = bulbazord wrote: `const`? https://github.com/llvm/llvm-project/pull/70950 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][split-dwarf] Add --errors-only argument separate-debug-info list (PR #71000)
@@ -445,7 +445,11 @@ class SymbolFile : public PluginInterface {
/// contains the keys "type", "symfile", and "separate-debug-info-files".
/// "type" can be used to assume the structure of each object in
/// "separate-debug-info-files".
- virtual bool GetSeparateDebugInfo(StructuredData::Dictionary ) {
+ /// \param errors_only
+ /// If true, then only return separate debug info files that encountered
+ /// errors during loading.
+ virtual bool GetSeparateDebugInfo(StructuredData::Dictionary ,
+bool errors_only) {
bulbazord wrote:
Not sure I'm a fan of a parameter that changes behavior. Why not introduce a
new function entirely?
https://github.com/llvm/llvm-project/pull/71000
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Read SME2's ZT0 register from Linux core files (PR #70934)
@@ -339,6 +337,18 @@ bool RegisterContextCorePOSIX_arm64::ReadRegister(const
RegisterInfo *reg_info,
value.SetFromMemoryData(*reg_info, src + sizeof(sve::user_za_header),
reg_info->byte_size, lldb::eByteOrderLittle,
error);
+} else if (m_register_info_up->IsSMERegZT(reg)) {
+ value.SetFromMemoryData(*reg_info, m_zt_data.GetDataStart(),
+ reg_info->byte_size, lldb::eByteOrderLittle,
+ error);
+} else {
+ offset = reg_info->byte_offset - m_register_info_up->GetSMEOffset();
+ assert(offset < sizeof(m_sme_pseudo_regs));
bulbazord wrote:
Since you're using an assert, I'll ask:
Is this a hard error that isn't recoverable from? Or could we do something else
here?
https://github.com/llvm/llvm-project/pull/70934
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Add SME2's ZT0 register (PR #70205)
bulbazord wrote: Looks fine to me, though I'm not an expert in this area. If nobody else reviews or approves soon, I can do so. https://github.com/llvm/llvm-project/pull/70205 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Replace the usage of module imp with module importlib (PR #70443)
https://github.com/bulbazord approved this pull request. Thanks for taking the time to make sure it works with Python 3.6! I'm a big fan of this kind of modernization/cleanup work. https://github.com/llvm/llvm-project/pull/70443 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Add the ability to get a C++ vtable ValueObject from another ValueObj… (PR #67599)
bulbazord wrote: The change was not reverted upstream. It was reverted in that @DanielCChen 's llvm-project fork. Unfortunately that's the nature of GitHub. https://github.com/llvm/llvm-project/pull/67599 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Fix missing comsumeError() with LLDB_LOG in ObjectFileCOFF/PECOFF (PR #70793)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/70793 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix the DEVELOPER_DIR computation (PR #70528)
@@ -461,13 +461,11 @@ static void ParseOSVersion(llvm::VersionTuple ,
NSString *Key) {
// Invoke xcrun with the shlib dir.
if (FileSpec fspec = HostInfo::GetShlibDir()) {
if (FileSystem::Instance().Exists(fspec)) {
-std::string contents_dir =
-XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath());
-llvm::StringRef shlib_developer_dir =
-llvm::sys::path::parent_path(contents_dir);
-if (!shlib_developer_dir.empty()) {
- auto sdk =
- xcrun(sdk_name, show_sdk_path, std::move(shlib_developer_dir));
+llvm::SmallString<0> shlib_developer_dir(
+XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath()));
+llvm::sys::path::append(shlib_developer_dir, "Developer");
+if (FileSystem::Instance().Exists(shlib_developer_dir)) {
bulbazord wrote:
Do we actually need to check its existence or can `xcrun` take care of that for
us?
https://github.com/llvm/llvm-project/pull/70528
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix the DEVELOPER_DIR computation (PR #70528)
@@ -461,13 +461,11 @@ static void ParseOSVersion(llvm::VersionTuple ,
NSString *Key) {
// Invoke xcrun with the shlib dir.
if (FileSpec fspec = HostInfo::GetShlibDir()) {
if (FileSystem::Instance().Exists(fspec)) {
-std::string contents_dir =
-XcodeSDK::FindXcodeContentsDirectoryInPath(fspec.GetPath());
-llvm::StringRef shlib_developer_dir =
-llvm::sys::path::parent_path(contents_dir);
-if (!shlib_developer_dir.empty()) {
- auto sdk =
- xcrun(sdk_name, show_sdk_path, std::move(shlib_developer_dir));
+llvm::SmallString<0> shlib_developer_dir(
bulbazord wrote:
Why 0?
I assume the switch to SmallString has to do with the
`llvm::sys::path::append`, that part makes sense to me.
https://github.com/llvm/llvm-project/pull/70528
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix the DEVELOPER_DIR computation (PR #70528)
https://github.com/bulbazord approved this pull request. Makes sense to me https://github.com/llvm/llvm-project/pull/70528 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Fix the DEVELOPER_DIR computation (PR #70528)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70528 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Remove some declarations without definitions (PR #70514)
https://github.com/bulbazord approved this pull request. I can't find any uses of these either. I also checked in Apple's downstream swift fork, it doesn't look used there either. LGTM https://github.com/llvm/llvm-project/pull/70514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
https://github.com/bulbazord approved this pull request. I think all of my comments have been addressed. Thanks! https://github.com/llvm/llvm-project/pull/70392 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
@@ -104,6 +104,19 @@ ScriptInterpreter::GetStatusFromSBError(const
lldb::SBError ) const {
return Status();
}
+Event *
+ScriptInterpreter::GetOpaqueTypeFromSBEvent(const lldb::SBEvent ) const {
+ return event.m_opaque_ptr;
+}
+
+Stream *ScriptInterpreter::GetOpaqueTypeFromSBStream(
+const lldb::SBStream ) const {
+ if (stream.m_opaque_up)
+return const_cast(stream).m_opaque_up.release();
bulbazord wrote:
I'm having a hard time wrapping my head around the SBStream portion of your
patch (everything else makes sense). So we pass in a `const SBStream &` to
`GetOpaqueTypeFromSBStream` which that takes ownership of the underlying
`Stream *` and returns it. What happens to the original SBStream argument? Can
we guarantee that nothing ever tries to use it again afterwards?
It looks like the intent in `ReverseTransform` is to take ownership of the
original user-provided stream and write its contents to some other stream
`original_arg` after performing some kind of copy on it. Instead of taking
ownership of the underlying stream (and invalidating its argument), why not
just use `.get()` instead of `.release()`? The `SBStream` passed in here won't
be invalidated and `original_arg` in `ReverseTransform` can still get the data
out of the Python `SBStream` object.
What am I missing?
https://github.com/llvm/llvm-project/pull/70392
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add logging to ObjectFileMachO::ParseSymtab (PR #70490)
@@ -2652,7 +2654,9 @@ void ObjectFileMachO::ParseSymtab(Symtab ) {
std::vector external_sym_trie_entries;
std::set resolver_addresses;
- if (dyld_trie_data.GetByteSize() > 0) {
+ const size_t dyld_trie_data_size = dyld_trie_data.GetByteSize();
+ if (dyld_trie_data_size > 0) {
bulbazord wrote:
Why did you pull this out?
https://github.com/llvm/llvm-project/pull/70490
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add logging to ObjectFileMachO::ParseSymtab (PR #70490)
https://github.com/bulbazord approved this pull request. looks fine to me, one small question though https://github.com/llvm/llvm-project/pull/70490 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add logging to ObjectFileMachO::ParseSymtab (PR #70490)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70490 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
@@ -104,6 +104,19 @@ ScriptInterpreter::GetStatusFromSBError(const
lldb::SBError ) const {
return Status();
}
+Event *
+ScriptInterpreter::GetOpaqueTypeFromSBEvent(const lldb::SBEvent ) const {
+ return event.m_opaque_ptr;
+}
+
+Stream *ScriptInterpreter::GetOpaqueTypeFromSBStream(
+const lldb::SBStream ) const {
+ if (stream.m_opaque_up)
+return const_cast(stream).m_opaque_up.release();
bulbazord wrote:
This feels a little sketchy to me... `GetOpaqueTypeFromSBStream` is taking
ownership of the underlying pointer and returning it to the caller. The
`SBStream` that gets passed in is no longer valid. Is this intentional? That
would mean this function -- and only this function -- has different semantics
from the other `GetOpaqueTypeFromSBClass` functions.
https://github.com/llvm/llvm-project/pull/70392
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70392 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
@@ -0,0 +1,40 @@ +//===-- ScriptedThreadInterface.h ---*- C++ -*-===// bulbazord wrote: The header is missing the world `Plan` https://github.com/llvm/llvm-project/pull/70392 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
@@ -0,0 +1,40 @@
+//===-- ScriptedThreadInterface.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_INTERPRETER_INTERFACES_SCRIPTEDTHREADPLANINTERFACE_H
+#define LLDB_INTERPRETER_INTERFACES_SCRIPTEDTHREADPLANINTERFACE_H
+
+#include "lldb/lldb-private.h"
+
+#include "ScriptedInterface.h"
+
+namespace lldb_private {
+class ScriptedThreadPlanInterface : public ScriptedInterface {
+public:
+ virtual llvm::Expected
+ CreatePluginObject(llvm::StringRef class_name,
+ lldb::ThreadPlanSP thread_plan_sp,
+ const StructuredDataImpl _sp) {
+llvm_unreachable("unimplemented!");
+ }
+
+ virtual llvm::Expected ExplainsStop(Event *event) { return true; }
+
+ virtual llvm::Expected ShouldStop(Event *event) { return true; }
+
+ virtual llvm::Expected IsStale() { return true; };
+
+ virtual lldb::StateType GetRunState() { return lldb::eStateStepping; }
+
+ virtual llvm::Expected GetStopDescription(lldb_private::Stream *s) {
+return true;
bulbazord wrote:
Why not make all of these pure virtual methods?
https://github.com/llvm/llvm-project/pull/70392
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Make use of Scripted{Python, }Interface for ScriptedThreadPlan (PR #70392)
@@ -0,0 +1,92 @@
+//===-- ScriptedThreadPlanPythonInterface.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 "lldb/Host/Config.h"
+#include "lldb/Utility/Log.h"
+#include "lldb/lldb-enumerations.h"
+
+#if LLDB_ENABLE_PYTHON
+
+// LLDB Python header must be included first
+#include "../lldb-python.h"
+
+#include "../SWIGPythonBridge.h"
+#include "../ScriptInterpreterPythonImpl.h"
+#include "ScriptedThreadPlanPythonInterface.h"
+
+using namespace lldb;
+using namespace lldb_private;
+using namespace lldb_private::python;
+
+ScriptedThreadPlanPythonInterface::ScriptedThreadPlanPythonInterface(
+ScriptInterpreterPythonImpl )
+: ScriptedThreadPlanInterface(), ScriptedPythonInterface(interpreter) {}
+
+llvm::Expected
+ScriptedThreadPlanPythonInterface::CreatePluginObject(
+const llvm::StringRef class_name, lldb::ThreadPlanSP thread_plan_sp,
+const StructuredDataImpl _sp) {
+ return ScriptedPythonInterface::CreatePluginObject(class_name, nullptr,
+ thread_plan_sp, args_sp);
+}
+
+llvm::Expected
+ScriptedThreadPlanPythonInterface::ExplainsStop(Event *event) {
+ Status error;
+ StructuredData::ObjectSP obj = Dispatch("explains_stop", error, event);
+
+ if (!CheckStructuredDataObject(LLVM_PRETTY_FUNCTION, obj, error))
+return error.ToError();
+
+ return obj->GetBooleanValue();
+}
+
+llvm::Expected
+ScriptedThreadPlanPythonInterface::ShouldStop(Event *event) {
+ Status error;
+ StructuredData::ObjectSP obj = Dispatch("should_stop", error, event);
+
+ if (!CheckStructuredDataObject(LLVM_PRETTY_FUNCTION, obj, error))
+return error.ToError();
+
+ return obj->GetBooleanValue();
+}
+
+llvm::Expected ScriptedThreadPlanPythonInterface::IsStale() {
+ Status error;
+ StructuredData::ObjectSP obj = Dispatch("is_stale", error);
+
+ if (!CheckStructuredDataObject(LLVM_PRETTY_FUNCTION, obj, error))
+return error.ToError();
+
+ return obj->GetBooleanValue();
+}
+
+lldb::StateType ScriptedThreadPlanPythonInterface::GetRunState() {
+ Status error;
+ StructuredData::ObjectSP obj = Dispatch("should_step", error);
bulbazord wrote:
You don't need to do anything but `should_step` is so close to `should_stop`...
https://github.com/llvm/llvm-project/pull/70392
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Replace the usage of module imp with module importlib (PR #70443)
https://github.com/bulbazord commented: `importlib` has been around since Python 3.1, maybe we shouldn't have the conditional logic to use `imp` instead? I'm pretty sure the minimum supported version is 3.6 since that's what LLVM requires. https://github.com/llvm/llvm-project/pull/70443 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64][Linux] Rename IsEnabled to IsPresent (PR #70303)
@@ -610,7 +610,7 @@
NativeRegisterContextLinux_arm64::CacheAllRegisters(uint32_t _size) {
return error;
// Here this means, does the system have ZA, not whether it is active.
bulbazord wrote:
You could probably remove this comment
https://github.com/llvm/llvm-project/pull/70303
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64][Linux] Rename IsEnabled to IsPresent (PR #70303)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70303 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64][Linux] Rename IsEnabled to IsPresent (PR #70303)
https://github.com/bulbazord approved this pull request. LGTM https://github.com/llvm/llvm-project/pull/70303 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -67,18 +67,15 @@ size_t WatchpointResource::GetNumberOfOwners() {
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
- if (it != m_owners.end())
-return true;
- return false;
+ return it != m_owners.end();
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- for (WatchpointCollection::const_iterator it = m_owners.begin();
- it != m_owners.end(); ++it)
-if ((*it).get() == wp)
- return true;
- return false;
+ WatchpointCollection::const_iterator match =
bulbazord wrote:
My bad... 臘
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -67,18 +67,15 @@ size_t WatchpointResource::GetNumberOfOwners() {
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
- if (it != m_owners.end())
-return true;
- return false;
+ return it != m_owners.end();
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- for (WatchpointCollection::const_iterator it = m_owners.begin();
- it != m_owners.end(); ++it)
-if ((*it).get() == wp)
- return true;
- return false;
+ WatchpointCollection::const_iterator match =
bulbazord wrote:
Looks fine, but the type should be different right? I think you removed
`WatchpointCollection` in the 3rd commit
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
bulbazord wrote:
Well, I'm not entirely sure how we're going to want to access the Owners list,
so I'd like to understand more how you plan on using it. If you want to do
something like `wp_resource_sp->Owners()[2]` then this method makes sense to
keep around, but I'd also be curious to know why you would want to access the
3rd owner directly. What I would like to avoid is writing a loop like this:
```
for (int i = 0; i < wp_resource_sp.GetNumberOfOwners(); i++) {
WatchpointSP wp_sp = wp_resource_sp->GetOwnerAtIndex(i);
// Use wp_sp
}
```
The two main reasons I am not a fan of this pattern:
1. As you pointed out, one would need to grab the `wp_resource_sp` owner mutex
to do this safely since another thread may call `RemoveOwner` in the middle of
this loop. I can't say I'm a fan of exposing the vector primarily because it
would mean we'd have to expose the mutex too.
2. Off-by-one errors are fairly easy to put in by mistake. Doubly so if we ever
iterate in reverse (i >0 vs i >=0 for the termination condition).
If we don't need to be able to access any of the owners at an arbitrary index,
I would suggest removing it so nobody can write that kind of loop. WDYT?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Part 2 of 2 - Refactor `CommandObject::DoExecute(...)` return `void` (not `bool`) (PR #69991)
bulbazord wrote: > > > I think I looked at every changed line, looks good to me overall. Found > > > one place with a small style issue but it's not a blocker. Thanks! > > > > > > Thanks @bulbazord! Do you know why the `code_formatter` check/bot didn't > > flag that or the other one-line `if` statements? > > This could be because it's part of the LLVM style guide but not enforced in > the `.clang-format`. > > Also looked at most of the lines, LGTM. I wonder however why did you decide > to make 2 separate PRs for this ? Is there any reason for this ? It could > have been 2 commits in the same PR. Yeah, I think it's probably not enforced. Maybe it should be? 樂 https://github.com/llvm/llvm-project/pull/69991 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)
@@ -129,8 +129,19 @@ void DebugNamesDWARFIndex::GetGlobalVariables(
DWARFUnit , llvm::function_ref callback) {
uint64_t cu_offset = cu.GetOffset();
bool found_entry_for_cu = false;
- for (const DebugNames::NameIndex : *m_debug_names_up) {
-for (DebugNames::NameTableEntry nte: ni) {
+ for (const DebugNames::NameIndex : *m_debug_names_up) {
+// Check if this name index contains an entry for the given CU.
+bool cu_matches = false;
+for (uint32_t i = 0; i < ni.getCUCount(); ++i) {
+ if (ni.getCUOffset(i) == cu_offset) {
+cu_matches = true;
+break;
+ }
+}
+if (!cu_matches)
+ continue;
bulbazord wrote:
It'd be great if we could use some kind of `find_if` instead of manually
walking the CU Offsets.
https://github.com/llvm/llvm-project/pull/70231
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70231 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Improve debug names index fetching global variables performance (PR #70231)
https://github.com/bulbazord commented: This patch looks fine to me in idea, but next time please leave your review up for longer so others have time to take a look as well. https://github.com/llvm/llvm-project/pull/70231 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Add SME2's ZT0 register (PR #70205)
@@ -625,6 +662,18 @@
NativeRegisterContextLinux_arm64::CacheAllRegisters(uint32_t _size) {
error = ReadZA();
if (error.Fail())
return error;
+
+// We will only be restoring ZT data if ZA is active. As writing to an
+// inactive ZT enables ZA, which may not be desireable.
+if (GetRegisterInfo().IsZTEnabled() &&
+m_za_header.size > sizeof(m_za_header)) {
bulbazord wrote:
The comment and the code are a little confusing to me. Reading the code, it's
not obvious that `GetRegisterInfo().IsZTEnabled() && m_za_header.size >
sizeof(m_za_header)` checks to see if ZA is active. This reads to me like "if
ZT0 is enabled and the za header is filled in correctly".
Why is "is ZT0 enabled" a condition if we just need to check ZA? Sorry if this
is a noob question, I'm trying to learn more about this so I can help review
these patches.
I also see you doing this below, might be good to build an abstraction for this?
https://github.com/llvm/llvm-project/pull/70205
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Add SME2's ZT0 register (PR #70205)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/70205 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][lldb-server] Enable sending RegisterFlags as XML (PR #69951)
https://github.com/bulbazord approved this pull request. LGTM. I didn't realize lldb-server didn't use libXML, learned something new today. :) https://github.com/llvm/llvm-project/pull/69951 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
bulbazord wrote:
It should take advantage of copy elision. Specifically we should be able to
take advantage of NRVO.
https://en.cppreference.com/w/cpp/language/copy_elision
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
bulbazord wrote:
`WatchpointCollection` got removed right? I think you'll need to rewrite it...
I'd suggest using `find_if`
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
}
bool WatchpointResource::OwnersContains(const Watchpoint *wp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp);
+ for (WatchpointCollection::const_iterator it = m_owners.begin();
+ it != m_owners.end(); ++it)
+if ((*it).get() == wp)
+ return true;
+ return false;
}
WatchpointSP WatchpointResource::GetOwnerAtIndex(size_t idx) {
std::lock_guard guard(m_owners_mutex);
- assert(idx < m_owners.GetSize());
- if (idx >= m_owners.GetSize())
+ lldbassert(idx < m_owners.size());
+ if (idx >= m_owners.size())
return {};
- return m_owners.GetByIndex(idx);
+ return m_owners[idx];
bulbazord wrote:
Looking over this again, is there a use case where you would want to reference
a specific owner by index instead of using `Owners()`?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -53,44 +49,52 @@ bool WatchpointResource::Contains(addr_t addr) {
void WatchpointResource::AddOwner(const WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Add(wp_sp);
+ m_owners.push_back(wp_sp);
}
void WatchpointResource::RemoveOwner(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- m_owners.Remove(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+m_owners.erase(it);
}
size_t WatchpointResource::GetNumberOfOwners() {
std::lock_guard guard(m_owners_mutex);
- return m_owners.GetSize();
+ return m_owners.size();
}
bool WatchpointResource::OwnersContains(WatchpointSP _sp) {
std::lock_guard guard(m_owners_mutex);
- return m_owners.Contains(wp_sp);
+ const auto = std::find(m_owners.begin(), m_owners.end(), wp_sp);
+ if (it != m_owners.end())
+return true;
+ return false;
bulbazord wrote:
`return it != m_owners.end();`
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
Oh wait, the mutex is to protect the Owners. Nevermind!
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
Ah I could have sworn the mutex was marked as `mutable`. How are things like
`GetType` and `GetByteSize` marked `const` then? Do they not lock?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Interpreter] Make ScriptedInterface Object creation more generic (PR #68052)
https://github.com/bulbazord commented: Works for me generally. I'm curious to know if some of these can be pure virtual function as Jonas mentioned. https://github.com/llvm/llvm-project/pull/68052 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Interpreter] Make ScriptedInterface Object creation more generic (PR #68052)
@@ -32,6 +32,84 @@ class ScriptedPythonInterface : virtual public
ScriptedInterface {
ScriptedPythonInterface(ScriptInterpreterPythonImpl );
~ScriptedPythonInterface() override = default;
+ template
+ llvm::Expected
+ CreatePluginObject(llvm::StringRef class_name,
+ StructuredData::Generic *script_obj, Args... args) {
+using namespace python;
+using Locker = ScriptInterpreterPythonImpl::Locker;
+
+std::string error_string;
+if (class_name.empty() &&
+llvm::StringRef(m_interpreter.GetDictionaryName()).empty() &&
+!script_obj)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "ScriptedPythonInterface::CreatePluginObject - missing script class "
+ "name, dictionary or object.");
+
+Locker py_lock(_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+ Locker::FreeLock);
+
+PythonObject result = {};
+
+if (!script_obj) {
+ auto dict =
+ PythonModule::MainModule().ResolveName(
+ m_interpreter.GetDictionaryName());
+ auto pfunc =
+ PythonObject::ResolveNameWithDictionary(
+ class_name, dict);
+
+ if (!pfunc.IsAllocated()) {
+error_string.append("Could not find script class: ");
+error_string.append(class_name);
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ error_string);
+ }
+
+ std::tuple original_args = std::forward_as_tuple(args...);
+ auto transformed_args = TransformArgs(original_args);
+
+ llvm::Expected arg_info = pfunc.GetArgInfo();
+ if (!arg_info) {
+llvm::handleAllErrors(
+arg_info.takeError(),
+[&](PythonException ) { error_string.append(E.ReadBacktrace());
},
+[&](const llvm::ErrorInfoBase ) {
+ error_string.append(E.message());
+});
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ error_string);
+ }
+
+ llvm::Expected expected_return_object =
+ llvm::make_error("Not initialized.",
+ llvm::inconvertibleErrorCode());
+
+ std::apply(
+ [, _return_object](auto &&...args) {
+llvm::consumeError(expected_return_object.takeError());
+expected_return_object = pfunc(args...);
+ },
+ transformed_args);
+
+ if (llvm::Error e = expected_return_object.takeError())
+return e;
+ result = std::move(expected_return_object.get());
+} else
+ result = PythonObject(PyRefType::Borrowed,
+static_cast(script_obj->GetValue()));
+
+if (!result.IsValid())
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "python object result is invalid.");
bulbazord wrote:
You could augment this error to say something like "The resulting object from
the call to $NAME is not a valid Python Object" or something to this effect
https://github.com/llvm/llvm-project/pull/68052
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Interpreter] Make ScriptedInterface Object creation more generic (PR #68052)
@@ -32,6 +32,84 @@ class ScriptedPythonInterface : virtual public
ScriptedInterface {
ScriptedPythonInterface(ScriptInterpreterPythonImpl );
~ScriptedPythonInterface() override = default;
+ template
+ llvm::Expected
+ CreatePluginObject(llvm::StringRef class_name,
+ StructuredData::Generic *script_obj, Args... args) {
+using namespace python;
+using Locker = ScriptInterpreterPythonImpl::Locker;
+
+std::string error_string;
+if (class_name.empty() &&
+llvm::StringRef(m_interpreter.GetDictionaryName()).empty() &&
+!script_obj)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "ScriptedPythonInterface::CreatePluginObject - missing script class "
+ "name, dictionary or object.");
+
+Locker py_lock(_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+ Locker::FreeLock);
+
+PythonObject result = {};
+
+if (!script_obj) {
+ auto dict =
+ PythonModule::MainModule().ResolveName(
+ m_interpreter.GetDictionaryName());
bulbazord wrote:
Do we need to check the validity of the dictionary?
https://github.com/llvm/llvm-project/pull/68052
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Interpreter] Make ScriptedInterface Object creation more generic (PR #68052)
@@ -32,6 +32,84 @@ class ScriptedPythonInterface : virtual public
ScriptedInterface {
ScriptedPythonInterface(ScriptInterpreterPythonImpl );
~ScriptedPythonInterface() override = default;
+ template
+ llvm::Expected
+ CreatePluginObject(llvm::StringRef class_name,
+ StructuredData::Generic *script_obj, Args... args) {
+using namespace python;
+using Locker = ScriptInterpreterPythonImpl::Locker;
+
+std::string error_string;
+if (class_name.empty() &&
+llvm::StringRef(m_interpreter.GetDictionaryName()).empty() &&
+!script_obj)
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "ScriptedPythonInterface::CreatePluginObject - missing script class "
+ "name, dictionary or object.");
+
+Locker py_lock(_interpreter, Locker::AcquireLock | Locker::NoSTDIN,
+ Locker::FreeLock);
+
+PythonObject result = {};
+
+if (!script_obj) {
+ auto dict =
+ PythonModule::MainModule().ResolveName(
+ m_interpreter.GetDictionaryName());
+ auto pfunc =
+ PythonObject::ResolveNameWithDictionary(
+ class_name, dict);
+
+ if (!pfunc.IsAllocated()) {
+error_string.append("Could not find script class: ");
+error_string.append(class_name);
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ error_string);
+ }
+
+ std::tuple original_args = std::forward_as_tuple(args...);
+ auto transformed_args = TransformArgs(original_args);
+
+ llvm::Expected arg_info = pfunc.GetArgInfo();
+ if (!arg_info) {
+llvm::handleAllErrors(
+arg_info.takeError(),
+[&](PythonException ) { error_string.append(E.ReadBacktrace());
},
+[&](const llvm::ErrorInfoBase ) {
+ error_string.append(E.message());
+});
+return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ error_string);
+ }
+
+ llvm::Expected expected_return_object =
+ llvm::make_error("Not initialized.",
+ llvm::inconvertibleErrorCode());
+
+ std::apply(
+ [, _return_object](auto &&...args) {
+llvm::consumeError(expected_return_object.takeError());
+expected_return_object = pfunc(args...);
+ },
+ transformed_args);
+
+ if (llvm::Error e = expected_return_object.takeError())
+return e;
+ result = std::move(expected_return_object.get());
+} else
+ result = PythonObject(PyRefType::Borrowed,
+static_cast(script_obj->GetValue()));
bulbazord wrote:
+1
https://github.com/llvm/llvm-project/pull/68052
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb/Interpreter] Make ScriptedInterface Object creation more generic (PR #68052)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/68052 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Part 2 of 2 - Refactor `CommandObject::DoExecute(...)` return `void` (not `bool`) (PR #69991)
@@ -34,16 +34,17 @@ CommandObjectMultipleThreads::CommandObjectMultipleThreads(
m_arguments.push_back({thread_arg});
}
-bool CommandObjectIterateOverThreads::DoExecute(Args ,
+void CommandObjectIterateOverThreads::DoExecute(Args ,
CommandReturnObject ) {
result.SetStatus(m_success_return);
bool all_threads = false;
if (command.GetArgumentCount() == 0) {
Thread *thread = m_exe_ctx.GetThreadPtr();
-if (!thread || !HandleOneThread(thread->GetID(), result))
- return false;
-return result.Succeeded();
+if (thread) {
+ HandleOneThread(thread->GetID(), result);
+}
bulbazord wrote:
LLVM style guidelines say no braces for single-line block in if/else/loop
constructs
https://github.com/llvm/llvm-project/pull/69991
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Part 2 of 2 - Refactor `CommandObject::DoExecute(...)` return `void` (not `bool`) (PR #69991)
https://github.com/bulbazord approved this pull request. I think I looked at every changed line, looks good to me overall. Found one place with a small style issue but it's not a blocker. Thanks! https://github.com/llvm/llvm-project/pull/69991 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Part 2 of 2 - Refactor `CommandObject::DoExecute(...)` return `void` (not `bool`) (PR #69991)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/69991 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Part 1 of 2 - Refactor `CommandObject::Execute(...)` return `void` (not `bool`) (PR #69989)
https://github.com/bulbazord approved this pull request. LGTM with everyone else's comments. Thanks for working on this! https://github.com/llvm/llvm-project/pull/69989 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][AArch64] Correct type of 32 bit GPR RegisterValues when using core files (PR #70054)
https://github.com/bulbazord approved this pull request. Since this bug only really manifests with later changes, I think it's fine to fix it without a test for now. Please make sure there is a test that exercises this behavior in your follow-up. https://github.com/llvm/llvm-project/pull/70054 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -34,7 +34,7 @@ struct CompilerContext {
}
bool operator!=(const CompilerContext ) const { return !(*this == rhs); }
- void Dump() const;
+ void Dump(Stream *s) const;
bulbazord wrote:
Yes, we basically always want the stream to be a valid pointer, a reference
makes more sense here.
https://github.com/llvm/llvm-project/pull/69981
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Reintroduce accidentally deleted "protected" keyword in SymbolFileDWARF (PR #69990)
@@ -321,6 +322,7 @@ class SymbolFileDWARF : public SymbolFileCommon {
m_file_index = file_index;
}
+protected:
bulbazord wrote:
Which things are you using specifically? If you want to use them out of tree,
we should think more carefully about which symbols actually make sense to
expose.
https://github.com/llvm/llvm-project/pull/69990
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
+values_ss.Printf("old value: %s", old_summary_cstr);
+ else {
+StreamString strm;
+DumpValueObjectOptions options;
+options.SetUseDynamicType(eNoDynamicValues)
+.SetHideRootType(true)
+.SetHideRootName(true)
+.SetHideName(true);
+m_old_value_sp->Dump(strm, options);
+if (strm.GetData())
+ values_ss.Printf("old value: %s", strm.GetData());
+ }
}
}
if (m_new_value_sp) {
+if (values_ss.GetSize())
+ values_ss.Printf("\n");
+
const char *new_value_cstr = m_new_value_sp->GetValueAsCString();
-if (new_value_cstr && new_value_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_value_cstr);
+if (new_value_cstr)
+ values_ss.Printf("new value: %s", new_value_cstr);
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -491,14 +491,13 @@ static StopInfoSP GetStopInfoForHardwareBP(Thread
, Target *target,
uint64_t exc_sub_sub_code) {
// Try hardware watchpoint.
if (target) {
+// LWP_TODO: We need to find the WatchpointResource that matches
bulbazord wrote:
`LWP_TODO` doesn't mean much to a lot of contributors, maybe you can change
this to something like `TODO(Large Watchpoint Support)`? Mostly to avoid
acronyms that aren't written down anywhere else.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -3105,102 +3122,184 @@ Status
ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) {
}
// Pre-requisite: wp != NULL.
-static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
- assert(wp);
- bool watch_read = wp->WatchpointRead();
- bool watch_write = wp->WatchpointWrite();
- bool watch_modify = wp->WatchpointModify();
-
- // watch_read, watch_write, watch_modify cannot all be false.
- assert((watch_read || watch_write || watch_modify) &&
- "watch_read, watch_write, watch_modify cannot all be false.");
- if (watch_read && (watch_write || watch_modify))
+static GDBStoppointType
+GetGDBStoppointType(const WatchpointResourceSP _res_sp) {
+ assert(wp_res_sp);
+ bool read, write;
+ wp_res_sp->GetType(read, write);
+
+ assert((read || write) && "read and write cannot both be false.");
+ if (read && write)
return eWatchpointReadWrite;
- else if (watch_read)
+ else if (read)
return eWatchpointRead;
- else // Must be watch_write or watch_modify, then.
+ else
return eWatchpointWrite;
}
-Status ProcessGDBRemote::EnableWatchpoint(Watchpoint *wp, bool notify) {
+Status ProcessGDBRemote::EnableWatchpoint(WatchpointSP wp_sp, bool notify) {
Status error;
- if (wp) {
-user_id_t watchID = wp->GetID();
-addr_t addr = wp->GetLoadAddress();
+ if (wp_sp) {
bulbazord wrote:
Since you're already refactoring this part, maybe you could flip the condition
and do the error handling up front. It's small, but it saves a level of
indentation in an already large code block.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
+values_ss.Printf("old value: %s", old_summary_cstr);
+ else {
+StreamString strm;
+DumpValueObjectOptions options;
+options.SetUseDynamicType(eNoDynamicValues)
+.SetHideRootType(true)
+.SetHideRootName(true)
+.SetHideName(true);
+m_old_value_sp->Dump(strm, options);
+if (strm.GetData())
+ values_ss.Printf("old value: %s", strm.GetData());
+ }
}
}
if (m_new_value_sp) {
+if (values_ss.GetSize())
+ values_ss.Printf("\n");
+
const char *new_value_cstr = m_new_value_sp->GetValueAsCString();
-if (new_value_cstr && new_value_cstr[0])
- s->Printf("\n%snew value: %s", prefix, new_value_cstr);
+if (new_value_cstr)
+ values_ss.Printf("new value: %s", new_value_cstr);
else {
const char *new_summary_cstr = m_new_value_sp->GetSummaryAsCString();
- if (new_summary_cstr && new_summary_cstr[0])
-s->Printf("\n%snew value: %s", prefix, new_summary_cstr);
+ if (new_summary_cstr)
+values_ss.Printf("new value: %s", new_summary_cstr);
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,90 @@
+//===-- WatchpointCollection.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 "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadSpec.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// WatchpointCollection constructor
+WatchpointCollection::WatchpointCollection() = default;
+
+// Destructor
+WatchpointCollection::~WatchpointCollection() = default;
+
+void WatchpointCollection::Add(const WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.push_back(wp);
+}
+
+bool WatchpointCollection::Remove(WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ for (collection::iterator pos = m_collection.begin();
+ pos != m_collection.end(); ++pos) {
+if (*pos == wp) {
+ m_collection.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+WatchpointSP WatchpointCollection::GetByIndex(size_t i) {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+const WatchpointSP WatchpointCollection::GetByIndex(size_t i) const {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+WatchpointCollection &
+WatchpointCollection::operator=(const WatchpointCollection ) {
+ if (this != ) {
+std::lock(m_collection_mutex, rhs.m_collection_mutex);
+std::lock_guard lhs_guard(m_collection_mutex, std::adopt_lock);
+std::lock_guard rhs_guard(rhs.m_collection_mutex,
+ std::adopt_lock);
bulbazord wrote:
You can use `std::scoped_lock` for this now that we're using C++17!
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -3105,102 +3122,184 @@ Status
ProcessGDBRemote::DisableBreakpointSite(BreakpointSite *bp_site) {
}
// Pre-requisite: wp != NULL.
-static GDBStoppointType GetGDBStoppointType(Watchpoint *wp) {
- assert(wp);
- bool watch_read = wp->WatchpointRead();
- bool watch_write = wp->WatchpointWrite();
- bool watch_modify = wp->WatchpointModify();
-
- // watch_read, watch_write, watch_modify cannot all be false.
- assert((watch_read || watch_write || watch_modify) &&
- "watch_read, watch_write, watch_modify cannot all be false.");
- if (watch_read && (watch_write || watch_modify))
+static GDBStoppointType
+GetGDBStoppointType(const WatchpointResourceSP _res_sp) {
+ assert(wp_res_sp);
+ bool read, write;
+ wp_res_sp->GetType(read, write);
+
+ assert((read || write) && "read and write cannot both be false.");
bulbazord wrote:
The assertion message here could be fleshed out a bit more. Something like
`"WatchpointResource type is neither read nor write"`.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
bulbazord wrote:
Instead of exposing this primitive, maybe it would make more sense to return an
iterator or an iterator range?
Something like:
```
IteratorRange Owners();
```
IMO this would make it easier to write bug-free code since we're not going to
have to manually set up index iteration logic.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
bulbazord wrote:
Why return the size instead of returning just a new `WatchpointCollection`
object? That way callers don't have to create a new one and pass it in, they
can just grab it from this method directly.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord commented: A lot of the `WatchpointResourceList::FindBy` methods can probably be rewritten with `std::find_if` (or `llvm::find_if` which is a little easier to use IMO). Overall, looks like a pretty nice mostly-NFC refactor! https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,90 @@
+//===-- WatchpointCollection.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 "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/Breakpoint/Watchpoint.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Target/Thread.h"
+#include "lldb/Target/ThreadSpec.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// WatchpointCollection constructor
+WatchpointCollection::WatchpointCollection() = default;
+
+// Destructor
+WatchpointCollection::~WatchpointCollection() = default;
+
+void WatchpointCollection::Add(const WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.push_back(wp);
+}
+
+bool WatchpointCollection::Remove(WatchpointSP ) {
+ std::lock_guard guard(m_collection_mutex);
+ for (collection::iterator pos = m_collection.begin();
+ pos != m_collection.end(); ++pos) {
+if (*pos == wp) {
+ m_collection.erase(pos);
+ return true;
+}
+ }
+ return false;
+}
+
+WatchpointSP WatchpointCollection::GetByIndex(size_t i) {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+const WatchpointSP WatchpointCollection::GetByIndex(size_t i) const {
+ std::lock_guard guard(m_collection_mutex);
+ if (i < m_collection.size())
+return m_collection[i];
+ return {};
+}
+
+WatchpointCollection &
+WatchpointCollection::operator=(const WatchpointCollection ) {
+ if (this != ) {
+std::lock(m_collection_mutex, rhs.m_collection_mutex);
+std::lock_guard lhs_guard(m_collection_mutex, std::adopt_lock);
+std::lock_guard rhs_guard(rhs.m_collection_mutex,
+ std::adopt_lock);
+m_collection = rhs.m_collection;
+ }
+ return *this;
+}
+
+void WatchpointCollection::Clear() {
+ std::lock_guard guard(m_collection_mutex);
+ m_collection.clear();
+}
+
+bool WatchpointCollection::Contains(const WatchpointSP _sp) {
+ std::lock_guard guard(m_collection_mutex);
+ const size_t size = m_collection.size();
+ for (size_t i = 0; i < size; ++i) {
+if (m_collection[i] == wp_sp)
+ return true;
+ }
bulbazord wrote:
Why not use iterators here? You can also use `llvm::is_contained` too.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
+
+ /// This method returns the Watchpoint at index \a index using this
+ /// Resource. The owners are listed ordinally from 0 to
+ /// GetNumberOfOwners() - 1 so you can use this method to iterate over the
+ /// owners.
+ ///
+ /// \param[in] idx
+ /// The index in the list of owners for which you wish the owner
location.
+ ///
+ /// \return
+ ///The Watchpoint at that index.
+ lldb::WatchpointSP GetOwnerAtIndex(size_t idx);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp_sp
+ /// The WatchpointSP to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(lldb::WatchpointSP _sp);
+
+ /// Check if the owners includes a watchpoint.
+ ///
+ /// \param[in] wp
+ /// The Watchpoint to search for.
+ ///
+ /// \result
+ /// true if this resource's owners includes the watchpoint.
+ bool OwnersContains(const lldb_private::Watchpoint *wp);
+
+ /// This method copies the watchpoint resource's owners into a new
collection.
+ /// It does this while the owners mutex is locked.
+ ///
+ /// \param[out] out_collection
+ ///The BreakpointLocationCollection into which to put the owners
+ ///of this breakpoint site.
+ ///
+ /// \return
+ ///The number of elements copied into out_collection.
+ size_t CopyOwnersList(WatchpointCollection _collection);
+
+ // The ID of the WatchpointResource is set by the WatchpointResourceList
+ // when the Resource has been set in the inferior and is being added
+ // to the List, in an attempt to match the hardware watchpoint register
+ // ordering. If a Process can correctly identify the hardware watchpoint
+ // register index when it has created the Resource, it may initialize it
+ // before it is inserted in the WatchpointResourceList.
+ void SetID(lldb::wp_resource_id_t);
+
+ lldb::wp_resource_id_t GetID() const;
+
+ bool Contains(lldb::addr_t addr);
+
+protected:
+ // The StopInfoWatchpoint knows when it is processing a hit for a thread for
+ // a site, so let it be the one to manage setting the location hit count once
+ // and only once.
+ friend class StopInfoWatchpoint;
+
+ void BumpHitCounts();
+
+private:
+ lldb::wp_resource_id_t m_id;
+
+ // start address & size aligned & expanded to be a valid watchpoint
+ // memory granule on this target.
+ lldb::addr_t m_addr;
+ size_t m_size;
+
+ bool m_watch_read; // true if we stop when the watched data is read from
+ bool m_watch_write; // true if we stop when the watched data is written to
bulbazord wrote:
IMO these comments are unnecessary, the names of the variables already describe
the intended semantics quite well I think.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -266,33 +268,73 @@ void Watchpoint::Dump(Stream *s) const {
// If prefix is nullptr, we display the watch id and ignore the prefix
// altogether.
-void Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
- if (!prefix) {
-s->Printf("\nWatchpoint %u hit:", GetID());
-prefix = "";
- }
+bool Watchpoint::DumpSnapshots(Stream *s, const char *prefix) const {
+ bool printed_anything = false;
+
+ // For read watchpoints, don't display any before/after value changes.
+ if (m_watch_read && !m_watch_modify && !m_watch_write)
+return printed_anything;
+
+ s->Printf("\n");
+ s->Printf("Watchpoint %u hit:\n", GetID());
+
+ StreamString values_ss;
+ if (prefix)
+values_ss.Indent(prefix);
if (m_old_value_sp) {
const char *old_value_cstr = m_old_value_sp->GetValueAsCString();
-if (old_value_cstr && old_value_cstr[0])
- s->Printf("\n%sold value: %s", prefix, old_value_cstr);
-else {
+if (old_value_cstr) {
+ values_ss.Printf("old value: %s", old_value_cstr);
+} else {
const char *old_summary_cstr = m_old_value_sp->GetSummaryAsCString();
- if (old_summary_cstr && old_summary_cstr[0])
-s->Printf("\n%sold value: %s", prefix, old_summary_cstr);
+ if (old_summary_cstr)
bulbazord wrote:
Same here
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
bulbazord wrote:
Suggestion: Return `std::pair` instead of passing out parameters.
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
+
+ lldb::addr_t GetAddress() const;
+
+ size_t GetByteSize() const;
+
+ void GetType(bool , bool ) const;
+
+ void SetType(bool read, bool write);
+
+ /// The "Owners" are the watchpoints that share this resource.
+ /// The method adds the \a owner to this resource's owner list.
+ ///
+ /// \param[in] owner
+ ///\a owner is the Wachpoint to add.
+ void AddOwner(const lldb::WatchpointSP );
+
+ /// The method removes the owner at \a owner from this watchpoint
+ /// resource.
+ void RemoveOwner(lldb::WatchpointSP );
+
+ /// This method returns the number of Watchpoints currently using
+ /// watchpoint resource.
+ ///
+ /// \return
+ ///The number of owners.
+ size_t GetNumberOfOwners();
bulbazord wrote:
This can be `const`-qualified right?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
@@ -0,0 +1,140 @@
+//===-- WatchpointResource.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_BREAKPOINT_WATCHPOINTRESOURCE_H
+#define LLDB_BREAKPOINT_WATCHPOINTRESOURCE_H
+
+#include "lldb/Breakpoint/WatchpointCollection.h"
+#include "lldb/lldb-public.h"
+
+#include
+
+namespace lldb_private {
+
+class WatchpointResource
+: public std::enable_shared_from_this {
+
+public:
+ // Constructors and Destructors
+ WatchpointResource(lldb::addr_t addr, size_t size, bool read, bool write);
+
+ ~WatchpointResource();
+
+ void GetMemoryRange(lldb::addr_t , size_t ) const;
bulbazord wrote:
Instead of out parameters, you could do `std::pair`.
But why have this and `GetAddress()` + `GetByteSize()`?
https://github.com/llvm/llvm-project/pull/68845
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] [mostly NFC] Large WP foundation: WatchpointResources (PR #68845)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/68845 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -142,8 +142,7 @@ TypeSP DWARFASTParserClang::ParseTypeFromClangModule(const SymbolContext , // If this type comes from a Clang module, recursively look in the // DWARF section of the .pcm file in the module cache. Clang // generates DWO skeleton units as breadcrumbs to find them. - llvm::SmallVector decl_context; - die.GetDeclContext(decl_context); + std::vector decl_context = die.GetDeclContext(); bulbazord wrote: Why the change from `llvm::SmallVector<$Type, 4>` to `std::vector`? Does this not generally contain 4 or fewer CompilerContexts anymore? https://github.com/llvm/llvm-project/pull/69981 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -533,6 +533,12 @@ class Function : public UserID, public SymbolContextScope {
/// The DeclContext, or NULL if none exists.
CompilerDeclContext GetDeclContext();
+ /// Get the CompilerContext for this function, if available.
+ ///
+ /// \return
+ /// The CompilerContext, or NULL if none exists.
+ std::vector GetCompilerContext();
bulbazord wrote:
The docs and the declaration conflict.
https://github.com/llvm/llvm-project/pull/69981
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -1391,7 +1391,16 @@
SymbolFileDWARFDebugMap::GetDeclContextContainingUID(lldb::user_id_t type_uid) {
SymbolFileDWARF *oso_dwarf = GetSymbolFileByOSOIndex(oso_idx);
if (oso_dwarf)
return oso_dwarf->GetDeclContextContainingUID(type_uid);
- return CompilerDeclContext();
+ return {};
+}
+
+std::vector
+SymbolFileDWARFDebugMap::GetCompilerContextForUID(lldb::user_id_t type_uid) {
+ const uint64_t oso_idx = GetOSOIndexFromUserID(type_uid);
+ SymbolFileDWARF *oso_dwarf = GetSymbolFileByOSOIndex(oso_idx);
+ if (oso_dwarf)
bulbazord wrote:
You can roll these lines into 1
```
if (SymbolFileDWARF *oso_dwarf = GetSymbolFileByOSOIndex(oso_idx))
```
https://github.com/llvm/llvm-project/pull/69981
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -816,6 +818,8 @@ Expected
opts::symbols::getAction() {
"Specify search type (-find) to use search options.");
return dumpModule;
+
+
bulbazord wrote:
Spurious whitespace
https://github.com/llvm/llvm-project/pull/69981
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
@@ -519,7 +530,17 @@ CompilerDeclContext Function::GetDeclContext() {
if (SymbolFile *sym_file = module_sp->GetSymbolFile())
return sym_file->GetDeclContextForUID(GetID());
}
- return CompilerDeclContext();
+ return {};
+}
+
+std::vector Function::GetCompilerContext() {
+ ModuleSP module_sp = CalculateSymbolContextModule();
+
+ if (module_sp) {
bulbazord wrote:
You can roll these into 1 line as well.
https://github.com/llvm/llvm-project/pull/69981
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
https://github.com/bulbazord commented: overall LGTM, few questions though https://github.com/llvm/llvm-project/pull/69981 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] Expose DWARFDIE::GetDeclContext() in lldb_private::Function. (PR #69981)
https://github.com/bulbazord edited https://github.com/llvm/llvm-project/pull/69981 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add `SBModule.SetLocateDwoCallback` (PR #69517)
bulbazord wrote: In that case, maybe it doesn't make sense for every Debugger to have its own callback and SymbolFile should own it? Can't say I'm a fan of having this effectively be a global variable since I've seen it be an issue in other situations (e.g. Logging). https://github.com/llvm/llvm-project/pull/69517 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Refactor InstrumentationRuntimeAsan and add a new plugin (PR #69388)
https://github.com/bulbazord approved this pull request. Thanks for breaking it up, that made it easier for me to review. I don't see any issues with it right now, so LGTM. https://github.com/llvm/llvm-project/pull/69388 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][docs] Update contributing links (PR #69726)
https://github.com/bulbazord approved this pull request. https://github.com/llvm/llvm-project/pull/69726 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb] Add a single bit constructor for RegisterFlags::Field (PR #69315)
https://github.com/bulbazord approved this pull request. Sorry for the delay, this looks fine to me. I don't have much to say about your RFC (since it's not something I have a lot of expertise on) but I look forward to seeing it get implemented! https://github.com/llvm/llvm-project/pull/69315 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB] Update breakpoint-command.test to use string instead of number. (PR #69796)
@@ -1,7 +1,7 @@ # REQUIRES: lua || python # RUN: %build %p/Inputs/dummy-target.c -o %t.out -# RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(95000 + 126)"' -o 'r' | FileCheck %s +# RUN: %lldb %t.out -o 'b main' -o 'break command add 1 -o "script print(\"A R4Nd0m\" + \" Str1ng\")"' -o 'r' | FileCheck %s bulbazord wrote: Interesting, is there a specific reason for doing string concatenation here? Or just keeping the `+` from the previous version? https://github.com/llvm/llvm-project/pull/69796 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [LLDB] Update breakpoint-command.test to use string instead of number. (PR #69796)
https://github.com/bulbazord approved this pull request. What an interesting failure condition Some arbitrary string with spaces seems a lot less likely to be a false positive compared to a number. https://github.com/llvm/llvm-project/pull/69796 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
