[Target] Generalize language-specific behavior in ThreadPlanStepThrough

[xiaobai]

Summary

When creating a ThreadPlan to step through a trampoline, we ask the
ObjC language runtime and the CPP language runtime to come up with such a thread
plan if the dynamic loader fails to give us one. I don't see why this behavior
can't be language agnostic.

[JDevlieghere]

source/Target/ThreadPlanStepThrough.cpp

91

Given that we seem to have this pattern in a bunch of places, would it be worth extracting this into some kind of convenience function?

96

Should there be a break here?

[xiaobai]

source/Target/ThreadPlanStepThrough.cpp

91

Yeah, I'm getting tired of writing it. We could pull it into Process, into a ForEachLanguageRuntime or something. I can either do that before this goes in or in a follow up

96

I put a !m_sub_plan_sp in the for loop's condition. If you think it would be more readable to put a break here, I could do that instead.

[JDevlieghere]

LGTM!

source/Target/ThreadPlanStepThrough.cpp

91

Alright, either works for me.

96

Aha, you're right, I missed that. I think the break is slightly more readable, but I'll leave it up to you.

[jingham]

There is a TypeSystemEnumerateSupportedLanguages that we use so that we don't have to enumerate over all the language in the languages enums. After all the plugin manager knows which languages we have type systems for. If you're going to be doing a lot of this generalization, can you add a similar enumeration for the LanguageRuntimes? There are 40 or so languages in the language enum but we only have a couple of LanguageRuntimes...

[xiaobai]

In D61921#1502254, @jingham wrote:

There is a TypeSystemEnumerateSupportedLanguages that we use so that we don't have to enumerate over all the language in the languages enums. After all the plugin manager knows which languages we have type systems for. If you're going to be doing a lot of this generalization, can you add a similar enumeration for the LanguageRuntimes? There are 40 or so languages in the language enum but we only have a couple of LanguageRuntimes...

My understanding is TypeSystemEnumerateSupportedLanguages is just for getting what languages are supported by the currently loaded TypeSystems (e.g. ClangASTContext) and not necessarily all the loaded language plugins. From what I understand, this is handled by Language itself using a map that it populates on-demand.

The same is probably achievable with LanguageRuntimes. I think adding a ForEachLanguageRuntime method to Process that would populate its LanguageRuntimes map on demand much like Language does would probably give us a similar effect. I could do this in a follow up commit to make these kinds of situations a little more efficient. What do you think of this plan?

[jingham]

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

It's fine to do that in a separate patch. If you do that it would be good to back port it to the other iterations over the LanguageRuntimes.

I don't see a really strong reason to make GetStepThroughTrampolinePlan a pure virtual method, this isn't required behavior for a language runtime. Why not make a default method that returns an empty thread plan?

[xiaobai]

In D61921#1502338, @jingham wrote:

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

It's fine to do that in a separate patch. If you do that it would be good to back port it to the other iterations over the LanguageRuntimes.

Excellent, sounds good. I definitely intend to backport it to any other iterations over LanguageRuntimes that I find. As for iterating over all available LanguageRuntimes, this patch and my last few have sufficient motivation for doing so, no? Maybe I'm misunderstanding what you mean.

I don't see a really strong reason to make GetStepThroughTrampolinePlan a pure virtual method, this isn't required behavior for a language runtime. Why not make a default method that returns an empty thread plan?

Thinking about it further, you're probably right here. Not every language might have the need to handle this scenario. I'll update this patch with your suggestion.

[labath]

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

[jingham]

In D61921#1502502, @labath wrote:

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

+1

[jingham]

In D61921#1502352, @xiaobai wrote:

In D61921#1502338, @jingham wrote:

Getting it from the Process's m_language_runtimes is probably fine. On reflection, I can't think of a reason why you would want to iterate over all the available LanguageRuntimes, including the ones that hadn't been recognized in this Process yet.

