Page MenuHomePhabricator

Move NativeProcessProtocol and friends from Host/ to Target/
AbandonedPublic

Authored by zturner on Nov 4 2014, 1:03 PM.

Details

Reviewers
clayborg
jingham
Summary

I moved this for a number reasons:

  1. There is no platform-specific code here so it does not seem to belong in Host
  2. It is conceptually related to Target, and seems more appropriate there.
  3. Code is using hackish include paths to #include from the source tree

Overall, it just seems like this code was in the wrong place.

Diff Detail

Event Timeline

zturner updated this revision to Diff 15782.Nov 4 2014, 1:03 PM
zturner retitled this revision from to Move NativeProcessProtocol and friends from Host/ to Target/.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: jingham, clayborg.
zturner added a subscriber: Unknown Object (MLST).
clayborg accepted this revision.Nov 4 2014, 1:11 PM
clayborg edited edge metadata.

The theory here is that the stuff would be for the host layer and would be used in NativeProcess and NativeThread, which do belong in the host layer. I am fine with the move if it is needed, but the theory is these are base classes for code that should be in the host layer. That said, the base classes can go in Target.

This revision is now accepted and ready to land.Nov 4 2014, 1:11 PM

Currently they're only base classes for stuff that are used in process
plugins. By NativeProcess and NativeThread, do you mean NativeProcessLinux
and NativeThreadLinux? Because those are in Plugins/Process/Linux, for
example.

The way these classes are used right now seems to be as utility classes to
facilitate communication between code that runs in the process plugin and
the lldb_private::Process object to update state and whatnot.

jingham edited edge metadata.Nov 4 2014, 2:13 PM

These files seem like they are photo-ProcessPlugin classes. Wouldn’t it be better to put then in the Plugins/Process directory under Native?

Jim

Target is getting crowded already, I’d rather

Well, they're used by lldb-gdbserver, and Plugins doesn't expose an include
directory. I mean it can just #include into the source tree, but that's
kind of what I was trying to fix. Can we create include/lldb/Plugins, as a
place to hold files that are usable for all plugins?

And does it make sense for llgs to be using files that are "plugin files"?

Looking at this more closely, it seems to me like the process plugins contain two disjoint codepaths. An llgs codepath (NativeProcessLinux, NativeProcessProtocol, etc) and a local debugging codepath (ProcessLinux, ProcessMonitor, etc).

This whole patch arose out of an effort to get local debugging working on Windows, because it looked like I needed to go through these NativeProcessProtocol and whatever. But now it looks like this is not the case.

It's not entirely clear why more of the code from ProcessLinux and the local debugging codepath wasn't re-used for the llgs side, but either way it looks like maybe I don't need to go down this route, and I can just have my debug driver thread communicate directly with ProcessWindows.

I'm happy to still commit this change (or make improvements as requested) if there's a better way to organize the code, but I'm also okay abandoning it since it seems it isn't necessary to get debugging working on this end.

As I understood it originally, the NativeProcess stuff would end up replacing the guts of the ProcessLinux, etc, but in a way that would easily allow you to slot them into llgs as well. That’s why I thought of them as some kind ur-process plugin.

Jim

In D6123#11, @jingham wrote:

As I understood it originally, the NativeProcess stuff would end up replacing the guts of the ProcessLinux, etc, but in a way that would easily allow you to slot them into llgs as well. That’s why I thought of them as some kind ur-process plugin.

Jim

Yea, in my mind I've always imagined llgs as just using all of the local debugging process plugin stuff as well. Part of the reason why I'm not too concerned with it atm for Windows, because when it does come time for it, all the hard stuff will already be written in the ProcessWindows plugin, and you just have to glue it together.

I guess what we're looking at now is the unfinished glue, so there's a little bit of both. It probably would have been less confusing if there were like ProcessLinuxRemote so that it's clear that for now, the two codepaths don't use each other.

Either way, I'll go ahead and put this stuff in Plugins and make a Plugins include directory for now. Going forward I'm going to keep modularity in mind when getting local debugging stuff working on Windows, so that we can reuse all of it when llgs starts coming together.

As an aside, is there a roadmap for getting all supported Darwin platforms
on llgs for both local and remote debugging?

zturner abandoned this revision.Nov 4 2014, 3:22 PM

Based on Jim's comments, it sounds like this code might be moving around in the future anyway, once the Native stuff goes away and a better code organization is finalized. Based on that, and since this change doesn't actually solve a problem I'm having, I will abandon it for now. No point in making a cosmetic change that affects the blame history if it doesn't actually improve anything.

In D6123#14, @zturner wrote:

Based on Jim's comments, it sounds like this code might be moving around in the future anyway, once the Native stuff goes away and a better code organization is finalized. Based on that, and since this change doesn't actually solve a problem I'm having, I will abandon it for now. No point in making a cosmetic change that affects the blame history if it doesn't actually improve anything.

I mean once the ProcessLinux / Monitor / etc stuff goes away.

Sure, that seems fine to me.

Jim