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/trunk/include/lldb/Core/ArchSpec.h

 #ifndef liblldb_ArchSpec_h_
 #define liblldb_ArchSpec_h_
 
 #if defined(__cplusplus)
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-private-enumerations.h"
+#include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h" // for StringRef
 #include "llvm/ADT/Triple.h"
 
 #include <string> // for string
 
 #include <stddef.h> // for size_t
 #include <stdint.h> // for uint32_t
 
 namespace lldb_private {
-class Platform;
-}
-namespace lldb_private {
-class Stream;
-}
-namespace lldb_private {
-class StringList;
-}
-namespace lldb_private {
-class Thread;
-}
-
-namespace lldb_private {
 
 //----------------------------------------------------------------------
 /// @class ArchSpec ArchSpec.h "lldb/Core/ArchSpec.h"
 /// @brief An architecture specification class.
 ///
 /// A class designed to be created from a cpu type and subtype, a
 /// string representation, or an llvm::Triple.  Keeping all of the
 /// conversions of strings to architecture enumeration values confined
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:
   /// 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);
+                              bool &os_version_different, bool &env_different) const;
 
   //------------------------------------------------------------------
   /// Detect whether this architecture uses thumb code exclusively
   ///
   /// Some embedded ARM chips (e.g. the ARM Cortex M0-7 line) can
   /// only execute the Thumb instructions, never Arm.  We should normally
   /// pick up arm/thumbness from their the processor status bits (cpsr/xpsr)
   /// or hints on each function - but when doing bare-boards low level

lldb/trunk/include/lldb/Core/Architecture.h

