[Target] Remove Process::GetCPPLanguageRuntime

[xiaobai]

Summary

I want to remove this method because I think that Process should be
language agnostic, or at least, not have knowledge about specific language
runtimes. In general, "GetLanguageRuntime()" should be used unless you
specifically need a CPPLanguageRuntime, in which case you can now use
CPPLanguageRuntime::GetCPPLanguageRuntime.

The next step I would like to do is remove "Process::GetObjCLanguageRuntime()"
as well.
There are a lot more instances of that function being used, so I wanted to
upload this one first to get the general reception to this idea.

[aprantl]

source/Target/Process.cpp

1601 ↗

(On Diff #202497)

so what about this mutex at the new call sites?

[jingham]

This seems like carrying purity a little too far. You haven't removed the fact that the using code is explicitly dialing up the C++ language runtime, you've just made the call-site a little harder to read.

I don't see any problem with having explicit accessors for the commonly used language runtimes.

[aprantl]

I tend to agree with Jim's comment.

There is "GetLanguageRuntime()" which should be used instead.

Can you explain why that is?

[xiaobai]

In D62755#1525790, @jingham wrote:

This seems like carrying purity a little too far.

I disagree that it is "carrying purity a little too far". My goal is to see LLDB's non-plugin libraries be more language agnostic.
I have two motivations for this:

1. Better language support in LLDB, making it easier to add support for new languages in a clean fashion.
2. Shrinking the size of lldb-server. Though the lldb on llvm.org doesn't suffer from this problem too much, swift-lldb has an issue where the lldb-server you build is incredibly bloated. On Linux I am measuring about 162MB and on android I measured 61MB. It is incredibly difficult to shrink the size without first decoupling components. Because swift-lldb is a downstream project, I want to make sure that the abstractions there make sense with what is on llvm.org, so I am doing this work here first.

You haven't removed the fact that the using code is explicitly dialing up the C++ language runtime, you've just made the call-site a little harder to read.

Right, that was not an explicit goal. There are legitimate instances where you need to get the C++ language runtime, I'm not saying that this is a bad thing. I'm saying that if you need a CPPLanguageRuntime from your process, you should use GetLanguageRuntime() with LanguageType being eLanguageTypeC_plus_plus. If you need to call a method from CPPLanguageRuntime (or one its derived classes) that isn't a part of the LanguageRuntime interface, then you should explicitly cast it. I also think that this should happen in Plugins that need details about the C++ language runtime explicitly and not in non-plugin libraries.

I don't see any problem with having explicit accessors for the commonly used language runtimes.

Language runtimes are supposed to be plugins. I don't think Process needs to have knowledge about specific language runtime plugins. This particular instance is relatively harmless, but it is a necessary part of a larger change to decouple specific language details from the core lldb implementation.
I recognize that CPPLanguageRuntime currently also lives in Target, but I would like to see it moved to "source/Plugins/LanguageRuntime/CPlusPlus/". I think that before I try to move it, this change makes sense as a first step.

In D62755#1525810, @aprantl wrote:

I tend to agree with Jim's comment.

There is "GetLanguageRuntime()" which should be used instead.

Can you explain why that is?

I think my response to Jim above explains my reasoning here. If there's something I haven't addressed or you have other concerns, please let me know.

source/Target/Process.cpp

1601 ↗

(On Diff #202497)

GetLanguageRuntime locks the mutex as well.

[aprantl]

I disagree that it is "carrying purity a little too far". My goal is to see LLDB's non-plugin libraries be more language agnostic.

I have two motivations for this:

Better language support in LLDB, making it easier to add support for new languages in a clean fashion.
Shrinking the size of lldb-server. Though the lldb on llvm.org doesn't suffer from this problem too much, swift-lldb has an issue where the lldb-server you build is incredibly bloated. On Linux I am measuring about 162MB and on android I measured 61MB. It is incredibly difficult to shrink the size without first decoupling components. Because swift-lldb is a downstream project, I want to make sure that the abstractions there make sense with what is on llvm.org, so I am doing this work here first.

Those are good goals. Thank you for working on this!

I don't yet see the connection between those goals and this patch, but I might be missing something. Would CPPLanguageRuntime need to be anything but a forward declaration in Process.h?

[xiaobai]

In D62755#1525890, @aprantl wrote:

Those are good goals. Thank you for working on this!

Thank you for taking the time to review this and discuss this with me! :)

I don't yet see the connection between those goals and this patch, but I might be missing something. Would CPPLanguageRuntime need to be anything but a forward declaration in Process.h?

Apologies for not making this clearer. I view this patch as a step in moving all non-plugin libraries over to using the LanguageRuntime interface instead of using CPPLanguageRuntime and ObjCLanguageRuntime directly. This particular patch is a part of making Target more language agnostic, as I've been doing over the past few weeks. Here are some other commits that I've recently made towards that goal: rL362164, rL362154, rL361999, rL360945.

Somewhat related, earlier this month I made the Breakpoint library language agnostic with rL360509. This is what I want to do, but for each of the non-plugin libraries (e.g. Target).

[xiaobai]

In D62755#1525890, @aprantl wrote:

I don't yet see the connection between those goals and this patch, but I might be missing something. Would CPPLanguageRuntime need to be anything but a forward declaration in Process.h?

I forgot to address this point. CPPLanguageRuntime would not be a forward declaration in Process.h, nor be referred to anywhere in Process.h

[jingham]

In D62755#1525919, @xiaobai wrote:

In D62755#1525890, @aprantl wrote:

I don't yet see the connection between those goals and this patch, but I might be missing something. Would CPPLanguageRuntime need to be anything but a forward declaration in Process.h?

I forgot to address this point. CPPLanguageRuntime would not be a forward declaration in Process.h, nor be referred to anywhere in Process.h

Yeah, getting CPPLanguageRuntime out of the general forward declarations does seem a worthy goal. But it would be great to do it in a way that doesn't add so much line noise. I added these methods because it got tiresome to write and read the version you are showing...

Maybe we can add a static:

CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process);

