Page MenuHomePhabricator

[Target] Generalize language-specific behavior in ThreadPlanStepThrough
ClosedPublic

Authored by xiaobai on May 14 2019, 3:19 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.May 14 2019, 3:19 PM
JDevlieghere added inline comments.May 14 2019, 3:22 PM
source/Target/ThreadPlanStepThrough.cpp
91 ↗(On Diff #199525)

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 ↗(On Diff #199525)

Should there be a break here?

xiaobai marked 2 inline comments as done.May 14 2019, 3:28 PM
xiaobai added inline comments.
source/Target/ThreadPlanStepThrough.cpp
91 ↗(On Diff #199525)

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 ↗(On Diff #199525)

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 accepted this revision.May 14 2019, 3:45 PM

LGTM!

source/Target/ThreadPlanStepThrough.cpp
91 ↗(On Diff #199525)

Alright, either works for me.

96 ↗(On Diff #199525)

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

This revision is now accepted and ready to land.May 14 2019, 3:45 PM

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

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?

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?

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 added a subscriber: labath.May 15 2019, 12:22 AM

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

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

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.

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.

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.

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.

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 updated this revision to Diff 202013.May 29 2019, 11:54 AM

Update to reflect the recently added method Process::GetLanguageRuntimes

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

JDevlieghere accepted this revision.May 30 2019, 1:13 PM
jingham accepted this revision.May 30 2019, 1:14 PM

LGTM

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2019, 3:00 PM