This is an archive of the discontinued LLVM Phabricator instance.

Move stop info override callback code from ArchSpec into Process
ClosedPublic

Authored by labath on Mar 20 2017, 11:51 PM.

Details

Summary

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.

Diff Detail

Event Timeline

zturner created this revision.Mar 20 2017, 11:51 PM
jingham requested changes to this revision.Mar 21 2017, 10:14 AM
jingham added a subscriber: jingham.

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 architectures is computed. Are these linked in some way I'm missing?

I don't have a strong opinion about whether this callback belongs in ArchSpec, but I don't think it belongs in Process. We've been trying hard to keep the algorithms in Process architecture independent, and that seems a worthy goal since otherwise it's going to be too easy to let little architecture specific hacks creep in and make the logic harder to follow. If you have some strong reason to take this out of ArchSpec then maybe we need another container for architecture specific algorithms. But I don't think Process is the right place for this.

This revision now requires changes to proceed.Mar 21 2017, 10:14 AM

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 dependency from Everywhere -> Core, and then it depends on Target, a couple of Plugins, and Host. There's only a very small amount of code in ArchSpec that accounts for all of these dependencies though. The first is this code that I'm attempting to move, the second is the code to set the triple given a Platform. So I wanted to tackle these one at a time.

One option would be to make lldb/Process/StopInfoOverride.h and cpp, and put the code there. Thoughts?

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 new architectures to go in and edit supposedly generic code. We put off that abstraction because it was unclear what we would need it to do, and because of that for the most part we just put these architecture specific behaviors inline in various places. Then this one callback had nowhere to go but ArchSpec which is the only logical place for architecture specific code to accumulate. It doesn't seem like we should gate cleaning up ArchSpec on that larger issue, however.

Rather than introducing another file, it seems simpler to make this part of StopInfo, since it is logically modifying the StopInfo for a thread. It isn't right there, but it's any worse, and StopInfo is closer to the machine (though so far mostly closer to the Platform) than Process should be. And StopInfo's already have to know about pretty much all the details of Process/Thread/etc... so you wouldn't be adding knowledge to them that isn't appropriate.

I'd also suggest removing the "callback" name from it since it really isn't a general purpose callback, it is absolutely determined by the ArchSpec. For instance if you were to use anything but the ARM one for ARM debugging you would break debugging. It's not optional... Instead I'd call it something like InvokeStopInfoArchOverride. That will also make it clear that this is a target for gathering up into some "ArchSpec specific behaviors" if/when we get around to that.

clayborg edited edge metadata.Mar 23 2017, 10:27 AM

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. Currently the class would look like:

class Architecture : public PluginInterface {
  Error StopInfoOverride(...);
};

Then using the target's arch, it can lookup the right one and cache it in the target or process?

Yes, that seems like the cleanest solution.

labath commandeered this revision.Jul 12 2017, 3:01 AM
labath edited reviewers, added: zturner; removed: labath.
labath edited edge metadata.

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 new version implementing that.

labath updated this revision to Diff 106165.Jul 12 2017, 3:02 AM
labath edited edge metadata.

The architecture plugin idea

A few inline fixes, but this looks great. Very close