to CPPLanguageRuntime.h?

If you are calling LanguageRuntime methods on the CPPLanguageRuntime, you shouldn't need the cast and probably shouldn't be including CPPLanguageRuntime.h. So in that case, you'd just do:

#include "Target/LanguageRuntime.h"
...

LanguageRuntime *cpp_runtime = process->GetLanguageRuntime(eLanguageTypeCPlusPlus)
cpp_runtime->GenericMethod();

which is easy to type and read.

You only need to get a runtime cast as a CPPLanguageRuntime if you are planning to call methods that are in CPPLanguageRuntime.h, so you would be including that header anyway, and then you could use the convenience method from there to improve readability.

[xiaobai]

In D62755#1525937, @jingham wrote:

Yeah, getting CPPLanguageRuntime out of the general forward declarations does seem a worthy goal. But it would be great to do it in a way that doesn't add so much line noise. I added these methods because it got tiresome to write and read the version you are showing...

Maybe we can add a static:

CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process);

to CPPLanguageRuntime.h?

If you are calling LanguageRuntime methods on the CPPLanguageRuntime, you shouldn't need the cast and probably shouldn't be including CPPLanguageRuntime.h. So in that case, you'd just do:

#include "Target/LanguageRuntime.h"
...

LanguageRuntime *cpp_runtime = process->GetLanguageRuntime(eLanguageTypeCPlusPlus)
cpp_runtime->GenericMethod();

which is easy to type and read.

You only need to get a runtime cast as a CPPLanguageRuntime if you are planning to call methods that are in CPPLanguageRuntime.h, so you would be including that header anyway, and then you could use the convenience method from there to improve readability.

What you're saying makes sense to me. Since it's true that casting requires including CPPLanguageRuntime.h in the first place, adding it as a static method to CPPLanguageRuntime itself shouldn't be a problem. I don't think this will be an issue when I try to move CPPLanguageRuntime into the Plugins directory either, so I'm perfectly fine with this solution. I will also see if I can remove CPPLanguageRuntime from the forward declarations, and will try to update this diff tonight or tomorrow.

[xiaobai]

Jim's suggestion

[labath]