It's fine to do that in a separate patch. If you do that it would be good to back port it to the other iterations over the LanguageRuntimes.

Excellent, sounds good. I definitely intend to backport it to any other iterations over LanguageRuntimes that I find. As for iterating over all available LanguageRuntimes, this patch and my last few have sufficient motivation for doing so, no? Maybe I'm misunderstanding what you mean.

On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift. But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded. A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process. So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones. Not that we didn't need one or the other iterator.

I don't see a really strong reason to make GetStepThroughTrampolinePlan a pure virtual method, this isn't required behavior for a language runtime. Why not make a default method that returns an empty thread plan?

Thinking about it further, you're probably right here. Not every language might have the need to handle this scenario. I'll update this patch with your suggestion.

[xiaobai]

In D61921#1503282, @jingham wrote:

In D61921#1502502, @labath wrote:

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

+1

Yeah this seems reasonable.

In D61921#1503290, @jingham wrote:

On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift. But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded. A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process. So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones. Not that we didn't need one or the other iterator.

Okay, that makes sense to me. Thanks for clearing that up. If I understand correctly (which I think I do), creating an iterator abstraction to iterate over the process' currently loaded runtimes instead of over every available runtime is the better option here. I'll follow up with that change later today or tomorrow unless somebody believes this is the wrong idea.

[jingham]

In D61921#1503331, @xiaobai wrote:

In D61921#1503282, @jingham wrote:

In D61921#1502502, @labath wrote:

If this iteration is going to be used a lot, I'd recommend taking a bit of time to implement an iterator abstraction over the language runtimes. It takes a bit longer to set up, but I hope we can all agree that for (runtime: process->GetLanguageRuntimes()) runtime->foo(); is more readable than process->ForEachLanguageRuntime([] (runtime) { runtime->foo(); }). This is particularly true if you need some sort of a control flow construct (continue, break, return) in the loop "body".

+1

Yeah this seems reasonable.

In D61921#1503290, @jingham wrote:

On macOS, there are a handful of runtimes that the Plugin Manager knows about - C++, ObjC, maybe Swift. But, for instance, we only load the ObjC language runtime into the process' m_language_runtimes array when we see libobjc.dylib get loaded. A pure C++ program might never load libobjc.dylib, and so even though the Plugin Manager knows we have support for the ObjC language runtime, that plugin wouldn't be active in the current lldb_private::Process. So there's a real difference between the "available" and the "currently loaded" runtimes. I was saying I didn't see any compelling reason to have an iterator over the available runtimes, just over the loaded ones. Not that we didn't need one or the other iterator.

Okay, that makes sense to me. Thanks for clearing that up. If I understand correctly (which I think I do), creating an iterator abstraction to iterate over the process' currently loaded runtimes instead of over every available runtime is the better option here. I'll follow up with that change later today or tomorrow unless somebody believes this is the wrong idea.

That seems the right choice to me and should be easy to implement - since it's just iterating over a map. Iterable.h is some little dealie Sean whipped up a while ago that we use to make other contained maps iterable - e.g. the Types in a TypeMap. You could just reuse that.

[xiaobai]

Update to reflect the recently added method Process::GetLanguageRuntimes

[xiaobai]

@jingham @JDevlieghere Mind giving this a quick look? I modified it to use the new method I added earlier and wanted to make sure there were no outstanding issues here.

[jingham]

LGTM

include/lldb/Target/CPPLanguageRuntime.h

public:
   ///     Curent thrad of execution.
   ///
   /// \param[in] stop_others
   ///     True if other threads should pause during execution.
   ///
   /// \return
   ///      A ThreadPlan Shared pointer
   lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
-                                                  bool stop_others);
+                                                  bool stop_others) override;
 
   bool IsRuntimeSupportValue(ValueObject &valobj) override;
 protected:
   // Classes that inherit from CPPLanguageRuntime can see and modify these
   CPPLanguageRuntime(Process *process);
 
 private:
   DISALLOW_COPY_AND_ASSIGN(CPPLanguageRuntime);
 };
 
 } // namespace lldb_private
 
 #endif // liblldb_CPPLanguageRuntime_h_