+//===-- Architecture.h ------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_CORE_ARCHITECTURE_H
+#define LLDB_CORE_ARCHITECTURE_H
+
+#include "lldb/Core/PluginInterface.h"
+
+namespace lldb_private {
+
+class Architecture : public PluginInterface {
+public:
+  Architecture() = default;
+  virtual ~Architecture() = default;
+
+  //------------------------------------------------------------------
+  /// This 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.
+  //------------------------------------------------------------------
+  virtual void OverrideStopInfo(Thread &thread) = 0;
+
+private:
+  Architecture(const Architecture &) = delete;
+  void operator=(const Architecture &) = delete;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_CORE_ARCHITECTURE_H

lldb/trunk/include/lldb/Core/PluginManager.h

 //===-- PluginManager.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_PluginManager_h_
 #define liblldb_PluginManager_h_
 
+#include "lldb/Core/Architecture.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/Status.h"          // for Status
 #include "lldb/lldb-enumerations.h"       // for ScriptLanguage
 #include "lldb/lldb-forward.h"            // for OptionValuePropertiesSP
 #include "lldb/lldb-private-interfaces.h" // for DebuggerInitializeCallback
 #include "llvm/ADT/StringRef.h"           // for StringRef
 
 #include <stddef.h> // for size_t
public:
   static bool UnregisterPlugin(ABICreateInstance create_callback);
 
   static ABICreateInstance GetABICreateCallbackAtIndex(uint32_t idx);
 
   static ABICreateInstance
   GetABICreateCallbackForPluginName(const ConstString &name);
 
   //------------------------------------------------------------------
+  // Architecture
+  //------------------------------------------------------------------
+  using ArchitectureCreateInstance =
+      std::unique_ptr<Architecture> (*)(const ArchSpec &);
+
+  static void RegisterPlugin(const ConstString &name,
+                             llvm::StringRef description,
+                             ArchitectureCreateInstance create_callback);
+
+  static void UnregisterPlugin(ArchitectureCreateInstance create_callback);
+
+  static std::unique_ptr<Architecture>
+  CreateArchitectureInstance(const ArchSpec &arch);
+
+  //------------------------------------------------------------------
   // Disassembler
   //------------------------------------------------------------------
   static bool RegisterPlugin(const ConstString &name, const char *description,
                              DisassemblerCreateInstance create_callback);
 
   static bool UnregisterPlugin(DisassemblerCreateInstance create_callback);
 
   static DisassemblerCreateInstance

lldb/trunk/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;
-  }
-
   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;
   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

lldb/trunk/include/lldb/Target/Target.h

 #include <vector>
 
 // Other libraries and framework includes
 // Project includes
 #include "lldb/Breakpoint/BreakpointList.h"
 #include "lldb/Breakpoint/BreakpointName.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/Architecture.h"
 #include "lldb/Core/Broadcaster.h"
 #include "lldb/Core/Disassembler.h"
 #include "lldb/Core/ModuleList.h"
 #include "lldb/Core/UserSettingsController.h"
 #include "lldb/Expression/Expression.h"
 #include "lldb/Symbol/TypeSystem.h"
 #include "lldb/Target/ExecutionContextScope.h"
 #include "lldb/Target/PathMappingList.h"
void ApplyNameToBreakpoints(BreakpointName &bp_name);
   /// @param[in] module_sp
   ///     A shared pointer reference to the module that checked.
   ///
   /// @return \b true if the module should be excluded, \b false otherwise.
   //------------------------------------------------------------------
   bool
   ModuleIsExcludedForUnconstrainedSearches(const lldb::ModuleSP &module_sp);
 
-  const ArchSpec &GetArchitecture() const { return m_arch; }
+  const ArchSpec &GetArchitecture() const { return m_arch.GetSpec(); }
 
   //------------------------------------------------------------------
   /// Set the architecture for this target.
   ///
   /// If the current target has no Images read in, then this just sets the
   /// architecture, which will
   /// be used to select the architecture of the ExecutableModule when that is
   /// set.
void ApplyNameToBreakpoints(BreakpointName &bp_name);
   ///
   /// @return
   ///     \b true if the architecture was successfully set, \bfalse otherwise.
   //------------------------------------------------------------------
   bool SetArchitecture(const ArchSpec &arch_spec);
 
   bool MergeArchitecture(const ArchSpec &arch_spec);
 
+  Architecture *GetArchitecturePlugin() { return m_arch.GetPlugin(); }
+
   Debugger &GetDebugger() { return m_debugger; }
 
   size_t ReadMemoryFromFileCache(const Address &addr, void *dst, size_t dst_len,
                                  Status &error);
 
   // Reading memory through the target allows us to skip going to the process
   // for reading memory if possible and it allows us to try and read from
   // any constant sections in our object files on disk. If you always want
protected:
   void ModuleRemoved(const ModuleList &module_list,
                      const lldb::ModuleSP &module_sp) override;
 
   void ModuleUpdated(const ModuleList &module_list,
                      const lldb::ModuleSP &old_module_sp,
                      const lldb::ModuleSP &new_module_sp) override;
   void WillClearList(const ModuleList &module_list) override;
 
+  class Arch {
+  public:
+    explicit Arch(const ArchSpec &spec);
+    const Arch &operator=(const ArchSpec &spec);
+
+    const ArchSpec &GetSpec() const { return m_spec; }
+    Architecture *GetPlugin() const { return m_plugin_up.get(); }
+
+  private:
+    ArchSpec m_spec;
+    std::unique_ptr<Architecture> m_plugin_up;
+  };
   //------------------------------------------------------------------
   // Member variables.
   //------------------------------------------------------------------
   Debugger &m_debugger;
   lldb::PlatformSP m_platform_sp; ///< The platform for this target.
   std::recursive_mutex m_mutex; ///< An API mutex that is used by the lldb::SB*
                                 /// classes make the SB interface thread safe
-  ArchSpec m_arch;
+  Arch m_arch;
   ModuleList m_images; ///< The list of images for this process (shared
                        /// libraries and anything dynamically loaded).
   SectionLoadHistory m_section_load_history;
   BreakpointList m_breakpoint_list;
   BreakpointList m_internal_breakpoint_list;
   using BreakpointNameList = std::map<ConstString, BreakpointName *>;
   BreakpointNameList m_breakpoint_names;
   

lldb/trunk/packages/Python/lldbsuite/test/arm/breakpoint-it/Makefile

+LEVEL = ../../make
+
+C_SOURCES := main.c
+CFLAGS_EXTRAS = -mthumb
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/arm/breakpoint-it/TestBreakpointIt.py

+"""
+Test that breakpoints in an IT instruction don't fire if their condition is
+false.
+"""
+from __future__ import print_function
+
+
+import lldb
+import os
+import time
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestBreakpointIt(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIf(archs=no_match(["arm"]))
+    def test_false(self):
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        self.runCmd("target create %s" % exe)
+        lldbutil.run_break_set_by_symbol(self, "bkpt_false",
+                extra_options="--skip-prologue 0")
+
+        self.runCmd("run")
+        self.assertEqual(self.process().GetState(), lldb.eStateExited,
+                "Breakpoint does not get hit")
+
+    @skipIf(archs=no_match(["arm"]))
+    def test_true(self):
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        self.runCmd("target create %s" % exe)
+        bpid = lldbutil.run_break_set_by_symbol(self, "bkpt_true",
+                extra_options="--skip-prologue 0")
+
+        self.runCmd("run")
+        self.assertIsNotNone(lldbutil.get_one_thread_stopped_at_breakpoint_id(
+            self.process(), bpid))

lldb/trunk/packages/Python/lldbsuite/test/arm/breakpoint-it/main.c

+int main() {
+  int value;
+  asm (
+      "cmp %1, %2\n\t"
+      "ite ne\n\t"
+      ".thumb_func\n\t"
+      "bkpt_true:\n\t"
+      "movne %0, %1\n\t"
+      ".thumb_func\n\t"
+      "bkpt_false:\n\t"
+      "moveq %0, %2\n\t"
+      : "=r" (value) : "r"(42), "r"(47));
+  return value;
+}

lldb/trunk/source/API/SystemInitializerFull.cpp

 #include "Plugins/ABI/SysV-hexagon/ABISysV_hexagon.h"
 #include "Plugins/ABI/SysV-i386/ABISysV_i386.h"
 #include "Plugins/ABI/SysV-mips/ABISysV_mips.h"
 #include "Plugins/ABI/SysV-mips64/ABISysV_mips64.h"
 #include "Plugins/ABI/SysV-ppc/ABISysV_ppc.h"
 #include "Plugins/ABI/SysV-ppc64/ABISysV_ppc64.h"
 #include "Plugins/ABI/SysV-s390x/ABISysV_s390x.h"
 #include "Plugins/ABI/SysV-x86_64/ABISysV_x86_64.h"
+#include "Plugins/Architecture/Arm/ArchitectureArm.h"
 #include "Plugins/Disassembler/llvm/DisassemblerLLVMC.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.h"
 #include "Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.h"
 #include "Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h"
 #include "Plugins/DynamicLoader/Static/DynamicLoaderStatic.h"
 #include "Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.h"
 #include "Plugins/Instruction/ARM64/EmulateInstructionARM64.h"
 #include "Plugins/InstrumentationRuntime/ASan/ASanRuntime.h"
+#include "Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h"
 #include "Plugins/InstrumentationRuntime/TSan/TSanRuntime.h"
 #include "Plugins/InstrumentationRuntime/UBSan/UBSanRuntime.h"
-#include "Plugins/InstrumentationRuntime/MainThreadChecker/MainThreadCheckerRuntime.h"
 #include "Plugins/JITLoader/GDB/JITLoaderGDB.h"
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
 #include "Plugins/Language/Go/GoLanguage.h"
 #include "Plugins/Language/Java/JavaLanguage.h"
 #include "Plugins/Language/OCaml/OCamlLanguage.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
 #include "Plugins/Language/ObjCPlusPlus/ObjCPlusPlusLanguage.h"
 #include "Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.h"
#endif
   ABISysV_hexagon::Initialize();
   ABISysV_i386::Initialize();
   ABISysV_x86_64::Initialize();
   ABISysV_ppc::Initialize();
   ABISysV_ppc64::Initialize();
   ABISysV_mips::Initialize();
   ABISysV_mips64::Initialize();
   ABISysV_s390x::Initialize();
+
+  ArchitectureArm::Initialize();
+
   DisassemblerLLVMC::Initialize();
 
   JITLoaderGDB::Initialize();
   ProcessElfCore::Initialize();
   minidump::ProcessMinidump::Initialize();
   MemoryHistoryASan::Initialize();
   AddressSanitizerRuntime::Initialize();
   ThreadSanitizerRuntime::Initialize();

lldb/trunk/source/Core/ArchSpec.cpp

 //===-- ArchSpec.cpp --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "lldb/Core/ArchSpec.h"
 
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Platform.h"
-#include "lldb/Target/RegisterContext.h"
-#include "lldb/Target/Thread.h"
 #include "lldb/Utility/NameMatches.h"
 #include "lldb/Utility/Stream.h" // for Stream
 #include "lldb/Utility/StringList.h"
 #include "lldb/lldb-defines.h" // for LLDB_INVALID_C...
 #include "lldb/lldb-forward.h" // for RegisterContextSP
 
-#include "Plugins/Process/Utility/ARMDefines.h"
-#include "Plugins/Process/Utility/InstructionUtils.h"
-
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Twine.h" // for Twine
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/BinaryFormat/MachO.h" // for CPUType::CPU_T...
 #include "llvm/Support/Compiler.h"   // for LLVM_FALLTHROUGH
 #include "llvm/Support/Host.h"
 

 }
 
 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();
-                Status 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) ||
         TripleVendorWasSpecified()) {
       const unsigned unspecified = 0;
       if (user_specified_triple.getOSMajorVersion() != unspecified) {
         user_triple_fully_specified = true;
       }
     }
   }
 
   return user_triple_fully_specified;
 }
 
 void ArchSpec::PiecewiseTripleCompare(
     const ArchSpec &other, bool &arch_different, bool &vendor_different,
-    bool &os_different, bool &os_version_different, bool &env_different) {
+    bool &os_different, bool &os_version_different, bool &env_different) const {
   const llvm::Triple &me(GetTriple());
   const llvm::Triple &them(other.GetTriple());
 
   arch_different = (me.getArch() != them.getArch());
 
   vendor_different = (me.getVendor() != them.getVendor());
 
   os_different = (me.getOS() != them.getOS());

lldb/trunk/source/Core/PluginManager.cpp

if (name) {
     for (pos = instances.begin(); pos != end; ++pos) {
       if (name == pos->name)
         return pos->create_callback;
     }
   }
   return nullptr;
 }
 
