Some observations on the following sample sample.o with GNU objcopy.

Any chance to restore the tracking history as well?

In D63166#1539491, @labath wrote:

Given that you're just moving code around, this should be fine, but I am including @amccarth, as I believe he is more familiar with this code.

The thing I would consider in your place is whether putting this into a separate class is enough, or if you want to put it into a separate module/subfolder as well. The reason for that is that if this is in the same module as the liblldb stuff, then you will still end up pulling the all the transitive deps into lldb-server. If you put that into a separate module then you can control the dependencies more precisely. I don't think this is that important, since hopefully the local codepath (along with the deps) will disappear once lldb-server starts to work, but you know.. temporary solutions have a tendency for becoming permanent... Anyway, I'll leave the decision up to you..

In D63165#1539118, @amccarth wrote:

Sorry for the stupid question, but ...

What exactly is meant here by "Native"? How is a NativeProcessWindows different from ProcessWindows?

The Native*** classes are meant to be used from lldb-server. They look somewhat similar to their non-native counterpart because they still do debugging, but they're a lot dumber, because they only deal with basic process control, and none of the fancy symbolic stuff that you'd need debug info for.

In D56237#1394320, @labath wrote:

I like the direction this is going. I have one small comment about the code organization inline, but the bigger question I have in mind is: After these changes, is there any use for Host::GetProcessBaseAddress left? If so, what is it? I'd like to understand it and remove it.

In D56237#1383070, @labath wrote:

Going to the Host straight from the DynamicLoader is the worst possible option, since then this code will not be correct for remote processes (it will just randomly pick up some data from the local process with the same pid, if you happen to have one). I'd like to hear what you think about the latest comments by Greg & myself.

In D56237#1373788, @clayborg wrote:

In D56237#1373234, @labath wrote:

I think there are some architectural issues here in how ProcessWindows works. Normally (i.e., on all non-windows process classes), it is the job of the dynamic loader plugin to create modules and set their load addresses. However, ProcessWindows does things differently, and creates the modules by itself (ProcessWindows::OnLoadDLL).
I think the behavior of ProcessWindows is fine (it flows naturally from how modules are handled on windows), but if it's going to handle module loading by itself, then I think it should do the job completely, and not only half-way. So, if we want ProcessWindows to do the module loading, then it should return a null value for GetDynamicLoader() (or a reasonably stubbed out implementation) instead of relying on a generic dynamic loader plugin to finish the job.

Or, if we want to have the DynamicLoader plugin, then ProcessWindows should stop meddling with the module list in the OnLoadDLL function. One way to achieve that would be:

• ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
• dynamic loader loads the module and sets load address
• dynamic loader does not have to call ProcessWindows::GetFileLoadAddress as it already got the load address from in the OnLoadDLL function

