Move stop info override callback code from ArchSpec into Process

[labath]

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.

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

[zturner]

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?

[jingham]

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]

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?

[jingham]

Yes, that seems like the cleanest solution.

[labath]

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]

The architecture plugin idea

[clayborg]

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]

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?

[labath]

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

[clayborg]

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.

[jingham]

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.

[jingham]

What Greg said...

[labath]

In 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, 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]

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

[clayborg]

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.

[labath]

In 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. 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)

[clayborg]

In D31172#808105, @labath wrote:

In 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. 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]

The version with a container object for ArchSpec+Plugin combo

[labath]

In D31172#808177, @clayborg wrote:

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

[labath]

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]

Looks fine. We can iterate on this.

[zturner]

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]

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]

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]

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]

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]

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]

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]

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.

[jasonmolenda]

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]

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.

lldb/include/lldb/Core/ArchSpec.h

 //===-- ArchSpec.h ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_ArchSpec_h_
 #define liblldb_ArchSpec_h_
 
 #if defined(__cplusplus)
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-forward.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Triple.h"
 
 namespace lldb_private {
 
 struct CoreDefinition;
 
 //----------------------------------------------------------------------
 /// @class ArchSpec ArchSpec.h "lldb/Core/ArchSpec.h"
enum Core {
     kCore_mips64el_first = eCore_mips64el,
     kCore_mips64el_last = eCore_mips64r6el,
 
     kCore_mips_first = eCore_mips32,
     kCore_mips_last = eCore_mips64r6el
 
   };
 
-  typedef void (*StopInfoOverrideCallbackType)(lldb_private::Thread &thread);
-
   //------------------------------------------------------------------
   /// Default constructor.
   ///
   /// Default constructor that initializes the object with invalid
   /// cpu type and subtype values.
   //------------------------------------------------------------------
   ArchSpec();
 
public:
   /// Assignment operator.
   ///
   /// @param[in] rhs another ArchSpec object to copy.
   ///
   /// @return A const reference to this object.
   //------------------------------------------------------------------
   const ArchSpec &operator=(const ArchSpec &rhs);
 
-  static size_t AutoComplete(llvm::StringRef name, StringList &matches);
+  static size_t AutoComplete(llvm::StringRef name, llvm::StringSet<> &matches);
 
   //------------------------------------------------------------------
   /// Returns a static string representing the current architecture.
   ///
   /// @return A static string correcponding to the current
   ///         architecture.
   //------------------------------------------------------------------
   const char *GetArchitectureName() const;
public:
   /// Compare an ArchSpec to another ArchSpec, requiring a compatible
   /// cpu type match between them.
   /// e.g. armv7s is compatible with armv7 - this method would return true
   ///
   /// @return true if the two ArchSpecs are compatible
   //------------------------------------------------------------------
   bool IsCompatibleMatch(const ArchSpec &rhs) const;
 
-  //------------------------------------------------------------------
-  /// Get a stop info override callback for the current architecture.
-  ///
-  /// Most platform specific code should go in lldb_private::Platform,
-  /// but there are cases where no matter which platform you are on
-  /// certain things hold true.
-  ///
-  /// This callback is currently intended to handle cases where a
-  /// program stops at an instruction that won't get executed and it
-  /// allows the stop reasonm, like "breakpoint hit", to be replaced
-  /// with a different stop reason like "no stop reason".
-  ///
-  /// This is specifically used for ARM in Thumb code when we stop in
-  /// an IT instruction (if/then/else) where the instruction won't get
-  /// executed and therefore it wouldn't be correct to show the program
-  /// stopped at the current PC. The code is generic and applies to all
-  /// ARM CPUs.
-  ///
-  /// @return NULL or a valid stop info override callback for the
-  ///     current architecture.
-  //------------------------------------------------------------------
-  StopInfoOverrideCallbackType GetStopInfoOverrideCallback() const;
-
   bool IsFullySpecifiedTriple() const;
 
   void PiecewiseTripleCompare(const ArchSpec &other, bool &arch_different,
                               bool &vendor_different, bool &os_different,
                               bool &os_version_different, bool &env_different);
 
   //------------------------------------------------------------------
   /// Detect whether this architecture uses thumb code exclusively

lldb/include/lldb/Target/Process.h

public:
   ///
   //------------------------------------------------------------------
   void RestoreProcessEvents();
 
   const lldb::ABISP &GetABI();
 
   OperatingSystem *GetOperatingSystem() { return m_os_ap.get(); }
 
-  ArchSpec::StopInfoOverrideCallbackType GetStopInfoOverrideCallback() const {
-    return m_stop_info_override_callback;
-  }
+  //------------------------------------------------------------------
+  /// Get a stop info override callback for the current architecture.
+  ///
+  /// Most platform specific code should go in lldb_private::Platform,
+  /// but there are cases where no matter which platform you are on
+  /// certain things hold true.
+  ///
+  /// This callback is currently intended to handle cases where a
+  /// program stops at an instruction that won't get executed and it
+  /// allows the stop reasonm, like "breakpoint hit", to be replaced
+  /// with a different stop reason like "no stop reason".
+  ///
+  /// This is specifically used for ARM in Thumb code when we stop in
+  /// an IT instruction (if/then/else) where the instruction won't get
+  /// executed and therefore it wouldn't be correct to show the program
+  /// stopped at the current PC. The code is generic and applies to all
+  /// ARM CPUs.
+  ///
+  /// @return NULL or a valid stop info override callback for the
+  ///     current architecture.
+  //------------------------------------------------------------------
+  typedef void (*StopInfoOverrideCallbackType)(lldb_private::Thread &);
+  void InvokeStopInfoOverrideCallback(lldb_private::Thread &Thread);
 
   virtual LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
                                               bool retry_if_null = true);
 
   virtual CPPLanguageRuntime *GetCPPLanguageRuntime(bool retry_if_null = true);
 
   virtual ObjCLanguageRuntime *
   GetObjCLanguageRuntime(bool retry_if_null = true);
