cameron314 added inline comments.
Comment at: tools/lldb-mi/MIUtilFileStd.cpp:234-241
@@ -223,11 +233,10 @@
-FILE *pTmp = nullptr;
-pTmp = ::fopen(vFileNamePath.c_str(), "wb");
+FILE *pTmp = lldb_private::FileSystem::Fopen(vFileNamePath.c_str(), "wb");
if (pTmp
cameron314 closed this revision.
cameron314 added a comment.
Closing now that the patch has been committed by
http://reviews.llvm.org/rL264074.
(Not sure why it didn't auto-link.)
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-co
cameron314 added a comment.
Excellent, thank you!
Fortunately, almost all the other changes I've made to llvm/clang/lldb locally
are fairly small patches.
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.
I think this is ready to go in. You probably noticed that this took a long
time to work out all the issues with. In the future if there's any way you
could break things up into smaller pat
cameron314 updated this revision to Diff 51231.
cameron314 added a comment.
Here's the latest patch, now with 100% more autoformatting and a centralized
`FileSystem::Stat` function.
I didn't rebase, but it should still apply cleanly.
http://reviews.llvm.org/D17107
Files:
cmake/modules/LLDBCo
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
zturner added a comment.
And btw, the multiply defined symbol error is gone now that the custom `signal`
function is removed
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mail
zturner added a comment.
Patch doesn't appear to be clang-formatted. All patches need to be run through
clang-format before submitting.
I think this is my last set of comments. If there's any you disagree with let
me know. If you agree with everything, I can probably just make the changes on
cameron314 added a comment.
I think we're in different time zones -- I'm heading home for the day, but I'll
look back in the morning to see if there's anything else that needs addressing.
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
zturner added a comment.
I can look at it today. Sorry again for the delay, rebase 1 more time and
I'll check it out today
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailma
cameron314 updated this revision to Diff 50979.
http://reviews.llvm.org/D17107
Files:
cmake/modules/LLDBConfig.cmake
include/lldb/Host/FileSystem.h
include/lldb/Host/posix/HostInfoPosix.h
include/lldb/Host/windows/HostInfoWindows.h
packages/Python/lldbsuite/test/dotest.py
source/Comma
zturner added a comment.
I'm getting this when linking:
[826/826] Linking CXX executable bin\lldb.exe
FAILED: cmd.exe /C "cd . && "C:\Program Files (x86)\CMake\bin\cmake.exe" -E
vs_link_exe --intdir=tools\lldb\tools\driver\CMakeFiles\lldb.dir --manifests
-- C:\PROGRA~2\MICROS~3.0\VC\bin\AM
cameron314 added a comment.
Since it works without my patch, you're probably right about it being related
to the UNICODE define. All the other changes are completely removed from that
part of the code. But I still don't see how it could affect that.
Here's the script I use to run cmake:
setl
zturner added a comment.
I'm using the amd64_x86 toolchain. They're supposed to be identical so
that's unlikely to be the problem, but it's the only difference i can see.
Let me know what happens on your clean rebuild
http://reviews.llvm.org/D17107
I can look at it today. Sorry again for the delay, rebase 1 more time and
I'll check it out today
On Thu, Mar 17, 2016 at 11:34 AM Cameron wrote:
> cameron314 added a comment.
>
> @zturner: Let me know when you're ready for this patch and I'll rebase it
> again, since there's been quite a few m
I'm using the amd64_x86 toolchain. They're supposed to be identical so
that's unlikely to be the problem, but it's the only difference i can see.
Let me know what happens on your clean rebuild
On Fri, Mar 18, 2016 at 7:56 AM Cameron wrote:
> cameron314 added a comment.
>
> Since it works without
cameron314 added a comment.
Hmm, that doesn't seem good. Locally it links for me (I get compile time
warnings about the signal stuff, though).
It looks like the WinSDK signal.h is being pulled in despite `_INC_SIGNAL`
being defined (note that there's no include guard in that header -- it uses
`
zturner added a comment.
I'm not sure what your source tree layout looks like, but this isn't applying
for me. All your paths have "trunk" in front of them, which is a little
unusual in the directory structure is supposed to be something like this:
llvm
tools
lldb
source
A
zturner added a comment.
Oh yea I think the reason I didn't look at it again is because I was
waiting for the update where you removed the codepage stuff from the driver.
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@list
Oh yea I think the reason I didn't look at it again is because I was
waiting for the update where you removed the codepage stuff from the driver.
On Thu, Mar 17, 2016 at 11:39 AM Zachary Turner wrote:
> I can look at it today. Sorry again for the delay, rebase 1 more time and
> I'll check it ou
cameron314 added a comment.
Ah. Sorry. I have the same source layout as you locally, but I changed the
paths in the patch to match the layout of Diffusion
(http://reviews.llvm.org/diffusion/L/). I'll redo the patch with relative paths
from lldb...
http://reviews.llvm.org/D17107
___
cameron314 updated this revision to Diff 50976.
cameron314 added a comment.
Here we go! Rebased to tip (http://reviews.llvm.org/rL263735).
http://reviews.llvm.org/D17107
Files:
lldb/trunk/cmake/modules/LLDBConfig.cmake
lldb/trunk/include/lldb/Host/FileSystem.h
lldb/trunk/include/lldb/Host
cameron314 added a comment.
Aha, I get the same error now:
fatal error LNK1169: one or more multiply defined symbols found
I'm looking into it!
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists
zturner added a comment.
The reason I mentioned `UNICODE` is because it's one of the main differences
between pre-patch and post-patch. It works for me pre-patch. What command
line are you passing to CMake? Are you using MSVC 2015 Update 1 or initial
release?
The other difference is that I'
cameron314 added a comment.
@zturner: Let me know when you're ready for this patch and I'll rebase it
again, since there's been quite a few more commits.
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http:
cameron314 added a comment.
No worries. I removed the codepage stuff when I did the last rebase. Another
one coming up shortly!
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/m
cameron314 updated this revision to Diff 50449.
cameron314 added a comment.
... and had a slash too many at the start of the path prefixes. Sorry for the
spam.
http://reviews.llvm.org/D17107
Files:
lldb/trunk/cmake/modules/LLDBConfig.cmake
lldb/trunk/include/lldb/Host/FileSystem.h
lldb/t
cameron314 updated this revision to Diff 50448.
cameron314 added a comment.
Oops, was missing a slash in the dst-prefix of the previous patch update.
http://reviews.llvm.org/D17107
Files:
/lldb/trunk/cmake/modules/LLDBConfig.cmake
/lldb/trunk/include/lldb/Host/FileSystem.h
/lldb/trunk/inc
cameron314 updated this revision to Diff 50447.
cameron314 added a comment.
Here we go! http://reviews.llvm.org/D17549 has been committed, so I've rebased
this patch onto that latest version (r263233).
http://reviews.llvm.org/D17107
Files:
/lldb/trunkcmake/modules/LLDBConfig.cmake
/lldb/tr
zturner added a comment.
Sounds good. I will test this out once that patch goes in. And yea, I
would prefer to remove the codepage stuff for now, and we can re-consider
adding it back if/when someone actually needs it. Then we can discuss some
other options like a preprocessor define that enabl
Sounds good. I will test this out once that patch goes in. And yea, I
would prefer to remove the codepage stuff for now, and we can re-consider
adding it back if/when someone actually needs it. Then we can discuss some
other options like a preprocessor define that enables setting the codepage,
o
cameron314 added a comment.
Yes, if lldb.exe crashes or otherwise exits without returning to main, then the
codepage will stay set in the current console. The latter can be fixed with a
static object destructor, but not the former. I don't think this would be a
practical problem, since very few
zturner added a comment.
I'm waiting for the LLVM side change to go in and this patch rebased on top of
that before I test it out.
In the meantime, one thing I'm concerned about is setting the codepage of the
console. You determined earlier that it affects the state of the console even
after
cameron314 updated this revision to Diff 49986.
cameron314 added a comment.
Sorry for the delay, I wanted to make sure LLDB compiled without warnings after
I rebased the patch. It applies cleanly on the tip as of this writing, and is
based on http://reviews.llvm.org/rL262819.
http://reviews.ll
cameron314 added inline comments.
Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+}
+::_wsopen_s(&m_descriptor, wpath.c_str(), oflag, _SH_DENYWR, mode);
+#else
zturner wrote:
> cameron314 wrote:
> > zturner wrote:
> > > Any par
zturner added inline comments.
Comment at: lldb/trunk/source/Host/common/File.cpp:330
@@ +329,3 @@
+}
+::_wsopen_s(&m_descriptor, wpath.c_str(), oflag, _SH_DENYWR, mode);
+#else
cameron314 wrote:
> zturner wrote:
> > Any particular reason you're us
cameron314 added a comment.
Absolutely, I'm rebasing now. This is unfortunately the type of patch that rots
really quickly ;-)
I did run `check-lldb` locally at one point, with no major differences before
and after the patch. But I recently discovered the version of Python I'd
compiled had acc
zturner added a comment.
Can you rebase against tip of trunk? I want to apply and run the test suite
with the patch applied, but I'm getting a lot of errors. After uploading a new
rebased patch, can you also respond with the revision you rebased against so I
can make sure I'm at the same plac
cameron314 added a comment.
No worries, thanks for taking a look!
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
zturner added a subscriber: zturner.
zturner added a comment.
Sorry slipped under my radar, I'll look again today
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo
Sorry slipped under my radar, I'll look again today
On Mon, Mar 7, 2016 at 7:50 AM Cameron wrote:
> cameron314 added a comment.
>
> I don't want to be pushy, but is there a chance of this patch being
> accepted soon?
> It does depend on http://reviews.llvm.org/D17549 in LLVM which is also
> still
cameron314 added a comment.
I don't want to be pushy, but is there a chance of this patch being accepted
soon?
It does depend on http://reviews.llvm.org/D17549 in LLVM which is also still
pending, but that one's at least very a simple patch adding the support
functions that this one uses.
htt
cameron314 updated this revision to Diff 49185.
http://reviews.llvm.org/D17107
Files:
lldb/trunk/cmake/modules/LLDBConfig.cmake
lldb/trunk/include/lldb/Host/FileSystem.h
lldb/trunk/include/lldb/Host/posix/HostInfoPosix.h
lldb/trunk/include/lldb/Host/windows/HostInfoWindows.h
lldb/trunk/
cameron314 added inline comments.
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:242
@@ -221,1 +241,3 @@
+path.push_back(0);
+path.pop_back();
}
amccarth wrote:
> I recognize that you're just repeating the pattern from above, but ...
>
amccarth added a comment.
I'm growing more comfortable with these changes, but I'll defer to Zach.
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:242
@@ -221,1 +241,3 @@
+path.push_back(0);
+path.pop_back();
}
I recognize that you're
cameron314 added inline comments.
Comment at: lldb/trunk/tools/lldb-mi/MICmnLLDBDebugSessionInfo.cpp:618
@@ -616,3 +617,3 @@
-static char pBuffer[MAX_PATH];
+static char pBuffer[PATH_MAX];
const MIuint nBytes =
rFrame.GetLineEntry().GetFileSpec().GetPath(&pBuffer[0
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 49066.
cameron314 added a comment.
Here's a new version of the patch which takes into account most of the feedback
so far (less `#ifdefs`, etc.). It depends on my pending patch in LLVM
(http:
cameron314 added inline comments.
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 added inline comments.
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());
-
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:
> > cameron
amccarth added inline comments.
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());
-
cameron314 added inline comments.
Comment at: lldb/trunk/tools/driver/Driver.cpp:1289
@@ +1288,3 @@
+// Indicate that all our output is in UTF-8
+SetConsoleCP(CP_UTF8);
+#endif
zturner wrote:
> Is this going to affect the state of the console even after qu
zturner 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
cameron314 wrote:
> cameron314 wrote:
> > zturner
cameron314 added a comment.
I've addressed more feedback. I'm working on a second version of the patch with
a lot less `#ifdefs` and nicer wrappers (and better error handling where
possible).
Comment at: lldb/trunk/source/Commands/CommandCompletions.cpp:171
@@ -168,1 +170,3 @@
zturner added inline comments.
Comment at: lldb/trunk/source/Host/common/FileSpec.cpp:108
@@ +107,3 @@
+if (stat_result == 0)
+*stats_ptr = *reinterpret_cast(&file_stats);
+return stat_result == 0;
cameron314 wrote:
> zturner wrote:
> >
zturner added a comment.
Also make sure you run `ninja check-lldb` and `ninja check-lldb-unit` with this
patch to make sure everything works.
Comment at: lldb/trunk/source/Core/ConnectionSharedMemory.cpp:140
@@ +139,3 @@
+llvm::SmallVector wname;
+if (llvm::convertUTF8T
zturner added inline comments.
Comment at: lldb/trunk/source/Commands/CommandCompletions.cpp:171
@@ -168,1 +170,3 @@
{
+#ifdef _WIN32
+llvm::SmallVector wpartial_name_copy;
cameron314 wrote:
> zturner wrote:
> > I'm not fond of this approach o
cameron314 added a comment.
I've replied to most of your comments, thanks everyone. I'll make most of the
suggested changes, but some of them have nothing to do with correct Unicode
handling and are rather pre-existing design issues, and so are out of the scope
of this patch.
@zturner: I agree
zturner added a comment.
In http://reviews.llvm.org/D17107#349463, @cameron314 wrote:
> Thanks for all the feedback! I'll look into it in detail tomorrow.
>
> Perhaps it would make more sense if I clarified where I'm coming from with
> this patch -- we use LLDB on Windows (with a custom backend)
cameron314 added a comment.
Thanks for all the feedback! I'll look into it in detail tomorrow.
Perhaps it would make more sense if I clarified where I'm coming from with this
patch -- we use LLDB on Windows (with a custom backend) where I work, and
wanted to be able to run a program with a non-
jingham added a comment.
My bad, that sees to be what it does.
Jim
Repository:
rL LLVM
http://reviews.llvm.org/D17107
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
My bad, that sees to be what it does.
Jim
> On Feb 10, 2016, at 4:59 PM, Zachary Turner wrote:
>
> zturner added a comment.
>
> In http://reviews.llvm.org/D17107#349421, @jingham wrote:
>
>> Unless something has changed since last I looked, the llvm path utilities
>> are host specific. I di
zturner added a comment.
In http://reviews.llvm.org/D17107#349421, @jingham wrote:
> Unless something has changed since last I looked, the llvm path utilities are
> host specific. I didn't inspect the code where this is being used, but it is
> in Host/common, so unless there's an ifdef WINDOWS
Unless something has changed since last I looked, the llvm path utilities are
host specific. I didn't inspect the code where this is being used, but it is
in Host/common, so unless there's an ifdef WINDOWS around it, I don't think you
should use the llvm path utilities.
Jim
> On Feb 10, 2016,
jingham added a subscriber: jingham.
jingham added a comment.
Unless something has changed since last I looked, the llvm path utilities are
host specific. I didn't inspect the code where this is being used, but it is
in Host/common, so unless there's an ifdef WINDOWS around it, I don't think yo
zturner requested changes to this revision.
zturner added a comment.
This revision now requires changes to proceed.
Overall I like the idea and I'm happy to get Unicode paths and strings working
properly. I'll look at the rest in more detail later, but I have some starting
comments for now.
On
amccarth added a subscriber: amccarth.
amccarth added a comment.
I like that you're doing this, but ...
There's a ton of duplication here. I'd really like to see the conversions
centralized with consistent error handling. Cluttering up already-too-long
functions with more #ifdefs and a half d
cameron314 created this revision.
cameron314 added a reviewer: zturner.
cameron314 added a subscriber: lldb-commits.
cameron314 set the repository for this revision to rL LLVM.
Various fixes for Win32 interop, particularly with regard to paths; paths
containing non-ASCII characters now work prope
68 matches
Mail list logo