I agree with your reasoning about language runtime dependencies, and I think this is the right way to accomplish this.

However, I just want to add that from an lldb-server POV, even the fact that we pull in the Process class into it's dependency graph is a bug. So, what I'd do (and what I have been doing so far, but mainly as a passtime effort) was to cut the dependencies way lower that this. Ideally, I wouldn't want to include anything from the Target library in lldb-server, possibly by moving some stuff that lldb-server does need to use (MemoryRegionInfo) out of it.

But, you know, there are multiple angles from which you can attack this problem, and if this results in dependency cleanups in the rest of lldb, then I don't see a reason to not proceed here.

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

It might be nice to add some glue so we could write llvm::cast_or_null<CPPLanguageRuntime>(...) here. The main advantage of that being that we'd automatically get an assert firing if GetLanguageRuntime ever returns something which is *not* a CPPLanguageRuntime.

[xiaobai]

In D62755#1527004, @labath wrote:

However, I just want to add that from an lldb-server POV, even the fact that we pull in the Process class into it's dependency graph is a bug.

Agreed

So, what I'd do (and what I have been doing so far, but mainly as a passtime effort) was to cut the dependencies way lower that this. Ideally, I wouldn't want to include anything from the Target library in lldb-server, possibly by moving some stuff that lldb-server does need to use (MemoryRegionInfo) out of it.

Yeah, I'd like to see this happen as well. My efforts are twofold: Remove non-plugin libraries dependencies on plugin libraries, and move things out of the core libraries into plugins (and possibly new non-plugin libraries, if it makes sense). There are many dependency issues that I'm not yet aware of. I'm hoping to uncover more as I keep decoupling.

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

I had thought about doing that, but there's an assertion in GetLanguageRuntime that achieves the same thing imo. I do think that we could move towards a llvm::cast_or_null implementation though.

[JDevlieghere]

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

I think would be worthwhile too :-)

[jingham]

This deals with my objections so I'm marking as accepted because that's the only way I can see to unblock. Seems like you were still discussing return types, and I don't want to preempt that, however...

[xiaobai]

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

Would y'all mind if I did that in a follow up commit? I think it is worthwhile/valuable, but I'd like it to be in a separate commit if possible.

[jingham]

Fine by me.

[JDevlieghere]

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

Sounds good

[labath]

include/lldb/Target/CPPLanguageRuntime.h

47 ↗