+#pragma mark Architecture
+
+struct ArchitectureInstance {
+  ConstString name;
+  std::string description;
+  PluginManager::ArchitectureCreateInstance create_callback;
+};
+
+typedef std::vector<ArchitectureInstance> ArchitectureInstances;
+
+static std::mutex g_architecture_mutex;
+
+static ArchitectureInstances &GetArchitectureInstances() {
+  static ArchitectureInstances g_instances;
+  return g_instances;
+}
+
+void PluginManager::RegisterPlugin(const ConstString &name,
+                                   llvm::StringRef description,
+                                   ArchitectureCreateInstance create_callback) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  GetArchitectureInstances().push_back({name, description, create_callback});
+}
+
+void PluginManager::UnregisterPlugin(
+    ArchitectureCreateInstance create_callback) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  auto &instances = GetArchitectureInstances();
+
+  for (auto pos = instances.begin(), end = instances.end(); pos != end; ++pos) {
+    if (pos->create_callback == create_callback) {
+      instances.erase(pos);
+      return;
+    }
+  }
+  llvm_unreachable("Plugin not found");
+}
+
+std::unique_ptr<Architecture>
+PluginManager::CreateArchitectureInstance(const ArchSpec &arch) {
+  std::lock_guard<std::mutex> guard(g_architecture_mutex);
+  for (const auto &instances : GetArchitectureInstances()) {
+    if (auto plugin_up = instances.create_callback(arch))
+      return plugin_up;
+  }
+  return nullptr;
+}
+
 #pragma mark Disassembler
 
 struct DisassemblerInstance {
   DisassemblerInstance() : name(), description(), create_callback(nullptr) {}
 
   ConstString name;
   std::string description;
   DisassemblerCreateInstance create_callback;

lldb/trunk/source/Plugins/Architecture/Arm/ArchitectureArm.h

+//===-- ArchitectureArm.h ---------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGIN_ARCHITECTURE_ARM_H
+#define LLDB_PLUGIN_ARCHITECTURE_ARM_H
+
+#include "lldb/Core/Architecture.h"
+
+namespace lldb_private {
+
+class ArchitectureArm : public Architecture {
+public:
+  static ConstString GetPluginNameStatic();
+  static void Initialize();
+  static void Terminate();
+
+  ConstString GetPluginName() override;
+  uint32_t GetPluginVersion() override;
+
+  void OverrideStopInfo(Thread &thread) override;
+
+private:
+  static std::unique_ptr<Architecture> Create(const ArchSpec &arch);
+  ArchitectureArm() = default;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_PLUGIN_ARCHITECTURE_ARM_H

lldb/trunk/source/Plugins/Architecture/Arm/ArchitectureArm.cpp

+//===-- ArchitectureArm.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/Architecture/Arm/ArchitectureArm.h"
+#include "Plugins/Process/Utility/ARMDefines.h"
+#include "Plugins/Process/Utility/InstructionUtils.h"
+#include "lldb/Core/ArchSpec.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Target/RegisterContext.h"
+#include "lldb/Target/Thread.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+ConstString ArchitectureArm::GetPluginNameStatic() {
+  return ConstString("arm");
+}
+
+void ArchitectureArm::Initialize() {
+  PluginManager::RegisterPlugin(GetPluginNameStatic(),
+                                "Arm-specific algorithms",
+                                &ArchitectureArm::Create);
+}
+
+void ArchitectureArm::Terminate() {
+  PluginManager::UnregisterPlugin(&ArchitectureArm::Create);
+}
+
+std::unique_ptr<Architecture> ArchitectureArm::Create(const ArchSpec &arch) {
+  if (arch.GetMachine() != llvm::Triple::arm)
+    return nullptr;
+  return std::unique_ptr<Architecture>(new ArchitectureArm());
+}
+
+ConstString ArchitectureArm::GetPluginName() { return GetPluginNameStatic(); }
+uint32_t ArchitectureArm::GetPluginVersion() { return 1; }
+
+void ArchitectureArm::OverrideStopInfo(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)
+    return;
+
+  const uint32_t cpsr = reg_ctx_sp->GetFlags(0);
+  if (cpsr == 0)
+    return;
+
+  // 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();
+    Status 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());
+      }
+    }
+  }
+}

