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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
