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

[Adrian McCarthy]

Thanks for the feedback.  I'm still sorting through the potential data races Pavel caught, so I'll have a new diff up soon.


================
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:
> ```
> &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.

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



================
Comment at: source/Plugins/Process/Windows/ProcessWindows.cpp:813
@@ +812,3 @@
+
+        FileSpec executable_file(file_name, false);
+        ModuleSpec module_spec(executable_file);
----------------
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?

http://reviews.llvm.org/D10404

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