This would make the flow very similar to how other platforms handle it (the only difference would be that the "OnLoadDLL" equivalent is called from a breakpoint callback function, instead of from a special debug event callback, but that's just due to a difference in how modules are reported).

I would like the see the dynamic loader do all of the work. We can easily add any new virtual functions to the DynamicLoader class to fit how windows does things.

That should be fairly easy to achieve. The process class is the one that creates the dynamic loader (in GetDynamicLoader()), so it can easily call any special method on it that it needs from OnLoadDLL (i.e., we don't even need to extend the public DynamicLoader API).

However, note that this would not be the first process class that handles module loading on its own. ProcessMinidump already does that too. It could be changed to use a special dynamic loader too, but it's not clear to me whether that is worth the trouble, as the loading process for minidumps is very simple.

In D56237#1344682, @labath wrote:

I think there is something wrong here. Normally, it should be the job of the dynamic loader plugin to figure out where a module got loaded (and populate the target section load list). Process::GetFileLoadAddress is there to help it with this job. So, the fact that ProcessWindows::GetFileLoadAddress goes back to the target section load list to get the data creates a weird loop. The only way I can see this working is if this implementation happens to return the module base address, as specified in the object file, and that base address happens to be correct (no ASLR). The kind of code I would expect to see here is a call to some windows API to determine the load address straight from the OS (if such a thing is possible). For example, on Linux, we parse the /proc/<pid>/maps file to determine this information.

In D56230#1361746, @Hui wrote:

It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the command “dir c:\" for Windows Command Terminal.
However it is not an issue for PS or MingGW.

It is observed the flattened one is

"\"C:\\WINDOWS\\system32\\cmd.exe\" /C \" dir c:\\\\\" "

which will be interpreted as the following that is not accepted by CMD.exe.(dir c:\ or dir c:\.\ is fine. There is no '\' directory or file on my Drive c).
However it is accepted by PS and MingGW.

"C:\\WINDOWS\\system32\\cmd.exe" /C " dir c:\\"

It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the command “dir c:\" for Windows Command Terminal.
However it is not an issue for PS or MingGW.

In D56230#1361634, @labath wrote:

In D56230#1358356, @zturner wrote:

I've always disliked this argument and hoped that someday someone would remove it entirely. My recollection (which may be wrong) is that the only actual use of it is so that if someone types a command, and we later need to print the command back, we will print it with the same quote char. It almost seems like we could just delete the argument and use a standardized quote char when flattening a command string.

+100

BTW, today I've tried to switch ProcessLauncherWindows to flattenWindowsCommandLine and this change alone was enough to fix TestQuoting, which has some tests XFAILed for windows due to quoting problems. I haven't sent out a patch yet because that has also broken platform shell dir c:\ for some reason, and I haven't gotten around to investigating that. If you (for any value of you) have some time, I'd encourage you to look into that. Otherwise, I'll get to that eventually.

• drop the if (m_entries[i].quote) branch. You don't need it here, and I don't believe it will be correct anyway, because all it will do is cause llvm::sys::flattenWindowsCommandLine to add one more quote level around that and escape the quotes added by yourself

In D56230#1355248, @labath wrote:

In D56230#1355247, @labath wrote:

For example, for a Args vector like lldb-server, gdb-remote, --log-channels=foo\\\ \\\""" ''', whatever, QuoteForCreateProcess would return
lldb-server gdb-remote "--log-channels=foo\\\ \\\\\\\"\"\" '''" whatever (there are other ways to quote this too). Passing this string to CreateProcess will result in the original vector being available to the main function of lldb-server, which is what this code (and all other code that works with the Args class) expects.

Btw, there is already code for doing this in llvm (llvm::sys::flattenWindowsCommandLine), so we can just steal the implementation from there.

In D56232#1349407, @labath wrote:

In D56232#1348514, @zturner wrote:

Is that even necessary? If a platform is not remote aware, IsHost() will always just return true by definition. So we could put all of this in the existing Platform base class.

I remember looking at this a while a go and concluding against it, but i'm not sure if it was impossible of just I didn't like the result.

The issue here is that PlatformWindows and PlatformPosix already have a m_remote_platform member (which normally is an instance of PlatformRemoteGDBServer). To move the common class into the base one, we'd need to move this member too. That would mean that any platform has a "remote" member, even those that already are "remote". That sounds a bit weird.

I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher.

In D56234#1352371, @labath wrote:

Looks very nice. Thanks.

Not quite sure but correct me if i am wrong.

With all the aaron's pending reviews on lldb-server, I could try the patch with the following platform apis.

In D53375#1269504, @asmith wrote:

Thanks for adding the cache. We noticed the slowdown also and were discussing the same thing. This LGTM if the other reviewers done have any comments.

In D52461#1249259, @aleksandr.urakov wrote:

In D52461#1245058, @clayborg wrote:

Try to use and extend CPlusPlusLanguage::MethodName as needed. I believe it was recently backed by a new clang parser that knows how to chop up C++ demangled names

It seems that CPlusPlusLanguage::MethodName is backed by LLDB CPlusPlusNameParser, which can't parse demangled names... Can you tell me, please, how is called a new Clang parser you have mentioned? May be I'll use it directly instead of PDBNameParser, or will back PDBNameParser by it (if the interface will be not very convenient)?