include/lldb/Target/LanguageRuntime.h

public:
 
   virtual lldb::SearchFilterSP CreateExceptionSearchFilter();
 
   virtual bool GetTypeBitSize(const CompilerType &compiler_type,
                               uint64_t &size) {
     return false;
   }
 
+  virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
+                                                          bool stop_others) = 0;
+
   /// Identify whether a value is a language implementation detaul
   /// that should be hidden from the user interface by default.
   virtual bool IsRuntimeSupportValue(ValueObject &valobj) { return false; }
 
   virtual void ModulesDidLoad(const ModuleList &module_list) {}
 
   // Called by the Clang expression evaluation engine to allow runtimes to
   // alter the set of target options provided to the compiler. If the options

include/lldb/Target/ObjCLanguageRuntime.h

public:
   }
 
   virtual bool IsModuleObjCLibrary(const lldb::ModuleSP &module_sp) = 0;
 
   virtual bool ReadObjCLibrary(const lldb::ModuleSP &module_sp) = 0;
 
   virtual bool HasReadObjCLibrary() = 0;
 
-  virtual lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
-                                                          bool stop_others) = 0;
-
   lldb::addr_t LookupInMethodCache(lldb::addr_t class_addr, lldb::addr_t sel);
 
   void AddToMethodCache(lldb::addr_t class_addr, lldb::addr_t sel,
                         lldb::addr_t impl_addr);
 
   TypeAndOrName LookupInClassNameCache(lldb::addr_t class_addr);
 
   void AddToClassNameCache(lldb::addr_t class_addr, const char *name,

source/Target/ThreadPlanStepThrough.cpp

 }
 
 void ThreadPlanStepThrough::LookForPlanToStepThroughFromCurrentPC() {
   DynamicLoader *loader = m_thread.GetProcess()->GetDynamicLoader();
   if (loader)
     m_sub_plan_sp =
         loader->GetStepThroughTrampolinePlan(m_thread, m_stop_others);
 
-  // If that didn't come up with anything, try the ObjC runtime plugin:
-  if (!m_sub_plan_sp.get()) {
-    ObjCLanguageRuntime *objc_runtime =
-        m_thread.GetProcess()->GetObjCLanguageRuntime();
-    if (objc_runtime)
-      m_sub_plan_sp =
-          objc_runtime->GetStepThroughTrampolinePlan(m_thread, m_stop_others);
-
-    CPPLanguageRuntime *cpp_runtime =
-        m_thread.GetProcess()->GetCPPLanguageRuntime();
-
-    // If the ObjC runtime did not provide us with a step though plan then if we
-    // have it check the C++ runtime for a step though plan.
-    if (!m_sub_plan_sp.get() && cpp_runtime)
+  // If the DynamicLoader was unable to provide us with a ThreadPlan, then we
+  // try the LanguageRuntimes.
+  if (!m_sub_plan_sp) {
+    for (unsigned lang = eLanguageTypeUnknown;
+         !m_sub_plan_sp && lang < eNumLanguageTypes; lang++) {
+      if (auto runtime = m_thread.GetProcess()->GetLanguageRuntime(
+              static_cast<lldb::LanguageType>(lang))) {
         m_sub_plan_sp =
-          cpp_runtime->GetStepThroughTrampolinePlan(m_thread, m_stop_others);
+            runtime->GetStepThroughTrampolinePlan(m_thread, m_stop_others);
+      }
+    }
   }
 
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_STEP));
   if (log) {
     lldb::addr_t current_address = GetThread().GetRegisterContext()->GetPC(0);
     if (m_sub_plan_sp) {
       StreamString s;
       m_sub_plan_sp->GetDescription(&s, lldb::eDescriptionLevelFull);