[Lldb-commits] [PATCH] D62755: [Target] Remove Process::GetCPPLanguageRuntime

[Alex Langford via Phabricator via lldb-commits]

xiaobai marked an inline comment as done.
xiaobai added a comment.

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



================
Comment at: source/Target/Process.cpp:1601
 
-CPPLanguageRuntime *Process::GetCPPLanguageRuntime(bool retry_if_null) {
-  std::lock_guard<std::recursive_mutex> guard(m_language_runtimes_mutex);
----------------
aprantl wrote:
> so what about this mutex at the new call sites?
GetLanguageRuntime locks the mutex as well.


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

https://reviews.llvm.org/D62755