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

[Cameron via lldb-commits]

cameron314 added inline comments.

================
Comment at: lldb/trunk/source/Core/Disassembler.cpp:881
@@ -878,3 +880,3 @@
     }
-        
-    FILE *test_file = fopen (file_name, "r");
+    FILE *test_file = nullptr;
+#if _WIN32
----------------
zturner wrote:
> cameron314 wrote:
> > cameron314 wrote:
> > > zturner wrote:
> > > > Would LLDB's `File` class work here instead of using raw APIs?  I see you fixed the code in `lldb_private::File`, so perhaps simply using that class here instead of a `FILE*` would allow you to reuse the fix from that class.
> > > No idea. Probably :-) I'll look into it, and make this change if it's trivial.
> > I don't trust myself to make this change without introducing a bug. But I'll get rid of the `#ifdef`.
> One possibility is to revert this portion of the patch (along with any other changes that you don't feel comfortable making) back to their original state.  This will still leave certain parts of unicode functionality broken, but it would reduce the surface area of "things that might be wrong with the patch" and give people a chance to make sure nothing else is seriously broken.  Then, we can do these other "more difficult" locations as a followup patch.  The nice thing about that is that if it does end up breaking something, and we have to revert it, it doesn't revert 100% of the work towards getting Unicode working, but only a couple of points.
This makes sense. I'm almost done with the next version of the patch, we'll see what should make the cut at that point.

================
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:1179
@@ +1178,3 @@
+
+            char child_path[PATH_MAX];
+            const int child_path_len = ::snprintf (child_path, sizeof(child_path), "%s\\%s", dir_path, fileName.c_str());
----------------
amccarth wrote:
> cameron314 wrote:
> > amccarth wrote:
> > > MAX_PATH in Windows.  (And someday, we should get rid of these fixed buffers so we can use UNC paths.)
> > I absolutely agree re the fixed buffers. I'll try to make this change in all of the functions that can allocate dynamic memory.
> > 
> > `MAX_PATH` is not `PATH_MAX`, though -- `MAX_PATH` is a Win32 constant of 260, whereas `PATH_MAX` is an LLDB constant of 32K. (Actually PATH_MAX is defined in multiple places and was either that or MAX_PATH depending on which header was included, but mostly it was 32K so I changed them all to at least be consistently 32K.)
> Ah, I see.  I work in another code base where PATH_MAX is synonymous with MAX_PATH, so I was confused.
> 
> Buffers of 32K characters on the stack seem like a bad idea.  We need a vector or string or some other container that puts the bulk of the data somewhere other than the stack.
Right. There's currently ~110 places in LLDB that allocate buffers of size `PATH_MAX` on the stack. Nearly all of those predate my patch, it seems.

================
Comment at: lldb/trunk/source/Host/windows/Windows.cpp:210
@@ +209,3 @@
+    wchar_t wpath[PATH_MAX];
+    if (wchar_t* wresult = _wgetcwd(wpath, PATH_MAX))
+    {
----------------
zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Why can't we just convert to the result of `_wgetcwd` directly to UTF8?
> > Not sure I understand. That's what this code does? There's a tad of extra logic needed because the `path` passed in is allowed to be `NULL`, indicating we should allocate a buffer for the caller.
> Ahh that extra logic is what I was referring to.  I wasn't aware of those semantics of `getcwd`.  Can you add a comment explaining that, since it's not obvious from looking at the code.
Haha, sure. I wasn't aware either, but it blew up at runtime since it turns out LLDB actually makes use of this when booting with a relative path.

================
Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1175
@@ -1166,1 +1174,3 @@
+    fp = _wfopen((const wchar_t *)wpath.data(), (const wchar_t *)wmode.data());
+#else
     fp = fopen(path, mode);
----------------
zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Here's another example of an #ifdef in common code that we will need to do something about.  Is there a constructor of the `File` class that takes a path and a mode?
> > There is, but the mode is an enum and not a string. I'll get rid of the `#ifdef`, though.
> We have a function somewhere that converts a mode string to the enum.  I think it's in `File.h` or `File.cpp`.  Let me know if you can't find it.
You know, it turns out there's one right in this very file. Thanks!


Repository:
  rL LLVM

http://reviews.llvm.org/D17107