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

[Chaoren Lin]

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

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

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

http://reviews.llvm.org/D10404

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