lldb/trunk/source/Plugins/Architecture/Arm/CMakeLists.txt

+add_lldb_library(lldbPluginArchitectureArm PLUGIN
+  ArchitectureArm.cpp
+
+  LINK_LIBS
+    lldbPluginProcessUtility
+    lldbCore
+    lldbTarget
+  LINK_COMPONENTS
+    Support
+  )

lldb/trunk/source/Plugins/Architecture/CMakeLists.txt

+add_subdirectory(Arm)

lldb/trunk/source/Plugins/CMakeLists.txt

 add_subdirectory(ABI)
+add_subdirectory(Architecture)
 add_subdirectory(Disassembler)
 add_subdirectory(DynamicLoader)
 add_subdirectory(ExpressionParser)
 add_subdirectory(Instruction)
 add_subdirectory(InstrumentationRuntime)
 add_subdirectory(JITLoader)
 add_subdirectory(Language)
 add_subdirectory(LanguageRuntime)

lldb/trunk/source/Target/Process.cpp

: ProcessProperties(this), UserID(LLDB_INVALID_PROCESS_ID),
       m_notifications(), m_image_tokens(), m_listener_sp(listener_sp),
       m_breakpoint_site_list(), m_dynamic_checkers_ap(),
       m_unix_signals_sp(unix_signals_sp), m_abi_sp(), m_process_input_reader(),
       m_stdio_communication("process.stdio"), m_stdio_communication_mutex(),
       m_stdin_forward(false), m_stdout_data(), m_stderr_data(),
       m_profile_data_comm_mutex(), m_profile_data(), m_iohandler_sync(0),
       m_memory_cache(*this), m_allocated_memory_cache(*this),
       m_should_detach(false), m_next_event_action_ap(), m_public_run_lock(),
