I'll have a look, sorry about that.
On Wed, Oct 9, 2019 at 4:37 PM Shafik Yaghmour via Phabricator <
revi...@reviews.llvm.org> wrote:
> shafik added a comment.
>
> This change broek the`TestDataFormatterInvalidStdUniquePtr.py` test, see:
> http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2
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
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-
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
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 @@
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
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
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());
---
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/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 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 ...
>
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 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 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
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
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
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 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
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
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 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 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 added a comment.
Cool! I got pulled into something else at work and didn't have time to
investigate the linker error that this led to, sorry. But the patch looks good
to me (not that I know LLDB very well).
Comment at: tools/driver/Driver.cpp:1314
@@ -1313,1 +1313,3
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.
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
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.
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 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
`
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
cameron314 added inline comments.
Comment at: tools/driver/Driver.cpp:1314
@@ -1313,1 +1313,3 @@
+signal(SIGINT, sigint_handler);
+#ifndef _MSC_VER
signal (SIGPIPE, SIG_IGN);
zturner wrote:
> I think `_MSC_VER` is the right check, because the builtin `sig
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
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
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
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.
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
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 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 created this revision.
cameron314 added reviewers: clayborg, zturner, jingham.
cameron314 added subscribers: mamai, lldb-commits.
cameron314 set the repository for this revision to rL LLVM.
When stopping the private state thread, there was a race condition between the
time the thread e
cameron314 created this revision.
cameron314 added a reviewer: spyffe.
cameron314 added subscribers: mamai, lldb-commits.
cameron314 set the repository for this revision to rL LLVM.
This allows expressions such as 'i == 1 || i == 2` to be executed using the IR
interpreter.
Repository:
rL LLVM
cameron314 added a comment.
Ah, that's a bit tricky at the moment. The LLVM tip no longer compiles with
VS2015 (specifically lib\Support\SourceMgr.cpp), and my Python setup for VS2013
is all wonky.
This will have to wait a bit.
Repository:
rL LLVM
http://reviews.llvm.org/D19124
_
cameron314 added a comment.
Whoops, my fault. I was accidentally using the VS2013 command prompt with the
2015 cmake files. Compiling happily as we speak, test coming up soon after.
Repository:
rL LLVM
http://reviews.llvm.org/D19124
___
lldb-com
cameron314 added inline comments.
Comment at: source/Target/Process.cpp:4116
@@ -4120,1 +4115,3 @@
+// Signal the private state thread
+if (m_private_state_thread.IsJoinable())
{
clayborg wrote:
> If you are going to do IsJoinable(), Cancel(), and Joi
cameron314 added a comment.
Note also that using a copy of the instance variable is no different from using
the instance variable directly, here, since as you say they both have a shared
pointer to the same underlying object which is manipulated through either of
the two to the same effect.
R
cameron314 added a comment.
Hmm, interesting. Yes, it should not be timing out in the first place. That's
likely a bug on our side, still to be determined. I'm looking into it.
Regardless, it led us to find and fix this race :-)
I think it still makes sense for LLDB to do a `Cancel()` as a last
cameron314 added a comment.
I read the same docs :D This is the important part:
> If multiple threads of execution access the same shared_ptr without
> synchronization and any of those accesses uses a non-const member function of
> shared_ptr then a data race will occur; the shared_ptr overload
cameron314 added a comment.
OK, I'll pare down the patch to just that line then.
I found the reason for the time out. At the end of our debug session, we
destroy the Target, which destroys the process. Process::Destroy in turn calls
Process::Detach, which calls DoDetach just before calling
Sto
cameron314 added a comment.
Ooh, that might work. But when ControlProvateStateThread resets
m_private_state_control_wait to false there's still a race between that and the
thread exiting. It could then be set back to false even after the thread has
exited (this is even likely for a detach).
I
cameron314 updated the summary for this revision.
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 54095.
cameron314 added a comment.
I've updated the diff to include a fix for the race between Detach and Stop.
This has been working nicely f
cameron314 updated this revision to Diff 54098.
cameron314 added a comment.
Oops, uploaded wrong diff a minute ago. Here's the one with the full fix.
http://reviews.llvm.org/D19122
Files:
include/lldb/Target/Process.h
source/Target/Process.cpp
Index: source/Target/Process.cpp
=
cameron314 removed rL LLVM as the repository for this revision.
cameron314 updated this revision to Diff 54134.
cameron314 added a comment.
I've added a test for this patch.
http://reviews.llvm.org/D19124
Files:
packages/Python/lldbsuite/test/expression_command/ir-interpreter/Makefile
pack
cameron314 added a comment.
Hmm, that's not good.
No, I don't have a Linux machine set up. I can set up a VM but if you already
have the dumps/logs it would be faster to start with those.
Thanks!
Repository:
rL LLVM
http://reviews.llvm.org/D19122
_
cameron314 added a comment.
I'd appreciate if someone could take a look at this when they have a moment.
http://reviews.llvm.org/D19124
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-com
cameron314 added a comment.
All right, no worries :-) I didn't want to bother anyone.
http://reviews.llvm.org/D19124
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
cameron314 added a comment.
@spyffe, do you have time to look at this sometime this week? It's a very
simple patch (the test code took longer to write than the patch itself). Thanks!
http://reviews.llvm.org/D19124
___
lldb-commits mailing list
lldb
cameron314 added a comment.
Excellent, thank you!
http://reviews.llvm.org/D19124
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Sorry to break the build! Apparently 'make clean' isn't executing cleanly
during the build step of the test, but I haven't the faintest idea why. It
builds/runs fine locally for me (then again, I'm on Windows). The makefile
is dead simple, and is identical to that of some other tests. Has anyone
se
Thanks Jim, I'll definitely use that as a template next time!
Ah, I think I found the problem with the test. The makefile was in the
patch but wasn't committed. Trying that out now.
On Thu, May 12, 2016 at 5:45 PM, Jim Ingham wrote:
>
> > On May 12, 2016, at 2:25 PM, Cameron wrote:
> >
> > Sor
I added the makefile with r269366. The tests seem to be working now:
http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/14641
Sorry about that!
On Thu, May 12, 2016 at 6:07 PM, Cameron wrote:
> Thanks Jim, I'll definitely use that as a template next time!
>
> Ah, I think I f
This revision was automatically updated to reflect the committed changes.
Closed by commit rL271209: [LLDB] Make sure that indexing is done before
clearing DIE info (authored by cameron314).
Changed prior to commit:
http://reviews.llvm.org/D20738?vs=58661&id=58965#toc
Repository:
rL LLVM
ht
cameron314 created this revision.
cameron314 added reviewers: clayborg, labath, zturner.
cameron314 added a subscriber: lldb-commits.
This is the follow-up to D19122, which was accepted but subsequently reverted
due to a bug it introduced (that I didn't see during local testing on Windows
but wh
cameron314 added a comment.
@clayborg: Thanks for having a look! I've added Jim Ingham as a reviewer.
@jingham, I'd appreciate if you could take a few minutes to look this over.
Right, I'd seen the backup/restore of the thread. As far as I can tell it
should still work; the code in `ControlPriv
cameron314 added a comment.
Thanks everyone :-)
Ah, yeah, sorry if I gave the wrong impression, but that comment is not
specific to Linux (in fact, I've only seen it once, on Windows). At one point
the debugger had entered ControlPrivateStateThread on one thread to stop it,
seen that the threa
This revision was automatically updated to reflect the committed changes.
Closed by commit rL272682: [lldb] Fixed race conditions on private state thread
exit (authored by cameron314).
Changed prior to commit:
http://reviews.llvm.org/D21296?vs=60536&id=60693#toc
Repository:
rL LLVM
http://r
cameron314 created this revision.
cameron314 added reviewers: spyffe, zturner, clayborg.
cameron314 added a subscriber: lldb-commits.
The `EntityVariable` materializer was, under certain conditions, taking the
bytes of a `DataExtractor` that were potentially in host order (e.g. little
endian) an
cameron314 added a comment.
@spyffe, do you have a minute to look this over?
http://reviews.llvm.org/D21328
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
cameron314 added a comment.
Sorry to bug you again, but @spyffe, could you have a look when you have a sec?
http://reviews.llvm.org/D21328
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-
cameron314 added a comment.
@spyffe, do you have a minute today?
https://reviews.llvm.org/D21328
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
cameron314 added a comment.
Hmm, I don't think this is easily testable at all. We happened to see it on our
(out-of-tree) big-endian architecture when debugging remotely on Windows, but
only for variables backed by registers. I was thus able to trace the problem
back to this code, fix it, and v
68 matches
Mail list logo