protected:
   bool m_should_detach; /// Should we detach if the process object goes away
                         /// with an explicit call to Kill or Detach?
   LanguageRuntimeCollection m_language_runtimes;
   InstrumentationRuntimeCollection m_instrumentation_runtimes;
   std::unique_ptr<NextEventAction> m_next_event_action_ap;
   std::vector<PreResumeCallbackAndBaton> m_pre_resume_actions;
   ProcessRunLock m_public_run_lock;
   ProcessRunLock m_private_run_lock;
-  ArchSpec::StopInfoOverrideCallbackType m_stop_info_override_callback;
+  StopInfoOverrideCallbackType m_stop_info_override_callback;
   bool m_currently_handling_do_on_removals;
   bool m_resume_requested; // If m_currently_handling_event or
                            // m_currently_handling_do_on_removals are true,
                            // Resume will only request a resume, using this flag
                            // to check.
   bool m_finalizing; // This is set at the beginning of Process::Finalize() to
                      // stop functions from looking up or creating things during
                      // a finalize call
protected:
 
   void StopPrivateStateThread();
 
   void PausePrivateStateThread();
 
   void ResumePrivateStateThread();
 
 private:
+  void SetStopInfoOverrideCallback(const ArchSpec &Arch);
+
   struct PrivateStateThreadArgs {
     PrivateStateThreadArgs(Process *p, bool s)
         : process(p), is_secondary_thread(s){};
     Process *process;
     bool is_secondary_thread;
   };
 
   // arg is a pointer to a new'ed PrivateStateThreadArgs structure.

lldb/source/Core/ArchSpec.cpp

 
 // Other libraries and framework includes
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/COFF.h"
 #include "llvm/Support/ELF.h"
 #include "llvm/Support/Host.h"
 
 // Project includes
-#include "Plugins/Process/Utility/ARMDefines.h"
-#include "Plugins/Process/Utility/InstructionUtils.h"
-#include "lldb/Core/StringList.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Platform.h"
-#include "lldb/Target/Process.h"
-#include "lldb/Target/RegisterContext.h"
-#include "lldb/Target/Thread.h"
 #include "lldb/Utility/Endian.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/SafeMachO.h"
 
 using namespace lldb;
 using namespace lldb_private;
 

 
 struct ArchDefinition {
   ArchitectureType type;
   size_t num_entries;
   const ArchDefinitionEntry *entries;
   const char *name;
 };
 