-      m_private_run_lock(), m_stop_info_override_callback(nullptr),
-      m_finalizing(false), m_finalize_called(false),
+      m_private_run_lock(), m_finalizing(false), m_finalize_called(false),
       m_clear_thread_plans_on_stop(false), m_force_next_event_delivery(false),
       m_last_broadcast_state(eStateInvalid), m_destroy_in_process(false),
       m_can_interpret_function_calls(false), m_warnings_issued(),
       m_run_thread_plan_lock(), m_can_jit(eCanJITDontKnow) {
   CheckInWithManager();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   if (log)
void Process::Finalize() {
   std::vector<Notifications> empty_notifications;
   m_notifications.swap(empty_notifications);
   m_image_tokens.clear();
   m_memory_cache.Clear();
   m_allocated_memory_cache.Clear();
   m_language_runtimes.clear();
   m_instrumentation_runtimes.clear();
   m_next_event_action_ap.reset();
-  m_stop_info_override_callback = nullptr;
   // Clear the last natural stop ID since it has a strong
   // reference to this process
   m_mod_id.SetStopEventForLastNaturalStopID(EventSP());
   //#ifdef LLDB_CONFIGURATION_DEBUG
   //    StreamFile s(stdout, false);
   //    EventSP event_sp;
   //    while (m_private_state_listener_sp->GetNextEvent(event_sp))
   //    {

 Status Process::Launch(ProcessLaunchInfo &launch_info) {
   Status error;
   m_abi_sp.reset();
   m_dyld_ap.reset();
   m_jit_loaders_ap.reset();
   m_system_runtime_ap.reset();
   m_os_ap.reset();
   m_process_input_reader.reset();
-  m_stop_info_override_callback = nullptr;
 
   Module *exe_module = GetTarget().GetExecutableModulePointer();
   if (exe_module) {
     char local_exec_file_path[PATH_MAX];
     char platform_exec_file_path[PATH_MAX];
     exe_module->GetFileSpec().GetPath(local_exec_file_path,
                                       sizeof(local_exec_file_path));
     exe_module->GetPlatformFileSpec().GetPath(platform_exec_file_path,
if (exe_module->GetFileSpec().Exists()) {
             // ProcessIOHandler.
             SetPublicState(state, false);
 
             if (PrivateStateThreadIsValid())
               ResumePrivateStateThread();
             else
               StartPrivateStateThread();
 
-            m_stop_info_override_callback =
-                GetTarget().GetArchitecture().GetStopInfoOverrideCallback();
-
             // 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) {
             // We exited while trying to launch somehow.  Don't call DidLaunch

 
 Status Process::Attach(ProcessAttachInfo &attach_info) {
   m_abi_sp.reset();
   m_process_input_reader.reset();
   m_dyld_ap.reset();
   m_jit_loaders_ap.reset();
   m_system_runtime_ap.reset();
   m_os_ap.reset();
-  m_stop_info_override_callback = nullptr;
 
   lldb::pid_t attach_pid = attach_info.GetProcessID();
   Status error;
   if (attach_pid == LLDB_INVALID_PROCESS_ID) {
     char process_name[PATH_MAX];
 
     if (attach_info.GetExecutableFile().GetPath(process_name,
                                                 sizeof(process_name))) {
if (log) {
       ModuleSP exe_module_sp = GetTarget().GetExecutableModule();
       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();
 }
 
 Status 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.
void Process::DidExec() {
   m_dyld_ap.reset();
   m_jit_loaders_ap.reset();
   m_image_tokens.clear();
   m_allocated_memory_cache.Clear();
   m_language_runtimes.clear();
   m_instrumentation_runtimes.clear();
   m_thread_list.DiscardThreadPlans();
   m_memory_cache.Clear(true);
-  m_stop_info_override_callback = nullptr;
   DoDidExec();
   CompleteAttach();
   // Flush the process (threads and all stack frames) after running
   // CompleteAttach()
   // in case the dynamic loader loaded things in new locations.
   Flush();
 
   // After we figure out what was loaded/unloaded in CompleteAttach,

lldb/trunk/source/Target/Target.cpp

 #include "lldb/Breakpoint/BreakpointResolverFileLine.h"
 #include "lldb/Breakpoint/BreakpointResolverFileRegex.h"
 #include "lldb/Breakpoint/BreakpointResolverName.h"
 #include "lldb/Breakpoint/Watchpoint.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/Section.h"
 #include "lldb/Core/SourceManager.h"
 #include "lldb/Core/State.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/Host.h"

 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
 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;
 }
 
 Target::Target(Debugger &debugger, const ArchSpec &target_arch,
                const lldb::PlatformSP &platform_sp, bool is_dummy_target)
     : TargetProperties(this),
       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)
 
 {
   SetEventName(eBroadcastBitBreakpointChanged, "breakpoint-changed");
   SetEventName(eBroadcastBitModulesLoaded, "modules-loaded");
   SetEventName(eBroadcastBitModulesUnloaded, "modules-unloaded");
   SetEventName(eBroadcastBitWatchpointChanged, "watchpoint-changed");
   SetEventName(eBroadcastBitSymbolsLoaded, "symbols-loaded");
 
   CheckInWithManager();
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   if (log)
     log->Printf("%p Target::Target()", static_cast<void *>(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());
   }
 }
 
 Target::~Target() {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_OBJECT));
   if (log)
     log->Printf("%p Target::~Target()", static_cast<void *>(this));
   DeleteCurrentProcess();
void Target::SetREPL(lldb::LanguageType language, lldb::REPLSP repl_sp) {
   m_repl_map[language] = repl_sp;
 }
 
 void Target::Destroy() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   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;
   m_breakpoint_list.RemoveAll(notify);
   m_internal_breakpoint_list.RemoveAll(notify);
   m_last_created_breakpoint.reset();
   m_last_created_watchpoint.reset();
   m_search_filter_sp.reset();
void Target::ClearModules(bool delete_locations) {
   m_section_load_history.Clear();
   m_images.Clear();
   m_scratch_type_system_map.Clear();
   m_ast_importer_sp.reset();
 }
 
 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::SetExecutableModule(ModuleSP &executable_sp,
                                  bool get_dependent_files) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
   ClearModules(false);
 
   if (executable_sp) {
     static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
     Timer scoped_timer(func_cat,
                        "Target::SetExecutableModule (executable = '%s')",
                        executable_sp->GetFileSpec().GetPath().c_str());
 
     m_images.Append(executable_sp); // The first image is our executable file
 
     // If we haven't set an architecture yet, reset our architecture based on
     // what we found in the executable module.
-    if (!m_arch.IsValid()) {
+    if (!m_arch.GetSpec().IsValid()) {
       m_arch = executable_sp->GetArchitecture();
-      if (log)
-        log->Printf("Target::SetExecutableModule setting architecture to %s "
-                    "(%s) based on executable file",
-                    m_arch.GetArchitectureName(),
-                    m_arch.GetTriple().getTriple().c_str());
+      LLDB_LOG(log,
+               "setting architecture to {0} ({1}) based on executable file",
+               m_arch.GetSpec().GetArchitectureName(),
+               m_arch.GetSpec().GetTriple().getTriple());
     }
 
     FileSpecList dependent_files;
     ObjectFile *executable_objfile = executable_sp->GetObjectFile();
 
     if (executable_objfile && get_dependent_files) {
       executable_objfile->GetDependentModules(dependent_files);
       for (uint32_t i = 0; i < dependent_files.GetSize(); i++) {
         FileSpec dependent_file_spec(
             dependent_files.GetFileSpecPointerAtIndex(i));
         FileSpec platform_dependent_file_spec;
         if (m_platform_sp)
           m_platform_sp->GetFileWithUUID(dependent_file_spec, nullptr,
                                          platform_dependent_file_spec);
         else
           platform_dependent_file_spec = dependent_file_spec;
 
-        ModuleSpec module_spec(platform_dependent_file_spec, m_arch);
+        ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
         ModuleSP image_module_sp(GetSharedModule(module_spec));
         if (image_module_sp) {
           ObjectFile *objfile = image_module_sp->GetObjectFile();
           if (objfile)
             objfile->GetDependentModules(dependent_files);
         }
       }
     }
   }
 }
 
 bool Target::SetArchitecture(const ArchSpec &arch_spec) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_TARGET));
-  bool missing_local_arch = !m_arch.IsValid();
+  bool missing_local_arch = !m_arch.GetSpec().IsValid();
   bool replace_local_arch = true;
   bool compatible_local_arch = false;
   ArchSpec other(arch_spec);
 
   if (!missing_local_arch) {
-    if (m_arch.IsCompatibleMatch(arch_spec)) {
-      other.MergeFrom(m_arch);
+    if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
+      other.MergeFrom(m_arch.GetSpec());
 
-      if (m_arch.IsCompatibleMatch(other)) {
+      if (m_arch.GetSpec().IsCompatibleMatch(other)) {
         compatible_local_arch = true;
         bool arch_changed, vendor_changed, os_changed, os_ver_changed,
             env_changed;
 
-        m_arch.PiecewiseTripleCompare(other, arch_changed, vendor_changed,
+        m_arch.GetSpec().PiecewiseTripleCompare(other, arch_changed, vendor_changed,
                                       os_changed, os_ver_changed, env_changed);
 
         if (!arch_changed && !vendor_changed && !os_changed && !env_changed)
           replace_local_arch = false;
       }
     }
   }
 
   if (compatible_local_arch || missing_local_arch) {
     // If we haven't got a valid arch spec, or the architectures are compatible
     // update the architecture, unless the one we already have is more specified
     if (replace_local_arch)
       m_arch = other;
-    if (log)
-      log->Printf("Target::SetArchitecture set architecture to %s (%s)",
-                  m_arch.GetArchitectureName(),
-                  m_arch.GetTriple().getTriple().c_str());
+    LLDB_LOG(log, "set architecture to {0} ({1})",
+             m_arch.GetSpec().GetArchitectureName(),
+             m_arch.GetSpec().GetTriple().getTriple());
     return true;
   }
 
   // If we have an executable file, try to reset the executable to the desired
   // architecture
   if (log)
     log->Printf("Target::SetArchitecture changing architecture to %s (%s)",
                 arch_spec.GetArchitectureName(),
if (!error.Fail() && executable_sp) {
       return true;
     }
   }
   return false;
 }
 
 bool Target::MergeArchitecture(const ArchSpec &arch_spec) {
   if (arch_spec.IsValid()) {
-    if (m_arch.IsCompatibleMatch(arch_spec)) {
+    if (m_arch.GetSpec().IsCompatibleMatch(arch_spec)) {
       // The current target arch is compatible with "arch_spec", see if we
       // can improve our current architecture using bits from "arch_spec"
 
       // Merge bits from arch_spec into "merged_arch" and set our architecture
-      ArchSpec merged_arch(m_arch);
+      ArchSpec merged_arch(m_arch.GetSpec());
       merged_arch.MergeFrom(arch_spec);
       return SetArchitecture(merged_arch);
     } else {
       // The new architecture is different, we just need to replace it
       return SetArchitecture(arch_spec);
     }
   }
   return false;
size_t Target::ReadScalarIntegerFromMemory(const Address &addr,
                                            uint32_t byte_size, bool is_signed,
                                            Scalar &scalar, Status &error) {
   uint64_t uval;
 
   if (byte_size <= sizeof(uval)) {
     size_t bytes_read =
         ReadMemory(addr, prefer_file_cache, &uval, byte_size, error);
     if (bytes_read == byte_size) {
-      DataExtractor data(&uval, sizeof(uval), m_arch.GetByteOrder(),
-                         m_arch.GetAddressByteSize());
+      DataExtractor data(&uval, sizeof(uval), m_arch.GetSpec().GetByteOrder(),
+                         m_arch.GetSpec().GetAddressByteSize());
       lldb::offset_t offset = 0;
       if (byte_size <= 4)
         scalar = data.GetMaxU32(&offset, byte_size);
       else
         scalar = data.GetMaxU64(&offset, byte_size);
 
       if (is_signed)
         scalar.SignExtend(byte_size * 8);
if (ReadScalarIntegerFromMemory(addr, prefer_file_cache, integer_byte_size,
     return scalar.ULongLong(fail_value);
   return fail_value;
 }
 
 bool Target::ReadPointerFromMemory(const Address &addr, bool prefer_file_cache,
                                    Status &error, Address &pointer_addr) {
   Scalar scalar;
   if (ReadScalarIntegerFromMemory(addr, prefer_file_cache,
-                                  m_arch.GetAddressByteSize(), false, scalar,
+                                  m_arch.GetSpec().GetAddressByteSize(), false, scalar,
                                   error)) {
     addr_t pointer_vm_addr = scalar.ULongLong(LLDB_INVALID_ADDRESS);
     if (pointer_vm_addr != LLDB_INVALID_ADDRESS) {
       SectionLoadList &section_load_list = GetSectionLoadList();
       if (section_load_list.IsEmpty()) {
         // No sections are loaded, so we must assume we are not running
         // yet and anything we are given is a file address.
         m_images.ResolveFileAddress(pointer_vm_addr, pointer_addr);
m_scratch_type_system_map.ForEach(
         return true; // Keep iterating the ForEach
       });
   return address;
 }
 
 lldb::addr_t Target::GetCallableLoadAddress(lldb::addr_t load_addr,
                                             AddressClass addr_class) const {
   addr_t code_addr = load_addr;
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
     switch (addr_class) {
     case eAddressClassData:
     case eAddressClassDebug:
       return LLDB_INVALID_ADDRESS;
default:
     break;
   }
   return code_addr;
 }
 
 lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
                                           AddressClass addr_class) const {
   addr_t opcode_addr = load_addr;
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el:
   case llvm::Triple::arm:
   case llvm::Triple::thumb:
     switch (addr_class) {
     case eAddressClassData:
lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
   }
   return opcode_addr;
 }
 
 lldb::addr_t Target::GetBreakableLoadAddress(lldb::addr_t addr) {
   addr_t breakable_addr = addr;
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
 
-  switch (m_arch.GetMachine()) {
+  switch (m_arch.GetSpec().GetMachine()) {
   default:
     break;
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el: {
     addr_t function_start = 0;
     addr_t current_offset = 0;
     uint32_t loop_count = 0;
     Address resolved_addr;
-    uint32_t arch_flags = m_arch.GetFlags();
+    uint32_t arch_flags = m_arch.GetSpec().GetFlags();
     bool IsMips16 = arch_flags & ArchSpec::eMIPSAse_mips16;
     bool IsMicromips = arch_flags & ArchSpec::eMIPSAse_micromips;
     SectionLoadList &section_load_list = GetSectionLoadList();
 
     if (section_load_list.IsEmpty())
       // No sections are loaded, so we must assume we are not running yet
       // and need to operate only on file address.
       m_images.ResolveFileAddress(addr, resolved_addr);
if (loop_count > 3) {
       // For mips-only, instructions are always 4 bytes, so scan previous 4
       // bytes only.
       else
         loop_count = 2;
     }
 
     // Create Disassembler Instance
     lldb::DisassemblerSP disasm_sp(
-        Disassembler::FindPlugin(m_arch, nullptr, nullptr));
+        Disassembler::FindPlugin(m_arch.GetSpec(), nullptr, nullptr));
 
     ExecutionContext exe_ctx;
     CalculateExecutionContext(exe_ctx);
     InstructionList instruction_list;
     InstructionSP prev_insn;
     bool prefer_file_cache = true; // Read from file
     uint32_t inst_to_choose = 0;
 

lldb/trunk/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);
+        if (Architecture *arch =
+                process_sp->GetTarget().GetArchitecturePlugin())
+          arch->OverrideStopInfo(*this);
       }
     }
   }
   return m_stop_info_sp;
 }
 
 lldb::StopReason Thread::GetStopReason() {
   lldb::StopInfoSP stop_info_sp(GetStopInfo());