[Lldb-commits] [PATCH] D17107: [lldb] Unicode support on Win32

[Cameron via lldb-commits]

cameron314 added a comment.

I'll run clang-format and submit a new patch. Thanks for fixing that duplicate symbol with the signal stuff!


================
Comment at: source/Host/common/FileSpec.cpp:94-115
@@ -93,2 +93,24 @@
+    {
+#ifdef _WIN32
+        std::wstring wresolved_path;
+        if (!llvm::ConvertUTF8toWide(resolved_path, wresolved_path))
+            return false;
+        int stat_result;
+#ifdef _USE_32BIT_TIME_T
+        struct _stat32 file_stats;
+        stat_result = ::_wstat32(wresolved_path.c_str(), &file_stats);
+#else
+        struct _stat64i32 file_stats;
+        stat_result = ::_wstat64i32(wresolved_path.c_str(), &file_stats);
+#endif
+        if (stat_result == 0)
+        {
+            static_assert(sizeof(struct stat) == sizeof(file_stats), "stat and _stat32/_stat64i32 must have the same layout");
+            *stats_ptr = *reinterpret_cast<struct stat *>(&file_stats);
+        }
+        return stat_result == 0;
+#else
         return ::stat (resolved_path, stats_ptr) == 0;
+#endif
+    }
     return false;
----------------
zturner wrote:
> Here we are doing some stuff with `_USE_32BIT_TIME_T`, but we reproduce almost this exact same logic in `File::GetPermissions()` except without the special case for `_USE_32BIT_TIME_T`.  I think we should introduce a function called `FileSystem::Stat()` in very much the same way that you've done `FileSystem::Fopen()`.  And then we put the implementation there so that we can have it in one place and make sure it's always going to be consistent and correct.  
Yeah, I agree this should really go in `FileSystem`.

================
Comment at: tools/lldb-mi/MIUtilFileStd.cpp:82-95
@@ -79,9 +81,16 @@
 
 #if !defined(_MSC_VER)
     // Open with 'write' and 'binary' mode
     m_pFileHandle = ::fopen(vFileNamePath.c_str(), "wb");
 #else
     // Open a file with exclusive write and shared read permissions
-    m_pFileHandle = ::_fsopen(vFileNamePath.c_str(), "wb", _SH_DENYWR);
+    std::wstring path;
+    if (llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+        m_pFileHandle = ::_wfsopen(path.c_str(), L"wb", _SH_DENYWR);
+    else
+    {
+        errno = EINVAL;
+        m_pFileHandle = nullptr;
+    }
 #endif // !defined( _MSC_VER )
 
----------------
zturner wrote:
> Should we be using `FileSystem::Fopen()` here?
Since this is MSVC specific code already, I think it's OK. The `_SH_DENYWR` seems important here.

================
Comment at: tools/lldb-mi/MIUtilFileStd.cpp:234-241
@@ -224,3 +233,10 @@
     FILE *pTmp = nullptr;
+#ifdef _WIN32
+    std::wstring path;
+    if (!llvm::ConvertUTF8toWide(vFileNamePath.c_str(), path))
+        return false;
+    pTmp = ::_wfopen(path.c_str(), L"wb");
+#else
     pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+#endif
     if (pTmp != nullptr)
----------------
zturner wrote:
> Same here.  `FileSystem::Fopen()`
Absolutely. I missed this one.


http://reviews.llvm.org/D17107