Windows: fix bug in getcwd() and add chdir().

[ted]

Summary

GetCurrentDirectory() returns the number of characters copied; 0 is a failure, not a success.

Add implementation for chdir().

[zturner]

source/Host/windows/Windows.cpp

168–177

I think a better fix would be to just delete this and change all the callsites to llvm::sys::fs::current_path(). The current implementation is currently already broken in the case of a path being longer than MAX_PATH (which is possible on Windows).

Admittedly, the fix here is certainly better than what we were doing before, but it would be better still to have a proper fix that re-uses the implementation in LLVM.

179–184

Can you also find all the places that call chdir() in LLDB, and remove the #ifdefs that disabled those codepaths on Windows? A quick search shows there's a few, so now that Windows has a chdir implementation I wouldn't want fixing the callsites to fall off of the radar.

181

You can probably just have this function return _chdir(). This guarantees that errno will be set appropriately.

[ted]

Changed getcwd() and chdir() to use _getcwd() and _chdir().

Removed #ifdef _WIN32 disabling calls to chdir() in Platform.cpp and GDBRemoteCommunicationServerPlatform.cpp.

[zturner]

source/Host/windows/Windows.cpp

167–171

This is fine for now, but it's worth pointing out this is still broken for paths longer than MAX_PATH. So if you ever run LLDB from a directory longer than 256 characters, things will get messed up. If you don't want to change this to use llvm::sys::fs::current_path(), can you add a TODO mentioning the problem?

[ted]

Replace erroneously removed comment

[ted]

llvm::sys::fs::current_path() doesn't handle paths > MAX_PATH-1:

std::error_code current_path(SmallVectorImpl<char> &result) {
  SmallVector<wchar_t, MAX_PATH> cur_path;
  DWORD len = MAX_PATH;

  do {
    cur_path.reserve(len);
    len = ::GetCurrentDirectoryW(cur_path.capacity(), cur_path.data());

    // A zero return value indicates a failure other than insufficient space.
    if (len == 0)
      return windows_error(::GetLastError());

    // If there's insufficient space, the len returned is larger than the len
    // given.
  } while (len > cur_path.capacity());

  // On success, GetCurrentDirectoryW returns the number of characters not
  // including the null-terminator.
  cur_path.set_size(len);
  return UTF16ToUTF8(cur_path.begin(), cur_path.size(), result);
}

The MSDN page for _getcwd() says that it supports paths > MAX_PATH if you pass NULL for the buffer. I'm not sure what happens if you pass a size > MAX_PATH.

What should getcwd() do if the path is > MAX_PATH? I could change it so if the first call returns an error, errno is ERANGE and max > MAX_PATH, call _getcwd() again with a NULL, then check that strlen(new buffer) < max, strcpy the new buffer into path and free it.

[ted]

Whoops - missed the do loop upping the size of the buffer to the return value. But I can't find any documentation saying that GetCurrentDirectory() supports paths > MAX_PATH. Are we sure it does?

[zturner]

In D9300#163393, @ted wrote:

Whoops - missed the do loop upping the size of the buffer to the return value. But I can't find any documentation saying that GetCurrentDirectory() supports paths > MAX_PATH. Are we sure it does?

The documentation for SetCurrentDirectory actually says it can't be longer than MAX_PATH. So maybe we can assume GetCurrentDirectory has the same issue. In any case, I guess it's not so important to change this. It still seems like a nice cleanup, but not very high priority, so feel free to commit as is.

include/lldb/Host/windows/win32.h

 
 #ifdef LLDB_DISABLE_PYTHON
 typedef uint32_t pid_t;
 #endif
 
 int usleep(uint32_t useconds);
 
 char* getcwd(char* path, int max);
+int chdir(const char* path);
 char* basename(char *path);
 char *dirname(char *path);
 
 int strcasecmp(const char* s1, const char* s2);
 int strncasecmp(const char* s1, const char* s2, size_t n);
 
 #define STDIN_FILENO  0
 #define STDOUT_FILENO 1

source/Host/windows/Windows.cpp

 #include <stdio.h>
 #include <stdarg.h>
 #include <string.h>
 #include <stdlib.h>
 #include <io.h>
 #include <cerrno>
 #include <ctype.h>
 
+// These prototypes are defined in <direct.h>, but it also defines chdir() and getcwd(), giving multiply defined errors
+extern "C"
+{
+    char *_getcwd(char *buffer, int maxlen);
+    int _chdir(const char *path);
+}
+
 int vasprintf(char **ret, const char *fmt, va_list ap)
 {
     char *buf;
     int len;
     size_t buflen;
     va_list ap2;
 
 #if defined(_MSC_VER) || defined(__MINGW64)

 {
     char* l1 = strrchr(path, '\\');
     char* l2 = strrchr(path, '/');
     if (l2 > l1) l1 = l2;
     if (!l1) return path; // no base name
     return &l1[1];
 }
 
+// use _getcwd() instead of GetCurrentDirectory() because it updates errno
 char* getcwd(char* path, int max)
 {
-    if (GetCurrentDirectory(max, path) == 0)
-        return path;
-    return NULL;
+    return _getcwd(path, max);
+}
+
+// use _chdir() instead of SetCurrentDirectory() because it updates errno
+int chdir(const char* path)
+{
+    return _chdir(path);
 }
 
 char *dirname(char *path)
 {
     char* l1 = strrchr(path, '\\');
     char* l2 = strrchr(path, '/');
     if (l2 > l1) l1 = l2;
     if (!l1) return NULL; // no dir name
     *l1 = 0;
     return path;
 }
 
 int strcasecmp(const char* s1, const char* s2)
 {
     return stricmp(s1, s2);
 }

source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp

 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerPlatform::Handle_QSetWorkingDir (StringExtractorGDBRemote &packet)
 {
     packet.SetFilePos (::strlen ("QSetWorkingDir:"));
     std::string path;
     packet.GetHexByteString (path);
 
-#ifdef _WIN32
-    // Not implemented on Windows
-    return SendUnimplementedResponse ("GDBRemoteCommunicationServerPlatform::Handle_QSetWorkingDir unimplemented");
-#else
     // If this packet is sent to a platform, then change the current working directory
     if (::chdir(path.c_str()) != 0)
         return SendErrorResponse (errno);
     return SendOKResponse ();
-#endif
 }
 
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerPlatform::Handle_qC (StringExtractorGDBRemote &packet)
 {
     // NOTE: lldb should now be using qProcessInfo for process IDs.  This path here
     // should not be used.  It is reporting process id instead of thread id.  The
     // correct answer doesn't seem to make much sense for lldb-platform.

source/Target/Platform.cpp

 bool
 Platform::SetWorkingDirectory (const ConstString &path)
 {
     if (IsHost())
     {
         Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM);
         if (log)
             log->Printf("Platform::SetWorkingDirectory('%s')", path.GetCString());
-#ifdef _WIN32
-        // Not implemented on Windows
-        return false;
-#else
         if (path)
         {
             if (chdir(path.GetCString()) == 0)
                 return true;
         }
         return false;
-#endif
     }
     else
     {
         m_working_dir.Clear();
         return SetRemoteWorkingDirectory(path);
     }
 }