This revision was automatically updated to reflect the committed changes.
Closed by commit rL316609: Move StopInfoOverride callback to the new
architecture plugin (authored by labath).
Changed prior to commit:
https://reviews.llvm.org/D31172?vs=106600&id=120309#toc
Repository:
rL LLVM
https
zturner accepted this revision.
zturner added a comment.
I will ping them for some numbers and more details of their test setup.
Regardless, I didn't mean to derail the code review. But, I really really want
to reach a point where we can stop falling back on the "we need to be safe even
in th
jasonmolenda added a comment.
> There was a talk at cppcon a few weeks ago from someone at backtrace.io who
> had some insightful metrics on debugger performance memory consumption, and
> LLDB had ~2x the memory consumption of GDB.
I haven't seen the paper, but my guess is that this is on linux
clayborg added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
zturner wrote:
> clayborg wrote:
> > zturner wrote:
> > > clayborg wrote:
> > > > One time at startup. No threads contending yet.
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> zturner wrote:
> > clayborg wrote:
> > > One time at startup. No threads contending yet. Asking for plug-in by
clayborg added inline comments.
Comment at: source/Core/PluginManager.cpp:282
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
zturner wrote:
> clayborg wrote:
> > We need "std::string" since it owns t
zturner added inline comments.
Comment at: source/Plugins/Architecture/Arm/ArchitectureArm.cpp:23
+ return ConstString("arm");
+}
+
clayborg wrote:
> One time at startup. No threads contending yet. Asking for plug-in by name is
> made fast for later. I would le
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
zturner wrote:
> labath wrote:
> > clayborg wrote:
> > > zturner wrote:
> > > > Why do we need a `ConstStr
clayborg added inline comments.
Comment at: include/lldb/Target/Target.h:1217
+const ArchSpec &GetSpec() const { return m_spec; }
+Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
zturner wrote:
> Can this return a reference instead of a po
zturner added inline comments.
Comment at: source/Core/PluginManager.cpp:281-282
+struct ArchitectureInstance {
+ ConstString name;
+ std::string description;
+ PluginManager::ArchitectureCreateInstance create_callback;
labath wrote:
> zturner wrote:
> > Why d
labath added inline comments.
Comment at: include/lldb/Target/Target.h:1217
+const ArchSpec &GetSpec() const { return m_spec; }
+Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
zturner wrote:
> Can this return a reference instead of a poin
zturner added a comment.
I know you're doing things the way it's always been done, but I want to start
questioning some long-standing practices :) So I'm not picking on you
specifically, but anywhere we can migrate towards something better
incrementally, I think we should do so.
===
Looks fine. We can start with this. I was thinking it would be nice to lazily
populate m_plugin_up, but then we would need to add a bit to see if we already
tried to look for it, so the current approach will work fine.
> On Oct 24, 2017, at 1:22 PM, Pavel Labath via Phabricator
> wrote:
>
> l
clayborg accepted this revision.
clayborg added a comment.
Looks fine. We can iterate on this.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath added a comment.
I'm back now, and I'd like to try to push this patch to completion.
After re-reading the discussion, I got the impression we have mostly reached a
consensus here. A small issue remained about how to guarantee that the
Architecture plugin and the ArchSpec object are in sy
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 met
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/Cor
clayborg added a comment.
In https://reviews.llvm.org/D31172#808105, @labath wrote:
> In https://reviews.llvm.org/D31172#808038, @clayborg wrote:
>
> > Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if
> > the arch is supported. Most of the time we will set this once an
labath added a comment.
In https://reviews.llvm.org/D31172#808038, @clayborg wrote:
> Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if
> the arch is supported. Most of the time we will set this once and never need
> to change it, even when we attach, change arch, etc.
clayborg added a comment.
Added a suggestion to make Target::GetArchitecturePlugin lazy and to ask if the
arch is supported. Most of the time we will set this once and never need to
change it, even when we attach, change arch, etc. The new suggestion will allow
us to use the same arch plug-in w
labath updated this revision to Diff 106398.
labath added a comment.
Moving the plugin to Target and other minor fixups.
I also wrote a test for the functionality covered by the override callback. (I'm
not including it here to avoid polluting the review, but a part of this change
will be to move
labath marked 2 inline comments as done.
labath added a comment.
In https://reviews.llvm.org/D31172#807180, @jingham wrote:
> Sorry I wasn't clear. I meant that since the Target knows everything it
> needs to know to vend the correct Architecture plugin, you should get it from
> the Target, no
jingham added a comment.
What Greg said...
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jingham added a comment.
Sorry I wasn't clear. I meant that since the Target knows everything it needs
to know to vend the correct Architecture plugin, you should get it from the
Target, not the Process. In general, I think that the highest class in the
stack that can vend a plugin is the one
clayborg added a comment.
I think is saying we should just store the Architecture plug-in in the target
instead of in the process. It will also need to be cleared when the target arch
is changed.
https://reviews.llvm.org/D31172
___
lldb-commits ma
labath added a comment.
In https://reviews.llvm.org/D31172#806804, @jingham wrote:
> This is a good addition. However, it seems to me that since you only need an
> ArchSpec to make one of these Architecture plugins, which plugin you get
> seems fully determined by the Target, not the Process.
jingham added a comment.
This is a good addition. However, it seems to me that since you only need an
ArchSpec to make one of these Architecture plugins, which plugin you get seems
fully determined by the Target, not the Process. I understand that the only
need for it at present is to do a Pr
clayborg added a comment.
A few inline fixes, but this looks great. Very close
Comment at: include/lldb/Core/ArchSpec.h:26-30
namespace lldb_private {
class Platform;
-}
-namespace lldb_private {
class Stream;
-}
-namespace lldb_private {
class StringList;
}
--
labath updated this revision to Diff 106165.
labath added a comment.
Herald added subscribers: javed.absar, mgorny.
The architecture plugin idea
https://reviews.llvm.org/D31172
Files:
include/lldb/Core/ArchSpec.h
include/lldb/Core/Architecture.h
include/lldb/Core/PluginManager.h
include
labath commandeered this revision.
labath edited reviewers, added: zturner; removed: labath.
labath added a comment.
It looks like this has ground to a halt, but it seems the consensus was to try
the Architecture plugin idea. I'm going to hijack this revision to maintain
context, and upload a ne
jingham added a comment.
Yes, that seems like the cleanest solution.
https://reviews.llvm.org/D31172
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
clayborg added a comment.
It almost seems like we need some sort of an architecture plug-in here. Maybe
something like Architecture plugins. The Architecture::FindPlugin() would take
an ArchSpec and return a lldb_private::Architecture class instance that can be
cached in the target or process.
Thanks for the suggestions! Not sure if I'll be able to make those changes
before tomorrow EOD, and after that I will be out for ~2 weeks, so if time
doesn't permit, I may not follow up with another patch until after I get
back.
On Tue, Mar 21, 2017 at 11:19 AM Jim Ingham via Phabricator <
revi..
jingham added a comment.
None of this isn't modeling the situation particularly clearly, since we have
"architecture specific modifications to general behaviors" that aren't coming
in through plugins so that it would be easy to modify the behavior for new
architectures. Instead, we're requiring
zturner added a comment.
The architecture help change snuck in, I already removed it locally but didn't
re-upload.
As for why I'm trying to get this out of `ArchSpec`, it turns out `ArchSpec` is
one of the biggest causes of dependency cycles. It's used all over the place,
which triggers a dep
jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
It looks like you are bundling two changes into one patch, one to move the
StopInfoOverrideCallback from ArchSpec to Process, and one to change how the
help for the list of archite
zturner created this revision.
The only real consumer of this is the `Process` class, so it makes sense (both
conceptually and from a layering standpoint) to hide this in the Process plugin.
This is intended to be NFC.
https://reviews.llvm.org/D31172
Files:
lldb/include/lldb/Core/ArchSpec.h
37 matches
Mail list logo
