Re: [Lldb-commits] [lldb] r247852 - Add an OperatingSystem plugin to support goroutines
Hi Jason,
I believe Ryan is no longer working for google. Since the go plugin
was his effort, I think that unfortunately leaves our go support
without a maintainer. (Unless he happens to be reading this from a
personal email). I have also recently disabled one of the go tests
because it was failing with go 1.8 (in wasn't crashing though, so I'm
not sure if it is related to your problem).
I'm not sure if we have a policy on what to do in these cases, but I
propose the following course of action:
- disable goroutine support via the setting you mentioned
- send out a call for maintainers to lldb-dev (I can forward that to
google go team, to see if anyone wants to pick up that effort)
- if nobody responds within a certain timeframe, delete the relevant code
On 14 July 2017 at 04:58, Jason Molenda via lldb-commits
wrote:
> Hi Ryan, we're having a tiny problem with the Go OperatingSystem plugin at
> Apple. I know it's been a while since you've worked on this, but I wanted to
> see if you had an opinion about how we should address this.
>
> Some iPhone app developers download static libraries (aka ranlib archives aka
> collection of .o files) to build into their projects, and we've seen a few
> static libraries that are written in Go. I have no idea what compilers were
> used to generate these object files.
>
> We go through the OperatingSystemGo::Init and find
>
> ValueObjectSP allgs_sp = FindGlobal(target_sp, "runtime.allgs");
> if (allgs_sp) {
> m_allg_sp = allgs_sp->GetChildMemberWithName(ConstString("array"), true);
> m_allglen_sp = allgs_sp->GetChildMemberWithName(ConstString("len"), true);
>
> Then when we're working on threads (and the plugin.os.goroutines.enable
> setting is enabled), OperatingSystemGo::UpdateThreadList is called to add
> extra threads ("goroutines"? sorry I haven't had a chance to play with the
> language yet, I'm not familiar with the terminology) into lldb's thread list.
> It starts by getting the runtime.allgs.len:
>
> uint64_t allglen = m_allglen_sp->GetValueAsUnsigned(0);
>
> and then iterates over that many goroutines:
>
> Error err;
> for (uint64_t i = 0; i < allglen; ++i) {
> goroutines.push_back(CreateGoroutineAtIndex(i, err));
> if (err.Fail()) {
> err.PutToLog(log, "OperatingSystemGo::UpdateThreadList");
> return new_thread_list.GetSize(false) > 0;
> }
>
> and in OperatingSystemGo::CreateGoroutineAtIndex we do
>
> ValueObjectSP g =
> m_allg_sp->GetSyntheticArrayMember(idx, true)->Dereference(err);
>
>
> and this is where we're crashing. For some reason the runtime.allgs.len says
> it has 8 but m_allg_sp->GetSyntheticArrayMember(0, true) returns an empty
> ValueObjectSP (shared pointer) so we crash when we dereference it.
>
> Of course the real question is why does m_allg_sp->GetSyntheticArrayMember(0,
> true) not return a child -- it should have 8 of them according to
> runtime.allgs.len -- but I'm not super interested in this, the people who are
> developing/debugging these apps are unaware that they have go code in their
> program and are not going to be debugging into it.
>
>
> I can avoid the crash by checking to see if we were able to fetch the child.
> If not, set the error object and return. UpdateThreadList() detects the
> error and doesn't add any goroutines to the lldb thread list. e.g.
>
> diff --git i/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> w/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> index c5a3ddee484..97875c7266d 100644
> --- i/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> +++ w/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> @@ -451,8 +451,14 @@ OperatingSystemGo::Goroutine
> OperatingSystemGo::CreateGoroutineAtIndex(uint64_t idx, Error &err) {
>err.Clear();
>Goroutine result = {};
> - ValueObjectSP g =
> - m_allg_sp->GetSyntheticArrayMember(idx, true)->Dereference(err);
> + ValueObjectSP child_sp = m_allg_sp->GetSyntheticArrayMember(idx, true);
> + if (!child_sp) {
> +err.SetErrorToGenericError();
> +err.SetErrorString("unable to find goroutines in array");
> +return result;
> + }
> +
> + ValueObjectSP g = child_sp->Dereference(err);
>if (err.Fail()) {
> return result;
>}
>
>
> I could also just disable goroutines in the lldb that we ship in Xcode. I'd
> rather not do that because it means people working in go on macos/ios need to
> re-enable it in their .lldbinit files before they get the expected debug
> experience.
>
>
> If you have a chance to look at this, could you let me know what you think is
> best here?
>
>
> Thanks
>
> Jason
>
>
>
>
>> On Sep 16, 2015, at 2:20 PM, Ryan Brown via lldb-commits
>> wrote:
>>
>> Author: ribrdb
>> Date: Wed Sep 16 16:20:44 2015
>> New Revision: 247852
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=247852&view=rev
>> Log:
>> Add an OperatingSystem plugin to support goroutines
>>
>> The Go runtime schedules user level threads (goroutines)
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
labath updated this revision to Diff 106600.
labath marked an inline comment as done.
labath added a comment.
The version with a container object for ArchSpec+Plugin combo
https://reviews.llvm.org/D31172
Files:
include/lldb/Core/ArchSpec.h
include/lldb/Core/Architecture.h
include/lldb/Core/PluginManager.h
include/lldb/Target/Process.h
include/lldb/Target/Target.h
packages/Python/lldbsuite/test/arm/breakpoint-it/Makefile
packages/Python/lldbsuite/test/arm/breakpoint-it/TestBreakpointIt.py
packages/Python/lldbsuite/test/arm/breakpoint-it/main.c
source/API/SystemInitializerFull.cpp
source/Core/ArchSpec.cpp
source/Core/PluginManager.cpp
source/Plugins/Architecture/Arm/ArchitectureArm.cpp
source/Plugins/Architecture/Arm/ArchitectureArm.h
source/Plugins/Architecture/Arm/CMakeLists.txt
source/Plugins/Architecture/CMakeLists.txt
source/Plugins/CMakeLists.txt
source/Target/Process.cpp
source/Target/Target.cpp
source/Target/Thread.cpp
Index: source/Target/Thread.cpp
===
--- source/Target/Thread.cpp
+++ source/Target/Thread.cpp
@@ -442,10 +442,9 @@
if (m_stop_info_override_stop_id != process_stop_id) {
m_stop_info_override_stop_id = process_stop_id;
if (m_stop_info_sp) {
-ArchSpec::StopInfoOverrideCallbackType callback =
-GetProcess()->GetStopInfoOverrideCallback();
-if (callback)
- callback(*this);
+if (Architecture *arch =
+process_sp->GetTarget().GetArchitecturePlugin())
+ arch->OverrideStopInfo(*this);
}
}
}
Index: source/Target/Target.cpp
===
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -26,6 +26,7 @@
#include "lldb/Core/Event.h"
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
+#include "lldb/Core/PluginManager.h"
#include "lldb/Core/Section.h"
#include "lldb/Core/SourceManager.h"
#include "lldb/Core/State.h"
@@ -65,6 +66,16 @@
constexpr std::chrono::milliseconds EvaluateExpressionOptions::default_timeout;
+Target::Arch::Arch(const ArchSpec &spec)
+: m_spec(spec),
+ m_plugin_up(PluginManager::CreateArchitectureInstance(spec)) {}
+
+const Target::Arch& Target::Arch::operator=(const ArchSpec &spec) {
+ m_spec = spec;
+ m_plugin_up = PluginManager::CreateArchitectureInstance(spec);
+ return *this;
+}
+
ConstString &Target::GetStaticBroadcasterClass() {
static ConstString class_name("lldb.target");
return class_name;
@@ -76,12 +87,12 @@
Broadcaster(debugger.GetBroadcasterManager(),
Target::GetStaticBroadcasterClass().AsCString()),
ExecutionContextScope(), m_debugger(debugger), m_platform_sp(platform_sp),
- m_mutex(), m_arch(target_arch), m_images(this), m_section_load_history(),
- m_breakpoint_list(false), m_internal_breakpoint_list(true),
- m_watchpoint_list(), m_process_sp(), m_search_filter_sp(),
- m_image_search_paths(ImageSearchPathsChanged, this), m_ast_importer_sp(),
- m_source_manager_ap(), m_stop_hooks(), m_stop_hook_next_id(0),
- m_valid(true), m_suppress_stop_hooks(false),
+ m_mutex(), m_arch(target_arch),
+ m_images(this), m_section_load_history(), m_breakpoint_list(false),
+ m_internal_breakpoint_list(true), m_watchpoint_list(), m_process_sp(),
+ m_search_filter_sp(), m_image_search_paths(ImageSearchPathsChanged, this),
+ m_ast_importer_sp(), m_source_manager_ap(), m_stop_hooks(),
+ m_stop_hook_next_id(0), m_valid(true), m_suppress_stop_hooks(false),
m_is_dummy_target(is_dummy_target)
{
@@ -96,10 +107,11 @@
Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
if (log)
log->Printf("%p Target::Target()", static_cast(this));
- if (m_arch.IsValid()) {
-LogIfAnyCategoriesSet(
-LIBLLDB_LOG_TARGET, "Target::Target created with architecture %s (%s)",
-m_arch.GetArchitectureName(), m_arch.GetTriple().getTriple().c_str());
+ if (target_arch.IsValid()) {
+LogIfAnyCategoriesSet(LIBLLDB_LOG_TARGET,
+ "Target::Target created with architecture %s (%s)",
+ target_arch.GetArchitectureName(),
+ target_arch.GetTriple().getTriple().c_str());
}
}
@@ -243,7 +255,7 @@
m_valid = false;
DeleteCurrentProcess();
m_platform_sp.reset();
- m_arch.Clear();
+ m_arch = ArchSpec();
ClearModules(true);
m_section_load_history.Clear();
const bool notify = false;
@@ -1226,8 +1238,8 @@
void Target::DidExec() {
// When a process exec's we need to know about it so we can do some cleanup.
- m_breakpoint_list.RemoveInvalidLocations(m_arch);
- m_internal_breakpoint_list.RemoveInvalidLocations(m_arch);
+ m_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
+ m_internal_breakpoint_list.RemoveInvalidLocations(m_arch.GetSpec());
}
void Target::Se
[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process
labath added a comment. In https://reviews.llvm.org/D31172#808177, @clayborg wrote: > In https://reviews.llvm.org/D31172#808105, @labath wrote: > > > - confine all the changes to m_arch to the SetArchitecture method. Right > > now only one of the assignments to m_arch is done outside of this method, > > and that one can be easily converted to a call to SetArchitecture(). This > > should make sure the two values never get out of sync. > > > That would be fine too. I've tried that, and eventually decided against it, as it would create an uncomfortable mutual recursion between SetArchitecture and SetExecutableModule. > > >> - (a more extreme version of the first idea) store the ArchSpec inside the >> architecture plugin (then use m_architecture_up->GetArchSpec() instead of >> m_arch). This will make it impossible for the values to go out of sync, as >> we will have a single source of truth. (We will need a default arch plugin >> for cases where we don't have a specific plugin) > > It would need to be the other way around. Store the Architecture plug-in > inside of the ArchSpec since we only have the "arm" Architecture plug-in and > architectures don't need to make one of these unless the arch has special > things it needs to do. We can't have ArchSpec vend the plugin, as that would cause ArchSpec to depend on the whole world again (which is what we are trying to remove by this). I don't see why the plugin could not hold the ArchSpec -- for architectures which don't need special processing, we could just have a default dumb "plugin" that does nothing but vend the ArchSpec. The alternative would be to have a third object which holds both ArchSpec and the plugin, and whose only job is to make sure these two are in sync -- it would basically be a glorified std::pair with a custom assignment operator. I think this would actually be a fairly clean solution -- the only problem I have is that I don't know how to name the new class. I'll upload it so you can see how it looks like. https://reviews.llvm.org/D31172 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [lldb] r247852 - Add an OperatingSystem plugin to support goroutines
Thanks Pavel, I got a bounce from his work email too. I agree with your plan
for this, I think this is the first time that we have a plugin/target without
an active maintainer that may be bitrotting or in need of maintenance. I was
experimenting with disabling the goroutine threads, the change for that is just
diff --git i/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
w/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
index c5a3ddee484..704f38121dd 100644
--- i/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
+++ w/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
@@ -47,7 +47,7 @@ using namespace lldb_private;
namespace {
static PropertyDefinition g_properties[] = {
- {"enable", OptionValue::eTypeBoolean, true, true, nullptr, nullptr,
+ {"enable", OptionValue::eTypeBoolean, true, false, nullptr, nullptr,
"Specify whether goroutines should be treated as threads."},
{NULL, OptionValue::eTypeInvalid, false, 0, NULL, NULL, NULL}};
I see a lot of people developing with go, I would not be surprised if we get
some motivated go developers who will fix up the existing plugin for newer
runtimes eventually. As long as it's not crashing, I'm in favor of leaving the
plugins enabled as much as we can.
On 07/14/17 02:01 AM, Pavel Labath wrote:
>
> Hi Jason,
>
> I believe Ryan is no longer working for google. Since the go plugin
> was his effort, I think that unfortunately leaves our go support
> without a maintainer. (Unless he happens to be reading this from a
> personal email). I have also recently disabled one of the go tests
> because it was failing with go 1.8 (in wasn't crashing though, so I'm
> not sure if it is related to your problem).
>
> I'm not sure if we have a policy on what to do in these cases, but I
> propose the following course of action:
> - disable goroutine support via the setting you mentioned
> - send out a call for maintainers to lldb-dev (I can forward that to
> google go team, to see if anyone wants to pick up that effort)
> - if nobody responds within a certain timeframe, delete the relevant code
>
>
> On 14 July 2017 at 04:58, Jason Molenda via lldb-commits
> wrote:
> > Hi Ryan, we're having a tiny problem with the Go OperatingSystem plugin at
> > Apple. I know it's been a while since you've worked on this, but I wanted
> > to see if you had an opinion about how we should address this.
> >
> > Some iPhone app developers download static libraries (aka ranlib archives
> > aka collection of .o files) to build into their projects, and we've seen a
> > few static libraries that are written in Go. I have no idea what compilers
> > were used to generate these object files.
> >
> > We go through the OperatingSystemGo::Init and find
> >
> > ValueObjectSP allgs_sp = FindGlobal(target_sp, "runtime.allgs");
> > if (allgs_sp) {
> > m_allg_sp = allgs_sp->GetChildMemberWithName(ConstString("array"), true);
> > m_allglen_sp = allgs_sp->GetChildMemberWithName(ConstString("len"), true);
> >
> > Then when we're working on threads (and the plugin.os.goroutines.enable
> > setting is enabled), OperatingSystemGo::UpdateThreadList is called to add
> > extra threads ("goroutines"? sorry I haven't had a chance to play with the
> > language yet, I'm not familiar with the terminology) into lldb's thread
> > list. It starts by getting the runtime.allgs.len:
> >
> > uint64_t allglen = m_allglen_sp->GetValueAsUnsigned(0);
> >
> > and then iterates over that many goroutines:
> >
> > Error err;
> > for (uint64_t i = 0; i < allglen; ++i) {
> > goroutines.push_back(CreateGoroutineAtIndex(i, err));
> > if (err.Fail()) {
> > err.PutToLog(log, "OperatingSystemGo::UpdateThreadList");
> > return new_thread_list.GetSize(false) > 0;
> > }
> >
> > and in OperatingSystemGo::CreateGoroutineAtIndex we do
> >
> > ValueObjectSP g =
> > m_allg_sp->GetSyntheticArrayMember(idx, true)->Dereference(err);
> >
> >
> > and this is where we're crashing. For some reason the runtime.allgs.len
> > says it has 8 but m_allg_sp->GetSyntheticArrayMember(0, true) returns an
> > empty ValueObjectSP (shared pointer) so we crash when we dereference it.
> >
> > Of course the real question is why does
> > m_allg_sp->GetSyntheticArrayMember(0, true) not return a child -- it should
> > have 8 of them according to runtime.allgs.len -- but I'm not super
> > interested in this, the people who are developing/debugging these apps are
> > unaware that they have go code in their program and are not going to be
> > debugging into it.
> >
> >
> > I can avoid the crash by checking to see if we were able to fetch the
> > child. If not, set the error object and return. UpdateThreadList() detects
> > the error and doesn't add any goroutines to the lldb thread list. e.g.
> >
> > diff --git i/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> > w/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp
> > index c5a3ddee484..97875c7266d 100644
> > --- i/source/Plugins/OperatingSystem/Go/Operating
[Lldb-commits] [PATCH] D33213: Use socketpair on all Unix platforms
DemiMarie updated this revision to Diff 106708.
DemiMarie added a comment.
Make --fd an alias for --pipe
https://reviews.llvm.org/D33213
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
===
--- tools/lldb-server/lldb-gdbserver.cpp
+++ tools/lldb-server/lldb-gdbserver.cpp
@@ -97,6 +97,7 @@
{"attach", required_argument, NULL, 'a'},
{"named-pipe", required_argument, NULL, 'N'},
{"pipe", required_argument, NULL, 'U'},
+{"fd", required_argument, NULL, 'F'},
{"native-regs", no_argument, NULL,
'r'}, // Specify to use the native registers instead of the gdb defaults
// for the architecture. NOTE: this is a do-nothing arg as it's
@@ -401,6 +402,7 @@
break;
case 'U': // unnamed pipe
+case 'F':
if (optarg && optarg[0])
unnamed_pipe_fd = StringConvert::ToUInt32(optarg, -1);
break;
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3287,7 +3287,7 @@
}
return error;
}
-#if defined(__APPLE__)
+#ifndef _WIN32
#define USE_SOCKETPAIR_FOR_LOCAL_CONNECTION 1
#endif
Index: tools/lldb-server/lldb-gdbserver.cpp
===
--- tools/lldb-server/lldb-gdbserver.cpp
+++ tools/lldb-server/lldb-gdbserver.cpp
@@ -97,6 +97,7 @@
{"attach", required_argument, NULL, 'a'},
{"named-pipe", required_argument, NULL, 'N'},
{"pipe", required_argument, NULL, 'U'},
+{"fd", required_argument, NULL, 'F'},
{"native-regs", no_argument, NULL,
'r'}, // Specify to use the native registers instead of the gdb defaults
// for the architecture. NOTE: this is a do-nothing arg as it's
@@ -401,6 +402,7 @@
break;
case 'U': // unnamed pipe
+case 'F':
if (optarg && optarg[0])
unnamed_pipe_fd = StringConvert::ToUInt32(optarg, -1);
break;
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -3287,7 +3287,7 @@
}
return error;
}
-#if defined(__APPLE__)
+#ifndef _WIN32
#define USE_SOCKETPAIR_FOR_LOCAL_CONNECTION 1
#endif
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33213: Use socketpair on all Unix platforms
DemiMarie added a comment. In https://reviews.llvm.org/D33213#806807, @clayborg wrote: > Ah yes, I thought there was something missing... I just made `--fd` an alias for `--pipe`. https://reviews.llvm.org/D33213 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33213: Use socketpair on all Unix platforms
clayborg accepted this revision. clayborg added a comment. Let Pavel try this and verify and this is good to go. https://reviews.llvm.org/D33213 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D33213: Use socketpair on all Unix platforms
clayborg added a comment. Any news on any speed changes? Please post the output of: (lldb) process plugin packet speed-test Both without and with this change. Nice to track any perf improvements https://reviews.llvm.org/D33213 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
