[Lldb-commits] [PATCH] D61921: [Target] Generalize language-specific behavior in ThreadPlanStepThrough

Wed May 15 10:30:00 PDT 2019

jingham added a comment.

In D61921#1502352 <https://reviews.llvm.org/D61921#1502352>, @xiaobai wrote:

> In D61921#1502338 <https://reviews.llvm.org/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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61921/new/

https://reviews.llvm.org/D61921