[Lldb-commits] [PATCH] Refactor Socket into a first class Host-object

[Zachary Turner]

This patch will be going in shortly.

All of the issues have been addressed, and I now have a fully passing test
suite on Linux.  Todd has already tested the patch on Mac and provided me
with an update to the Xcode project, which is included here as well.  If
this causes a build failure I will try to fix it up quickly, but I don't
expect any major test regressions.


On Wed, Jul 30, 2014 at 11:21 PM, Zachary Turner <zturner at google.com> wrote:

> I actually fixed a few instances of the casting in the most recent patch,
> thanks for catching a few stragglers.  I do intend to fix those because I
> don't like it either.
>
>
> On Wed, Jul 30, 2014 at 6:16 PM, Greg Clayton <gclayton at apple.com> wrote:
>
>> A few comments:
>> - Any lldb_private forward declarations should be added to
>> "include/lldb/lldb-forward.h" and then that file should be included instead
>> of inlining any forward declarations manually. Also if you are going to use
>> shared pointers to then there is a section in lldb-forward.h at the bottom
>> where you should declare them like the others.
>>
>>  namespace lldb_private {
>>
>> +class Error;
>> +class Socket;
>>
>>
>> - Can we rename IoObject to IOObject? Or is there a better name?
>> IOConnection? IOFile?
>>
>> - Add the "_sp" suffix to each of these and use IOObjectSP that you
>> define in lldb-forward.h:
>>
>> +    std::shared_ptr<IoObject> m_read;
>> +    std::shared_ptr<IoObject> m_write;
>>
>> - We have been using the "_ap" suffix for auto pointers, but we are
>> slowly transitioning to using a "_up" for the unique pointers.
>>
>> +    std::unique_ptr<Socket> m_listening_socket; // For listen/accept
>> connections, hold onto the
>>
>> These make me nervous and should probably return a pointer or a shared
>> pointer, not a reference unless they are private and are known to be used
>> only when these are valid:
>>
>> +    IoObject& GetReadObject() { return *m_read; }
>> +    const IoObject& GetReadObject() const { return *m_read; }
>>
>>
>> This kind of code seems like we are trying to make a new class IoObject,
>> but don't want to correctly abstract it down to a virtual class that can do
>> everything that we need of it. Seems like we should add a GetBoundPort(...)
>> to the IoObject class and have it return something invalid instead of
>> casting and having to know what the IoObject is and cast it:
>>
>> +            if (read_object.GetFdType() == IoObject::eFDTypeSocket)
>>              {
>> -                char port_cstr[32];
>> -                snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i",
>> out_port);
>> -                // Send the host and port down that debugserver and
>> specify an option
>> -                // so that it connects back to the port we are listening
>> to in this process
>> -                debugserver_args.AppendArgument("--reverse-connect");
>> -                debugserver_args.AppendArgument(port_cstr);
>> +                Socket& tcp_socket = static_cast<Socket&>(read_object);
>> +                assert(tcp_socket.GetSocketProtocol() ==
>> Socket::ProtocolTcp);
>> +                out_port = tcp_socket.GetBoundPort(10);
>>
>> Other than that, lots of code has moved around into the host layer but it
>> is mostly the same. So looks good.
>>
>> > On Jul 28, 2014, at 4:57 PM, Zachary Turner <zturner at google.com> wrote:
>> >
>> > This fixes the crashing in lldb-gdbserver, and should include all of
>> the necessary things to get it to compile on MacOSX as well.  (I have a
>> request for my own Mac workstation pending, until then I have to cross my
>> fingers and hope I did it right).
>> >
>> > lldb-gdbserver tests still fail, but now it's because of a pipe that's
>> not getting any data.  I suspect that all the remaining test failures now
>> have the same underlying cause.  Still investigating exactly what that
>> might be, though.
>> >
>> > http://reviews.llvm.org/D4641
>> >
>> > Files:
>> >  include/lldb/Core/ConnectionFileDescriptor.h
>> >  include/lldb/Host/Editline.h
>> >  include/lldb/Host/File.h
>> >  include/lldb/Host/IoObject.h
>> >  include/lldb/Host/Socket.h
>> >  lldb.xcodeproj/project.pbxproj
>> >  source/Core/ConnectionFileDescriptor.cpp
>> >  source/Host/common/CMakeLists.txt
>> >  source/Host/common/File.cpp
>> >  source/Host/common/IoObject.cpp
>> >  source/Host/common/Socket.cpp
>> >  source/Plugins/Process/MacOSX-Kernel/ProcessKDP.cpp
>> >  source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
>> >  tools/lldb-gdbserver/lldb-gdbserver.cpp
>> >  tools/lldb-platform/lldb-platform.cpp
>> > <D4641.11964.patch>_______________________________________________
>> > lldb-commits mailing list
>> > lldb-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140806/1898877e/attachment.html>