include/lldb/Core/ArchSpec.h
26–30 ↗(On Diff #106165)

This should actually be removed and replaced with a lldb-forward.h file. I know it was wrong to begin with, but we should fix it as long as we are making changes.

include/lldb/Core/Architecture.h
17 ↗(On Diff #106165)

This should inherit from PluginInterface so we can extract the plug-in name from it if needed for logging.

19 ↗(On Diff #106165)

Not sure the constructor is needed?

jingham added a comment.EditedJul 12 2017, 10:41 AM

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 Process-related task. But that task actually takes a Thread as a parameter, so the Architecture plugin doesn't need to know it's containing process to do its job. And it seems like it diminishes the plugin's future utility to have it more limited in scope than it needs to be.

What do you think?

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 Process-related task. But that task actually takes a Thread as a parameter, so the Architecture plugin doesn't need to know it's containing process to do its job. And it seems like it diminishes the plugin's future utility to have it more limited in scope than it needs to be.

What do you think?

I'm not sure I understand what you are proposing here.

Is it to have the architecture plugin store a Target as a member variable? If that's the case, then I'd say let's wait until a need for that arises. I am not fundamentally against the idea, but I don't see a reason to add it while we don't have a need for it.

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.

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 that should.

What Greg said...

labath marked 2 inline comments as done.Jul 13 2017, 4:16 AM

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 that should.

Ah, got it. That sounds like a good idea, I'll switch this over to Target.

include/lldb/Core/Architecture.h
19 ↗(On Diff #106165)

It is, because the default constructor is not emitted automatically if you provide a copy constructor (even a deleted one).

labath updated this revision to Diff 106398.Jul 13 2017, 4:19 AM

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 testcases/arm_emulation to testcases/arm/emulation to make space
for the new arm-specific test).

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 without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

include/lldb/Target/Target.h
916 ↗(On Diff #106398)

We might want to make this lazy? Only fill it in if anyone asks. Another way to ensure this stays up to date it to ask it if it supports the target's arch each time.

Architecture *Target::GetArchitecturePlugin() { 
  if (m_architecture_up && !m_architecture_up->Supports(m_arch))
    m_architecture_up.reset();
  if (!m_architecture_up)
    m_architecture_up = PluginManager::CreateArchitectureInstance(m_arch);
  return m_architecture_up.get(); 
}

Then we never need to worry about clearing this or updating it when the arch changes. MacOS is the typical place this will happen where the user types:

(lldb) file a.out
(lldb) attach 123

And where a.out has 2 architectures: i386 and x86_64. By default we will load the 64 bit arch, but when we attach we might end up switching. Same thing for exec. We have a mac test case for debugging through exec calls where we switch between i386 and x86_64 a few times to ensure breakpoints correctly unresolve and re-resolve.

source/Plugins/Architecture/Arm/ArchitectureArm.cpp
22 ↗(On Diff #106398)

Other plug-ins have typically stuck with lower case names and very simple. I would suggest simply "arm" as the name. Each plug-in has its own namespace, so the names can be easy and short.

source/Target/Target.cpp
1253 ↗(On Diff #106398)

We can avoid this if we change Target::GetArchitecturePlugin as noted above.

1318 ↗(On Diff #106398)

We can avoid this if we change Target::GetArchitecturePlugin as noted above.

1334 ↗(On Diff #106398)

We can avoid this if we change Target::GetArchitecturePlugin as noted above.

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 without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

Well... I don't feel strongly about it, but it's not my preferred solution (the IsSupported() call feels hacky). But since we're already throwing ideas out there, here are two of mine:

  • 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.
  • (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)

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 without re-fetching it. Let me know what you think as the changes are thrown out there to see what people's opinions are.

Well... I don't feel strongly about it, but it's not my preferred solution (the IsSupported() call feels hacky). But since we're already throwing ideas out there, here are two of mine:

  • 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.

  • (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.

I am fine with either approach and fine not doing that approach I suggested. I just don't want this to ever get out of sync. So maybe the easiest way it to ask the ArchSpec object for it's architecture plug-in? If there is no state maintained in these plug-ins, we might just have a single instance of each Architecture plug-in that everyone uses (across targets)? Then everyone doesn't need to hold onto a shared/unique_ptr and can just fetch it from the Plugin manager each time?

labath updated this revision to Diff 106600.Jul 14 2017, 2:53 AM
labath marked an inline comment as done.

The version with a container object for ArchSpec+Plugin combo

  • 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.

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 sync. Several versions were thrown around, with no clear conclusion.

Does anyone hav objections to how this part is implemented in the last version of the patch? If not, I'd like to put this in...

clayborg accepted this revision.Oct 24 2017, 1:45 PM

Looks fine. We can iterate on this.

zturner edited edge metadata.Oct 24 2017, 2:08 PM

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.

include/lldb/Target/Target.h
1213–1214 ↗(On Diff #106600)

Add a move constructor maybe?

1217 ↗(On Diff #106600)

Can this return a reference instead of a pointer?

source/Core/PluginManager.cpp
281–282 ↗(On Diff #106600)

Why do we need a ConstString and a std::string here? I don't think any plugin ever has a dynamically generated name or description, they exclusively originate from string literals. So, with that in mind, can both of these just be StringRef?

318–323 ↗(On Diff #106600)

These mutexes have always bothered me. Are people really registering and unregistering plugins dynamically? It seems to me all of this always happens at debugger startup. Are the mutexes necessary?

source/Plugins/Architecture/Arm/ArchitectureArm.cpp
21–23 ↗(On Diff #106600)

Again, doesn't make much sense to me why these need to be const-ified. They're all string literals anyway, this is just introducing unnecessary locking and contention on the global string pool.

source/Plugins/Architecture/Arm/ArchitectureArm.h
26 ↗(On Diff #106600)

Can you forward declare Thread in this file?

source/Target/Target.cpp
90–95 ↗(On Diff #106600)

Can probably delete all of these initializations that are just invoking default constructors, and move the rest to inline class member initialization.

labath added inline comments.Oct 24 2017, 2:26 PM
include/lldb/Target/Target.h
1217 ↗(On Diff #106600)

It can't, because the architecture plugin is not always present (currently we only have the arm one, and I currently don't see a use for others). We could consider a no-op plugin, which gets used if no specific plugin is present. This would even avoid the need for this class, as then the plugin could be responsible for returning the archspec.

source/Core/PluginManager.cpp
281–282 ↗(On Diff #106600)

They, could be, but I don't see a way to enforce that the names come from string literals, which makes me wonder if the usage of StringRef is a win in this case...

IIRC, the main reason I made this ConstString (instead of std::string, like the description), is that this name eventually makes it's way to the PluginInterface virtual method (which cannot be changed without touching every plugin).

318–323 ↗(On Diff #106600)

I'm pretty sure they aren't needed. I was just following what the other plugins do.

zturner added inline comments.Oct 24 2017, 2:34 PM
source/Core/PluginManager.cpp
281–282 ↗(On Diff #106600)

I don't think we need to enforce it. Documentation is good enough. If someone accepts a StringRef to a function, you, as the caller of the function, cannot enforce what they do with it. They could store a pointer to it. That doesn't mean you should never pass in a std::string and let the implicit conversion happen, it just means you have to follow the contract. Same could happen if your function took a const std::string&.
Documenting the top level interface to say "The memory for this string is assumed to live for the life of the program" should be sufficient.

If someone needs to do something funky and construct a name dynamically, they can make their plugin contain an llvm::StringSaver and get a stable pointer that way.

318–323 ↗(On Diff #106600)

So, follow up question... Can we remove it?

clayborg added inline comments.Oct 24 2017, 2:40 PM
include/lldb/Target/Target.h
1217 ↗(On Diff #106600)

No, most architectures don't have one of these plug-ins. It isn't mandatory, so NULL is just fine.

source/Core/PluginManager.cpp
281 ↗(On Diff #106600)

ConstString makes for faster lookups. Many plugins can ask for a plug-in by name, so ConstString works well in that regard.

282 ↗(On Diff #106600)

We need "std::string" since it owns the storage. Most people do init this with a const string, but they don't need to. ConstString doesn't work here because we never will search for a description. StringRef doesn't own the storage, so I would rather avoid StringRef unless you can guarantee no one can construct a StringRef with local data.

323 ↗(On Diff #106600)

You are probably correct. I don't really mind the thread safety. We would need to worry about this more if people could write external plug-ins, but that isn't the case here. I would just leave it personally.

source/Plugins/Architecture/Arm/ArchitectureArm.cpp
23 ↗(On Diff #106600)

One time at startup. No threads contending yet. Asking for plug-in by name is made fast for later. I would leave this.

source/Plugins/Architecture/Arm/ArchitectureArm.h
26 ↗(On Diff #106600)

use lldb-forward.h

zturner added inline comments.Oct 24 2017, 2:47 PM
source/Core/PluginManager.cpp
281 ↗(On Diff #106600)

I'm not convinced it makes for faster lookups, because while true that it is "just" a pointer comparison, it is a pointer comparison that involves taking a global mutex.

In the case of StringRef constructed from a string literal (which as I mentioned, is I believe 100% of usage today), StringRef will be faster, because it will pass an unguarded pointer comparison. Only if the pointer comparisons fail does StringRef fall back to memcmp (or at least, it *should* work that way, if it doesn't we can make it)

282 ↗(On Diff #106600)

But why do you need to own the storage of a string literal? The binary already owns that storage. Just document in the API that it needs to be stable storage, since that covers 100% of existing use cases. And if someone needs something else they can use an llvm::StringSaver

source/Plugins/Architecture/Arm/ArchitectureArm.cpp
23 ↗(On Diff #106600)

Is asking for a plugin by name something that happens in a hot path?

clayborg added inline comments.Oct 24 2017, 2:55 PM
source/Core/PluginManager.cpp
282 ↗(On Diff #106600)

Why not just use a std::string then. Both std::string and StringRef will have similar compare times.

source/Plugins/Architecture/Arm/ArchitectureArm.cpp
23 ↗(On Diff #106600)

No. If we are talking about making the code safer, I would rather go with something that can guarantee safety. The StringRef has the possibility of going wrong if someone uses a local string. So unless we can guarantee it I don't see a reason to change.

zturner added inline comments.Oct 24 2017, 3:17 PM
source/Plugins/Architecture/Arm/ArchitectureArm.cpp
23 ↗(On Diff #106600)

I agree with you that it leaves open the possibility of something going wrong, but why isn't "don't use a local string" a sufficient answer to this. I thought (perhaps mistakenly) that we agreed that if you document your assumptions, you can then assume that people will follow them. This isn't user input.

It's worth mentioning that strings are one of LLDB's biggest memory hogs, and getting as much stuff out of the global string pool as possible is a win from a memory perspective. So, I don't think "I'm not really sure if anyone will ever do the wrong thing here, so let's just be safe" is a good long term strategy. Instead, we shoudl look at each case and say "can we tell users to do something reasonable here", and if so we should do that.

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.

So, I think this isn't a pretend problem, and that we should be more vigilant about mindlessly constifying all strings "just in case". There has to be a strong technical argument for it (i.e. millions of duplicated strings originating from user input, a hot-path, etc).

That said, this is a pretty uninteresting case, so I won't push it here, but unnecessary use of ConstString this is something I'm actively interested in reducing.

clayborg added inline comments.Oct 24 2017, 3:28 PM
source/Plugins/Architecture/Arm/ArchitectureArm.cpp
23 ↗(On Diff #106600)

We mmap everything. Many people don't know how to correctly say which programs are using too much memory. I don't care if we mmap 20GB of data, I care if we page that all into real memory. Not saying that the people that presented were making errors, but it could be interesting to see their data to see what they found and see if their issues are real ones.

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 and lldb wasn't using any accelerator tables for the dwarf of their inferior test program.

gdb was designed before there were any accelerator tables, so in their absence, it will create "partial symbol tables" with the address ranges and globally visible names of each CU. When details are needed for a specific CU, the psymtab is expanded into a symbol table, symtab. DWARF is scanned on initial load to create the psymtab names, and then when the symtab is created, it is ingested a CU at a time. nb: my knowledge of lldb is a decade old at this point, I have not kept up with the project.

lldb was designed with the assumption that we could get these global symbol names from accelerator tables - we would map them into our address space and search for names in them. When we are interested in a specific type or function or variable, we will read only the DWARF related to that type/function/variable.

In the absence of accelerator tables, I am willing to believe that lldb exhibits poor memory and startup performance -- that was not something we (at apple) concentrated on the performance of. It's a completely valid area for future work in lldb -- adopting to using accelerator tables other than apple's vendor specific ones.

I suspect, in an absence of concrete information, that the measurement of lldb's memory use being primarily strings, and the perf issues mentioned in this paper, are all due to this non-performant code path on Linux. If we see these memory issues when there are accelerator tables, then that's a big problem and I'll be shocked if we can actually be out-performed by gdb. They were the base line that we designed ourselves to do better than.

We can expend a lot of energy on making string tables smaller, and of course I wouldn't object to people working in that direction. But I think the real solution is to get lldb on non-darwin platforms to be using the accelerator tables that exist and allowing the full design of lldb to work as is intended. My two cents.

zturner accepted this revision.Oct 24 2017, 3:44 PM

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 the presence of stuff that is clearly not user input" argument. I understand the concerns, but I don't think this is a reasonable path forward for the project. If it's not user input, if we own it, then we can make whatever assumption we want that leads to the best performance and memory usage characteristics.

As I said this is a pretty uninteresting case, and probably makes no difference since we're talking about 30-40 strings in the whole program, so I'll leave it at that. But I will probably push on this harder in the future if it's in some other area of code that has a larger impact.

This revision was automatically updated to reflect the committed changes.