(On Diff #202523)

no problem here.

[labath]

In D62755#1527679, @xiaobai wrote:

So, what I'd do (and what I have been doing so far, but mainly as a passtime effort) was to cut the dependencies way lower that this. Ideally, I wouldn't want to include anything from the Target library in lldb-server, possibly by moving some stuff that lldb-server does need to use (MemoryRegionInfo) out of it.

Yeah, I'd like to see this happen as well. My efforts are twofold: Remove non-plugin libraries dependencies on plugin libraries, and move things out of the core libraries into plugins (and possibly new non-plugin libraries, if it makes sense). There are many dependency issues that I'm not yet aware of. I'm hoping to uncover more as I keep decoupling.

Sounds good. I'm looking forward to seeing how this turns out.

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

public:
   FindLibCppStdFunctionCallableInfo(lldb::ValueObjectSP &valobj_sp);
 
   ~CPPLanguageRuntime() override;
 
   lldb::LanguageType GetLanguageType() const override {
     return lldb::eLanguageTypeC_plus_plus;
   }
 
+  static CPPLanguageRuntime *GetCPPLanguageRuntime(Process &process) {
+    return static_cast<CPPLanguageRuntime *>(
+        process.GetLanguageRuntime(lldb::eLanguageTypeC_plus_plus));
+  }
+
   virtual bool IsVTableName(const char *name) = 0;
 
   bool GetObjectDescription(Stream &str, ValueObject &object) override;
 
   bool GetObjectDescription(Stream &str, Value &value,
                             ExecutionContextScope *exe_scope) override;
 
   /// Obtain a ThreadPlan to get us into C++ constructs such as std::function.

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

public:
   OperatingSystem *GetOperatingSystem() { return m_os_up.get(); }
 
   std::vector<LanguageRuntime *>
   GetLanguageRuntimes(bool retry_if_null = true);
 
   LanguageRuntime *GetLanguageRuntime(lldb::LanguageType language,
                                       bool retry_if_null = true);
 
-  CPPLanguageRuntime *GetCPPLanguageRuntime(bool retry_if_null = true);
-
   ObjCLanguageRuntime *GetObjCLanguageRuntime(bool retry_if_null = true);
 
   bool IsPossibleDynamicValue(ValueObject &in_value);
 
   bool IsRunning() const;
 
   DynamicCheckerFunctions *GetDynamicCheckers() {
     return m_dynamic_checkers_up.get();

lldb/trunk/include/lldb/lldb-forward.h

 class BreakpointOptionGroup;
 class BreakpointOptions;
 class BreakpointResolver;
 class BreakpointSite;
 class BreakpointSiteList;
 class BroadcastEventSpec;
 class Broadcaster;
 class BroadcasterManager;
-class CPPLanguageRuntime;
 class ClangASTContext;
 class ClangASTImporter;
 class ClangASTMetadata;
 class ClangASTSource;
 class ClangExpressionDeclMap;
 class ClangExpressionParser;
 class ClangExpressionVariable;
 class ClangModulesDeclVendor;

lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp

if (!valobj_sp)
     return false;
 
   ExecutionContext exe_ctx(valobj_sp->GetExecutionContextRef());
   Process *process = exe_ctx.GetProcessPtr();
 
   if (process == nullptr)
     return false;
 
-  CPPLanguageRuntime *cpp_runtime = process->GetCPPLanguageRuntime();
+  CPPLanguageRuntime *cpp_runtime =
+      CPPLanguageRuntime::GetCPPLanguageRuntime(*process);
 
   if (!cpp_runtime)
     return false;
 
   CPPLanguageRuntime::LibCppStdFunctionCallableInfo callable_info =
       cpp_runtime->FindLibCppStdFunctionCallableInfo(valobj_sp);
 
   switch (callable_info.callable_case) {

lldb/trunk/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp

if (target.GetArchitecture().GetTriple().getVendor() == llvm::Triple::Apple) {
     return target.GetSearchFilterForModuleList(&filter_modules);
   } else {
     return LanguageRuntime::CreateExceptionSearchFilter();
   }
 }
 
 ValueObjectSP AppleObjCRuntime::GetExceptionObjectForThread(
     ThreadSP thread_sp) {
-  auto cpp_runtime = m_process->GetCPPLanguageRuntime();
+  auto *cpp_runtime = m_process->GetLanguageRuntime(eLanguageTypeC_plus_plus);
   if (!cpp_runtime) return ValueObjectSP();
   auto cpp_exception = cpp_runtime->GetExceptionObjectForThread(thread_sp);
   if (!cpp_exception) return ValueObjectSP();
 
   auto descriptor = GetClassDescriptor(*cpp_exception);
   if (!descriptor || !descriptor->IsValid()) return ValueObjectSP();
   
   while (descriptor) {

lldb/trunk/source/Target/Process.cpp

if (runtime)
     // for example, CPPLanguageRuntime will support eLanguageTypeC_plus_plus,
     // eLanguageTypeC_plus_plus_03, etc. Because of this, we should get the
     // primary language type and make sure that our runtime supports it.
     assert(runtime->GetLanguageType() == Language::GetPrimaryLanguage(language));
 
   return runtime;
 }
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
-  LanguageRuntime *runtime =
-      GetLanguageRuntime(eLanguageTypeC_plus_plus, retry_if_null);
-  if (!runtime)
-    return nullptr;
-
-  return static_cast<CPPLanguageRuntime *>(runtime);
-}
-
 ObjCLanguageRuntime *Process::GetObjCLanguageRuntime(bool retry_if_null) {
   std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
   LanguageRuntime *runtime =
       GetLanguageRuntime(eLanguageTypeObjC, retry_if_null);
   if (!runtime)
     return nullptr;
 
   return static_cast<ObjCLanguageRuntime *>(runtime);