[Lldb-commits] [PATCH] Fix process attach and detach for Windows

[Adrian McCarthy]

Thanks all.  New patch coming momentarily.


================
Comment at: source/Plugins/Process/Windows/DebuggerThread.cpp:374
@@ -358,1 +373,3 @@
 
+    if (m_pid_to_detach != 0 && m_active_exception->GetExceptionCode() == EXCEPTION_BREAKPOINT) {
+        WINLOG_IFANY(WINDOWS_LOG_EVENT | WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_PROCESS,
----------------
labath wrote:
> amccarth wrote:
> > labath wrote:
> > > This is almost a nit, since it will generally work, but technically, this looks like a data race ( => UDB) on the `m_pid_to_detach` variable: You are accessing the variable here and in DebugDetach() and one of those accesses is write. Is there some synchronization here that I am not aware of which is preventing these accesses to occur simultaneously?
> > I see your point.  DebugDetach, which sets the pid, is called only from ProcessWindows under its lock, and then it triggers a breakpoint exception, so in the normal case it does work.  But if a breakpoint is hit as we're setting the pid, there is indeed a data race.  Nice catch.
> > 
> > I suppose I could make m_pid_to_detach a std::atomic.
> Yes, I believe an atomic variable would suffice in this case.
Done.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:71
@@ +70,3 @@
+        file_name.resize(file_name_size);
+        copied = ::GetModuleFileNameEx(process_handle, NULL, &file_name[0], file_name_size);
+    } while (copied >= file_name_size);
----------------
chaoren wrote:
> amccarth wrote:
> > chaoren wrote:
> > > ```
> > > &file_name[0]
> > > ```
> > > I think this is undefined behavior (might be different in recent C++ standards). Please use a `std::vector<char>` and `std::vector::data()`.
> > It's defined behavior in C++11, but I'll switch to a std::vector if you insist.
> > 
> > The data() member functions of std::vector and std::string return const pointers, so they're not useful for this purpose.
> [[ http://en.cppreference.com/w/cpp/container/vector/data | `std::vector::data` ]] can also be non-const.
Done.

Note, that std::vector::data is new with C++11, which is the same version of the spec that guarantees that std::string would work as I was using it.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:474
@@ +473,3 @@
+
+    SetPrivateState(eStateDetached);
+
----------------
labath wrote:
> amccarth wrote:
> > labath wrote:
> > > I don't know enough about ProcessWindows, but a thought comes to mind here:
> > > You set the process state to be eStateDetached, but that is not exactly true (yet). You have merely requested detachment, which is a process that can take a while (especially if the inferior is in the middle of processing some other breakpoint). It seems likely that this activity (processing a breakpoint hit) will race with whatever happens after DoDetach() returns. I would expect some kind of synchronization here that waits for a signal from the debugger thread that the detachment was successful.
> > > 
> > > As I said, I'm not an expert, but I was wondering if you have considered this scenario.
> > I'm pretty new to this as well.  No, I hadn't considered the scenario where we're already stopped at a breakpoint.  I was trying to get TestAttachResume to pass, which detaches while the inferior is still running.
> > 
> > I'll try to work through the implications of that and update the patch as needed.  Thanks!
> These things are tricky. Wait until you get around to TestConcurrentEvents. :)
> We also need a separate debug thread on linux, and getting these synchronization things right took a lot of time, and I'm still not convinced we have everything covered.
Ah, I see now that StopDebugging already had a stub for Detach.  I've reworked the solution to use that, which handles resuming from an existing breakpoint and waits for the debugger thread to finish (which avoids prematurely setting the process state to detached).

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:775
@@ -719,2 +774,3 @@
         return exe_module_sp->GetFileSpec().Exists();
-    return false;
+    // However, if there is no executable module, we return true since we might be preparing to attach.
+    return true;
----------------
chaoren wrote:
> amccarth wrote:
> > chaoren wrote:
> > > Is there a way to check if we're actually preparing to attach? There are probably other things we can check too (e.g., if //any// module exists).
> > I'm not exactly sure what you're asking.  It's not entirely clear to me what CanDebug is supposed to do.  This ProcessWindows implementation of it looks like it was cloned from ProcessLinux and ProcessPOSIX, which do no further checking.  Process::CanDebug doesn't have a comment explaining what it should do.
> > 
> > GetExecutableModule returns the first module if it can't find an actual executable module.  Thus, if it fails, we already know that there are no modules.
> > 
> > 
> Yeah, it does look like it's copied from ProcessPOSIX. Please ignore me then.
Acknowledged.

================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:813
@@ +812,3 @@
+
+        FileSpec executable_file(file_name, false);
+        ModuleSpec module_spec(executable_file);
----------------
chaoren wrote:
> amccarth wrote:
> > chaoren wrote:
> > > Shouldn't `resolve = true`?
> > file_name is already a fully-resolved, absolute path (because that's what GetProcessExecutableName returns).  Is there something more that FileSpec's resolve_path will do?
> It marks `FileSpec::m_is_resolved` to true, though I'm not sure if it's actually necessary for anything.
Done.  Setting it seems to have no negative effects.

http://reviews.llvm.org/D10404

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/