-size_t ArchSpec::AutoComplete(llvm::StringRef name, StringList &matches) {
+size_t ArchSpec::AutoComplete(llvm::StringRef name,
+                              llvm::StringSet<> &matches) {
   if (!name.empty()) {
     for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i) {
       if (NameMatches(g_core_definitions[i].name, NameMatch::StartsWith, name))
-        matches.AppendString(g_core_definitions[i].name);
+        matches.insert(g_core_definitions[i].name);
     }
   } else {
     for (uint32_t i = 0; i < llvm::array_lengthof(g_core_definitions); ++i)
-      matches.AppendString(g_core_definitions[i].name);
+      matches.insert(g_core_definitions[i].name);
   }
-  return matches.GetSize();
+  return matches.size();
 }
 
 #define CPU_ANY (UINT32_MAX)
 
 //===----------------------------------------------------------------------===//
 // A table that gets searched linearly for matches. This table is used to
 // convert cpu type and subtypes to architecture names, and to convert
 // architecture names to cpu types and subtypes. The ordering is important and

 }
 
 bool lldb_private::operator<(const ArchSpec &lhs, const ArchSpec &rhs) {
   const ArchSpec::Core lhs_core = lhs.GetCore();
   const ArchSpec::Core rhs_core = rhs.GetCore();
   return lhs_core < rhs_core;
 }
 
