This revision was automatically updated to reflect the committed changes.
Closed by commit rL314127: Use socketpair on all Unix platforms (authored by
eugene).
Repository:
rL LLVM
https://reviews.llvm.org/D33213
Files:
lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
DemiMarie added a comment.
In https://reviews.llvm.org/D33213#873962, @jingham wrote:
> Pavel & Eugene haven't marked it accepted yet, but from the comments is looks
> like they are both okay with the change. So it looks to me like everything
> is done but checking the code in.
>
> If you
eugene accepted this revision.
eugene added a comment.
I did mark it Accepted.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
jingham added a comment.
Pavel & Eugene haven't marked it accepted yet, but from the comments is looks
like they are both okay with the change. So it looks to me like everything is
done but checking the code in.
If you have checkin privileges, then just check it in and as Greg says keep an
clayborg accepted this revision.
clayborg added a comment.
This should be good to go. Watch the buildbots for failures.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
DemiMarie added a comment.
Does this need any further action on my part?
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
DemiMarie updated this revision to Diff 110636.
DemiMarie added a comment.
Delete misleading comment
https://reviews.llvm.org/D33213
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
clayborg accepted this revision.
clayborg added a comment.
Remove the outdated comment and we are good to go.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
eugene accepted this revision.
eugene added a comment.
LGTM. Test run on Linux is clear. I also see a bit of perf bump.
Before checking in please remove outdated comment from
ProcessGDBRemote::LaunchAndConnectToDebugserver.
// Use a socketpair on Apple for now until other platforms can
DemiMarie updated this revision to Diff 108796.
DemiMarie added a comment.
Set FD_CLOEXEC on the communication fd
https://reviews.llvm.org/D33213
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
labath added a reviewer: eugene.
labath added a comment.
I am not able to test this right now. Eugene, can you take a look?
Comment at: tools/lldb-server/lldb-gdbserver.cpp:241
+ std::unique_ptr connection_up;
+ if (connection_fd != -1) {
+// Build the connection string.
clayborg accepted this revision.
clayborg added a comment.
I'm ok as long as Pavel is ok. Please wait for the OK from him as well.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
DemiMarie added a comment.
In https://reviews.llvm.org/D33213#820238, @clayborg wrote:
> So where did the other changes go where we use "--fd" for non Apple builds?
> Did those changes get lost? They will be needed.
>
> Are you able to run the test suite now?
I did. Not all tests passed, but
DemiMarie updated this revision to Diff 108214.
DemiMarie added a comment.
Actually use socketpair :)
https://reviews.llvm.org/D33213
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
clayborg added a comment.
So where did the other changes go where we use "--fd" for non Apple builds? Did
those changes get lost? They will be needed.
Are you able to run the test suite now?
https://reviews.llvm.org/D33213
___
lldb-commits
DemiMarie updated this revision to Diff 108094.
DemiMarie added a comment.
Initalize connection_fd
Otherwise undefined behavior ensues whenever --fd is not passed to
lldb-server -g.
https://reviews.llvm.org/D33213
Files:
tools/lldb-server/lldb-gdbserver.cpp
Index:
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Please init fd
Comment at: tools/lldb-server/lldb-gdbserver.cpp:388
bool reverse_connect = false;
+ int connection_fd;
This must be
DemiMarie updated this revision to Diff 107805.
DemiMarie added a comment.
Fix connection when there is no host:port
https://reviews.llvm.org/D33213
Files:
source/Host/posix/ConnectionFileDescriptorPosix.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
clayborg added a comment.
I am not sure how you expect to submit any patches then. Patches must be tested
prior to submission.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
labath added a comment.
In https://reviews.llvm.org/D33213#814041, @DemiMarie wrote:
> In https://reviews.llvm.org/D33213#813037, @clayborg wrote:
>
> > Before any changes are submitted, we assume you are getting a clean test
> > suite run.
>
>
> On my system, I get build failures in clang.
I
DemiMarie added a comment.
In https://reviews.llvm.org/D33213#813037, @clayborg wrote:
> Before any changes are submitted, we assume you are getting a clean test
> suite run.
On my system, I get build failures in clang.
https://reviews.llvm.org/D33213
clayborg requested changes to this revision.
clayborg added a comment.
Before any changes are submitted, we assume you are getting a clean test suite
run.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
clayborg added a comment.
Any news on any speed changes? Please post the output of:
(lldb) process plugin packet speed-test
Both without and with this change. Nice to track any perf improvements
https://reviews.llvm.org/D33213
___
lldb-commits
clayborg accepted this revision.
clayborg added a comment.
Let Pavel try this and verify and this is good to go.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
DemiMarie added a comment.
In https://reviews.llvm.org/D33213#806807, @clayborg wrote:
> Ah yes, I thought there was something missing...
I just made `--fd` an alias for `--pipe`.
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
DemiMarie updated this revision to Diff 106708.
DemiMarie added a comment.
Make --fd an alias for --pipe
https://reviews.llvm.org/D33213
Files:
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
tools/lldb-server/lldb-gdbserver.cpp
Index: tools/lldb-server/lldb-gdbserver.cpp
clayborg added a comment.
Ah yes, I thought there was something missing...
Repository:
rL LLVM
https://reviews.llvm.org/D33213
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
Have you tried that this actually works? (It doesn't work for me)
As far as I can tell, you will have to teach lldb-server to understand the
`--fd` argument before you can flip this
clayborg added a comment.
Looks fine to me, lets make sure Pavel is OK with this. On MacOS socketpair is
also much faster. Please run the following command while stopped at a
breakpoint with and without your change:
(lldb) process plugin packet speed-test
It will send and receive packets
29 matches
Mail list logo
