Page MenuHomePhabricator

[Target] Remove Process::GetCPPLanguageRuntime
ClosedPublic

Authored by xiaobai on May 31 2019, 3:27 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.May 31 2019, 3:27 PM
aprantl added inline comments.May 31 2019, 3:37 PM
source/Target/Process.cpp
1601 ↗(On Diff #202497)

so what about this mutex at the new call sites?

jingham requested changes to this revision.May 31 2019, 3:41 PM

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.

This revision now requires changes to proceed.May 31 2019, 3:41 PM

I tend to agree with Jim's comment.

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

Can you explain why that is?

xiaobai marked an inline comment as done.May 31 2019, 4:33 PM

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.

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.

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?

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

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

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.

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 updated this revision to Diff 202523.May 31 2019, 7:15 PM

Jim's suggestion

xiaobai edited the summary of this revision. (Show Details)May 31 2019, 7:17 PM

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 marked an inline comment as done.Jun 3 2019, 9:43 AM

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 added inline comments.Jun 3 2019, 4:56 PM
include/lldb/Target/CPPLanguageRuntime.h
47 ↗(On Diff #202523)

I think would be worthwhile too :-)

jingham accepted this revision.Jun 3 2019, 5:00 PM

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

This revision is now accepted and ready to land.Jun 3 2019, 5:00 PM
xiaobai marked an inline comment as done.Jun 3 2019, 5:11 PM
xiaobai added inline comments.
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.

Fine by me.

JDevlieghere added inline comments.Jun 3 2019, 6:36 PM
include/lldb/Target/CPPLanguageRuntime.h
47 ↗(On Diff #202523)

Sounds good

labath accepted this revision.Jun 3 2019, 11:01 PM
labath added inline comments.
include/lldb/Target/CPPLanguageRuntime.h
47 ↗(On Diff #202523)

no problem here.

labath added a comment.Jun 4 2019, 3:41 AM

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2019, 1:14 PM