-static void StopInfoOverrideCallbackTypeARM(lldb_private::Thread &thread) {
-  // We need to check if we are stopped in Thumb mode in a IT instruction
-  // and detect if the condition doesn't pass. If this is the case it means
-  // we won't actually execute this instruction. If this happens we need to
-  // clear the stop reason to no thread plans think we are stopped for a
-  // reason and the plans should keep going.
-  //
-  // We do this because when single stepping many ARM processes, debuggers
-  // often use the BVR/BCR registers that says "stop when the PC is not
-  // equal to its current value". This method of stepping means we can end
-  // up stopping on instructions inside an if/then block that wouldn't get
-  // executed. By fixing this we can stop the debugger from seeming like
-  // you stepped through both the "if" _and_ the "else" clause when source
-  // level stepping because the debugger stops regardless due to the BVR/BCR
-  // triggering a stop.
-  //
-  // It also means we can set breakpoints on instructions inside an an
-  // if/then block and correctly skip them if we use the BKPT instruction.
-  // The ARM and Thumb BKPT instructions are unconditional even when executed
-  // in a Thumb IT block.
-  //
-  // If your debugger inserts software traps in ARM/Thumb code, it will
-  // need to use 16 and 32 bit instruction for 16 and 32 bit thumb
-  // instructions respectively. If your debugger inserts a 16 bit thumb
-  // trap on top of a 32 bit thumb instruction for an opcode that is inside
-  // an if/then, it will change the it/then to conditionally execute your
-  // 16 bit trap and then cause your program to crash if it executes the
-  // trailing 16 bits (the second half of the 32 bit thumb instruction you
-  // partially overwrote).
-
-  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
-  if (reg_ctx_sp) {
-    const uint32_t cpsr = reg_ctx_sp->GetFlags(0);
-    if (cpsr != 0) {
-      // Read the J and T bits to get the ISETSTATE
-      const uint32_t J = Bit32(cpsr, 24);
-      const uint32_t T = Bit32(cpsr, 5);
-      const uint32_t ISETSTATE = J << 1 | T;
-      if (ISETSTATE == 0) {
-// NOTE: I am pretty sure we want to enable the code below
-// that detects when we stop on an instruction in ARM mode
-// that is conditional and the condition doesn't pass. This
-// can happen if you set a breakpoint on an instruction that
-// is conditional. We currently will _always_ stop on the
-// instruction which is bad. You can also run into this while
-// single stepping and you could appear to run code in the "if"
-// and in the "else" clause because it would stop at all of the
-// conditional instructions in both.
-// In such cases, we really don't want to stop at this location.
-// I will check with the lldb-dev list first before I enable this.
-#if 0
-                // ARM mode: check for condition on intsruction
-                const addr_t pc = reg_ctx_sp->GetPC();
-                Error error;
-                // If we fail to read the opcode we will get UINT64_MAX as the
-                // result in "opcode" which we can use to detect if we read a
-                // valid opcode.
-                const uint64_t opcode = thread.GetProcess()->ReadUnsignedIntegerFromMemory(pc, 4, UINT64_MAX, error);
-                if (opcode <= UINT32_MAX)
-                {
-                    const uint32_t condition = Bits32((uint32_t)opcode, 31, 28);
-                    if (!ARMConditionPassed(condition, cpsr))
-                    {
-                        // We ARE stopped on an ARM instruction whose condition doesn't
-                        // pass so this instruction won't get executed.
-                        // Regardless of why it stopped, we need to clear the stop info
-                        thread.SetStopInfo (StopInfoSP());
-                    }
-                }
-#endif
-      } else if (ISETSTATE == 1) {
-        // Thumb mode
-        const uint32_t ITSTATE =
-            Bits32(cpsr, 15, 10) << 2 | Bits32(cpsr, 26, 25);
-        if (ITSTATE != 0) {
-          const uint32_t condition = Bits32(ITSTATE, 7, 4);
-          if (!ARMConditionPassed(condition, cpsr)) {
-            // We ARE stopped in a Thumb IT instruction on an instruction whose
-            // condition doesn't pass so this instruction won't get executed.
-            // Regardless of why it stopped, we need to clear the stop info
-            thread.SetStopInfo(StopInfoSP());
-          }
-        }
-      }
-    }
-  }
-}
-
-ArchSpec::StopInfoOverrideCallbackType
-ArchSpec::GetStopInfoOverrideCallback() const {
-  const llvm::Triple::ArchType machine = GetMachine();
-  if (machine == llvm::Triple::arm)
-    return StopInfoOverrideCallbackTypeARM;
-  return nullptr;
-}
-
 bool ArchSpec::IsFullySpecifiedTriple() const {
   const auto &user_specified_triple = GetTriple();
 
   bool user_triple_fully_specified = false;
 
   if ((user_specified_triple.getOS() != llvm::Triple::UnknownOS) ||
       TripleOSWasSpecified()) {
     if ((user_specified_triple.getVendor() != llvm::Triple::UnknownVendor) ||

lldb/source/Interpreter/CommandObject.cpp

 #include "lldb/Target/Target.h"
 #include "lldb/Utility/FileSpec.h"
 
 #include "lldb/Target/Language.h"
 
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringSet.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
 //-------------------------------------------------------------------------
 // CommandObject
 //-------------------------------------------------------------------------
 
 CommandObject::CommandObject(CommandInterpreter &interpreter, llvm::StringRef name,
if (!handled) {
     if (CheckRequirements(result))
       handled = DoExecute(args_string, result);
 
     Cleanup();
   }
   return handled;
 }
 
-static llvm::StringRef arch_helper() {
-  static StreamString g_archs_help;
-  if (g_archs_help.Empty()) {
-    StringList archs;
-    ArchSpec::AutoComplete(llvm::StringRef(), archs);
-    g_archs_help.Printf("These are the supported architecture names:\n");
-    archs.Join("\n", g_archs_help);
-  }
-  return g_archs_help.GetString();
+static std::string computeArchHelp() {
+  llvm::StringSet<> Archs;
+  ArchSpec::AutoComplete("", Archs);
+  std::string Result;
+  llvm::raw_string_ostream OS(Result);
+  OS << "These are the supported architecture names:\n";
+  OS << llvm::join_range(Archs.keys(), "\n");
+  return OS.str();
+
+static llvm::StringRef arch_helper() {
+  static std::string Help = computeArchHelp();
+  return Help;
 }
 
 CommandObject::ArgumentTableEntry CommandObject::g_arguments_data[] = {
     // clang-format off
     { eArgTypeAddress, "address", CommandCompletions::eNoCompletion, { nullptr, false }, "A valid address in the target program's execution space." },
     { eArgTypeAddressOrExpression, "address-expression", CommandCompletions::eNoCompletion, { nullptr, false }, "An expression that resolves to an address." },
     { eArgTypeAliasName, "alias-name", CommandCompletions::eNoCompletion, { nullptr, false }, "The name of an abbreviation (alias) for a debugger command." },
     { eArgTypeAliasOptions, "options-for-aliased-command", CommandCompletions::eNoCompletion, { nullptr, false }, "Command options to be used as part of an alias (abbreviation) definition.  (See 'help commands alias' for more information.)" },

lldb/source/Target/Process.cpp

 #include <atomic>
 #include <mutex>
 
 // Other libraries and framework includes
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 
 // Project includes
+#include "Plugins/Process/Utility/ARMDefines.h"
 #include "Plugins/Process/Utility/InferiorCallPOSIX.h"
+#include "Plugins/Process/Utility/InstructionUtils.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Event.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/State.h"
const size_t curr_bytes_written = DoWriteMemory(
         addr + bytes_written, bytes + bytes_written, curr_size, error);
     bytes_written += curr_bytes_written;
     if (curr_bytes_written == curr_size || curr_bytes_written == 0)
       break;
   }
   return bytes_written;
 }
 
+static void StopInfoOverrideCallbackTypeARM(lldb_private::Thread &thread) {
+  // We need to check if we are stopped in Thumb mode in a IT instruction
+  // and detect if the condition doesn't pass. If this is the case it means
+  // we won't actually execute this instruction. If this happens we need to
+  // clear the stop reason to no thread plans think we are stopped for a
+  // reason and the plans should keep going.
+  //
+  // We do this because when single stepping many ARM processes, debuggers
+  // often use the BVR/BCR registers that says "stop when the PC is not
+  // equal to its current value". This method of stepping means we can end
+  // up stopping on instructions inside an if/then block that wouldn't get
+  // executed. By fixing this we can stop the debugger from seeming like
+  // you stepped through both the "if" _and_ the "else" clause when source
+  // level stepping because the debugger stops regardless due to the BVR/BCR
+  // triggering a stop.
+  //
+  // It also means we can set breakpoints on instructions inside an an
+  // if/then block and correctly skip them if we use the BKPT instruction.
+  // The ARM and Thumb BKPT instructions are unconditional even when executed
+  // in a Thumb IT block.
+  //
+  // If your debugger inserts software traps in ARM/Thumb code, it will
+  // need to use 16 and 32 bit instruction for 16 and 32 bit thumb
+  // instructions respectively. If your debugger inserts a 16 bit thumb
+  // trap on top of a 32 bit thumb instruction for an opcode that is inside
+  // an if/then, it will change the it/then to conditionally execute your
+  // 16 bit trap and then cause your program to crash if it executes the
+  // trailing 16 bits (the second half of the 32 bit thumb instruction you
+  // partially overwrote).
+
+  RegisterContextSP reg_ctx_sp(thread.GetRegisterContext());
+  if (reg_ctx_sp) {
+    const uint32_t cpsr = reg_ctx_sp->GetFlags(0);
+    if (cpsr != 0) {
+      // Read the J and T bits to get the ISETSTATE
+      const uint32_t J = Bit32(cpsr, 24);
+      const uint32_t T = Bit32(cpsr, 5);
+      const uint32_t ISETSTATE = J << 1 | T;
+      if (ISETSTATE == 0) {
+// NOTE: I am pretty sure we want to enable the code below
+// that detects when we stop on an instruction in ARM mode
+// that is conditional and the condition doesn't pass. This
+// can happen if you set a breakpoint on an instruction that
+// is conditional. We currently will _always_ stop on the
+// instruction which is bad. You can also run into this while
+// single stepping and you could appear to run code in the "if"
+// and in the "else" clause because it would stop at all of the
+// conditional instructions in both.
+// In such cases, we really don't want to stop at this location.
+// I will check with the lldb-dev list first before I enable this.
+#if 0
+        // ARM mode: check for condition on intsruction
+        const addr_t pc = reg_ctx_sp->GetPC();
+        Error error;
+        // If we fail to read the opcode we will get UINT64_MAX as the
+        // result in "opcode" which we can use to detect if we read a
+        // valid opcode.
+        const uint64_t opcode = thread.GetProcess()->ReadUnsignedIntegerFromMemory(pc, 4, UINT64_MAX, error);
+        if (opcode <= UINT32_MAX)
+        {
+          const uint32_t condition = Bits32((uint32_t)opcode, 31, 28);
+          if (!ARMConditionPassed(condition, cpsr))
+          {
+            // We ARE stopped on an ARM instruction whose condition doesn't
+            // pass so this instruction won't get executed.
+            // Regardless of why it stopped, we need to clear the stop info
+            thread.SetStopInfo(StopInfoSP());
+          }
+        }
+#endif
+      } else if (ISETSTATE == 1) {
+        // Thumb mode
+        const uint32_t ITSTATE =
+            Bits32(cpsr, 15, 10) << 2 | Bits32(cpsr, 26, 25);
+        if (ITSTATE != 0) {
+          const uint32_t condition = Bits32(ITSTATE, 7, 4);
+          if (!ARMConditionPassed(condition, cpsr)) {
+            // We ARE stopped in a Thumb IT instruction on an instruction whose
+            // condition doesn't pass so this instruction won't get executed.
+            // Regardless of why it stopped, we need to clear the stop info
+            thread.SetStopInfo(StopInfoSP());
+          }
+        }
+      }
+    }
+  }
+}
+
+void Process::SetStopInfoOverrideCallback(const ArchSpec &Arch) {
+  if (Arch.GetMachine() == llvm::Triple::arm)
+    m_stop_info_override_callback = StopInfoOverrideCallbackTypeARM;
+}
+
+void Process::InvokeStopInfoOverrideCallback(lldb_private::Thread &Thread) {
+  if (m_stop_info_override_callback)
+    m_stop_info_override_callback(Thread);
+}
+
 size_t Process::WriteMemory(addr_t addr, const void *buf, size_t size,
                             Error &error) {
 #if defined(ENABLE_MEMORY_CACHING)
   m_memory_cache.Flush(addr, size);
 #endif
 
   if (buf == nullptr || size == 0)
     return 0;
if (exe_module->GetFileSpec().Exists()) {
             // ProcessIOHandler.
             SetPublicState(state, false);
 
             if (PrivateStateThreadIsValid())
               ResumePrivateStateThread();
             else
               StartPrivateStateThread();
 
-            m_stop_info_override_callback =
-                GetTarget().GetArchitecture().GetStopInfoOverrideCallback();
+            SetStopInfoOverrideCallback(GetTarget().GetArchitecture());
 
             // Target was stopped at entry as was intended. Need to notify the
             // listeners
             // about it.
             if (state == eStateStopped &&
                 launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
               HandlePrivateEvent(event_sp);
           } else if (state == eStateExited) {
if (log) {
       log->Printf(
           "Process::%s after looping through modules, target executable is %s",
           __FUNCTION__,
           exe_module_sp ? exe_module_sp->GetFileSpec().GetPath().c_str()
                         : "<none>");
     }
   }
 
-  m_stop_info_override_callback = process_arch.GetStopInfoOverrideCallback();
+  SetStopInfoOverrideCallback(process_arch);
 }
 
 Error Process::ConnectRemote(Stream *strm, llvm::StringRef remote_url) {
   m_abi_sp.reset();
   m_process_input_reader.reset();
 
   // Find the process and its architecture.  Make sure it matches the
   // architecture of the current Target, and if not adjust it.

lldb/source/Target/Thread.cpp

if (process_sp) {
     // "m_stop_info_stop_id != process_stop_id" as the condition for
     // the if statement below, we must also check the stop info to see
     // if we need to override it. See the header documentation in
     // Process::GetStopInfoOverrideCallback() for more information on
     // the stop info override callback.
     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);
+        GetProcess()->InvokeStopInfoOverrideCallback(*this);
       }
     }
   }
   return m_stop_info_sp;
 }
 
 lldb::StopReason Thread::GetStopReason() {
   lldb::StopInfoSP stop_info_sp(GetStopInfo());