[Lldb-commits] [lldb] r223251 - Use timeout when reading debugserver's port from a named pipe.
Zachary Turner
zturner at google.com
Wed Dec 3 18:02:24 PST 2014
That's fine too, that way you won't have to change up all the existing call
sites of places using the current constructor.
As long as you think it won't take too long to fix, I'm fine leaving it as
is until the fix goes in. I'm probably the most frequent builder of
Windows, but I know there are at least a few others, so I don't want anyone
to be broken for too long.
On Wed Dec 03 2014 at 5:58:30 PM Oleksiy Vyalov <ovyalov at google.com> wrote:
> Or we may have 2 Pipe constructors - default creates anonymous pipe and
> constructor with parameter is for named pipe.
>
> On Wed, Dec 3, 2014 at 5:41 PM, Zachary Turner <zturner at google.com> wrote:
>
>> What about passing a value to Pipe's constructor that is the name of the
>> pipe? If it passes null, it's an anonymous pipe, if it passes non-null,
>> it's a named pipe. The same model should be fine for Windows too.
>>
>>
>> On Wed Dec 03 2014 at 5:40:19 PM Oleksiy Vyalov <ovyalov at google.com>
>> wrote:
>>
>>> Sorry for breakage.
>>>
>>> LLDB uses mkfifo to create a named pipe and for Windows there is just* if
>>> ( false ) *substitution - so, even with platform-independent read logic
>>> a named pipe creation should be abstracted as well.
>>> Can we fix this problem in 2 steps?:
>>> - Fix build with #ifdef _WIN32 #include <Winsock2.h> #else #include
>>> <sys/select.h> #endif (or even make ReadPortFromPipe available only for
>>> non-Windows configuration).
>>> - Redesign create/read pipe API to make it compatible with Windows
>>> since PipePosix in its current state represents anonymous pipe model
>>> only.
>>>
>>> On Wed, Dec 3, 2014 at 5:03 PM, Zachary Turner <zturner at google.com>
>>> wrote:
>>>
>>>> I took another look at the implementations. Pipe does not expose a
>>>> method that will do what we want. I think the correct fix is to add an
>>>> overload of Read() to PipePosix.h that specifies a timeout. In the
>>>> overloaded version, you call this select. I will implement the equivalent
>>>> overload on Windows so that everything will work.
>>>>
>>>> Thoughts?
>>>>
>>>> On Wed Dec 03 2014 at 4:55:17 PM Zachary Turner <zturner at google.com>
>>>> wrote:
>>>>
>>>>> This breaks the Windows build. For starters, Windows doesn't have
>>>>> sys/select.h. Secondly, although Windows doesn't currently use llgs, it
>>>>> will in the future. And select() on Windows does not work with Pipes.
>>>>>
>>>>> Can we revert this change? We have lldb/Host/Pipe.h that should be
>>>>> used for pipes instead of File. On Windows this is implemented in such a
>>>>> way that it's possible to get non-blocking behavior without select, and on
>>>>> posix I believe it uses select.
>>>>>
>>>>> On Wed Dec 03 2014 at 10:25:23 AM Oleksiy Vyalov <ovyalov at google.com>
>>>>> wrote:
>>>>>
>>>>>> Author: ovyalov
>>>>>> Date: Wed Dec 3 12:19:16 2014
>>>>>> New Revision: 223251
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=223251&view=rev
>>>>>> Log:
>>>>>> Use timeout when reading debugserver's port from a named pipe.
>>>>>>
>>>>>> http://reviews.llvm.org/D6490
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>> lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommun
>>>>>> ication.cpp
>>>>>>
>>>>>> Modified: lldb/trunk/source/Plugins/Process/gdb-remote/
>>>>>> GDBRemoteCommunication.cpp
>>>>>> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugin
>>>>>> s/Process/gdb-remote/GDBRemoteCommunication.cpp?rev=223251&r
>>>>>> 1=223250&r2=223251&view=diff
>>>>>> ============================================================
>>>>>> ==================
>>>>>> --- lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
>>>>>> (original)
>>>>>> +++ lldb/trunk/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
>>>>>> Wed Dec 3 12:19:16 2014
>>>>>> @@ -13,6 +13,7 @@
>>>>>> // C Includes
>>>>>> #include <limits.h>
>>>>>> #include <string.h>
>>>>>> +#include <sys/select.h>
>>>>>> #include <sys/stat.h>
>>>>>>
>>>>>> // C++ Includes
>>>>>> @@ -42,6 +43,50 @@
>>>>>> using namespace lldb;
>>>>>> using namespace lldb_private;
>>>>>>
>>>>>> +namespace
>>>>>> +{
>>>>>> +
>>>>>> +Error
>>>>>> +ReadPortFromPipe (const char *const named_pipe_path, uint16_t& port,
>>>>>> const int timeout_secs)
>>>>>> +{
>>>>>> + File name_pipe_file;
>>>>>> + auto error = name_pipe_file.Open (named_pipe_path,
>>>>>> File::eOpenOptionRead | File::eOpenOptionNonBlocking);
>>>>>> + if (error.Fail ())
>>>>>> + return error;
>>>>>> +
>>>>>> + struct timeval tv = {timeout_secs, 0};
>>>>>> + const auto pipe_handle = name_pipe_file.GetWaitableHandle ();
>>>>>> + fd_set rfds;
>>>>>> + FD_ZERO(&rfds);
>>>>>> + FD_SET(pipe_handle, &rfds);
>>>>>> +
>>>>>> + const auto retval = ::select (pipe_handle + 1, &rfds, NULL,
>>>>>> NULL, &tv);
>>>>>> + if (retval == -1)
>>>>>> + {
>>>>>> + error.SetErrorToErrno ();
>>>>>> + return error;
>>>>>> + }
>>>>>> + if (retval == 0)
>>>>>> + {
>>>>>> + error.SetErrorString ("timeout exceeded");
>>>>>> + return error;
>>>>>> + }
>>>>>> +
>>>>>> + char port_cstr[256];
>>>>>> + port_cstr[0] = '\0';
>>>>>> + size_t num_bytes = sizeof(port_cstr);
>>>>>> + error = name_pipe_file.Read (port_cstr, num_bytes);
>>>>>> +
>>>>>> + if (error.Success ())
>>>>>> + {
>>>>>> + assert (num_bytes > 0 && port_cstr[num_bytes-1] == '\0');
>>>>>> + port = Args::StringToUInt32 (port_cstr, 0);
>>>>>> + }
>>>>>> + return error;
>>>>>> +}
>>>>>> +
>>>>>> +}
>>>>>> +
>>>>>> GDBRemoteCommunication::History::History (uint32_t size) :
>>>>>> m_packets(),
>>>>>> m_curr_idx (0),
>>>>>> @@ -872,25 +917,23 @@ GDBRemoteCommunication::StartDebugserver
>>>>>> launch_info.AppendSuppressFileAction (STDIN_FILENO, true,
>>>>>> false);
>>>>>> launch_info.AppendSuppressFileAction (STDOUT_FILENO, false,
>>>>>> true);
>>>>>> launch_info.AppendSuppressFileAction (STDERR_FILENO, false,
>>>>>> true);
>>>>>> -
>>>>>> +
>>>>>> error = Host::LaunchProcess(launch_info);
>>>>>> -
>>>>>> +
>>>>>> if (error.Success() && launch_info.GetProcessID() !=
>>>>>> LLDB_INVALID_PROCESS_ID)
>>>>>> {
>>>>>> if (named_pipe_path[0])
>>>>>> {
>>>>>> - File name_pipe_file;
>>>>>> - error = name_pipe_file.Open(named_pipe_path,
>>>>>> File::eOpenOptionRead);
>>>>>> + error = ReadPortFromPipe(named_pipe_path, out_port,
>>>>>> 10);
>>>>>> if (error.Success())
>>>>>> {
>>>>>> - char port_cstr[256];
>>>>>> - port_cstr[0] = '\0';
>>>>>> - size_t num_bytes = sizeof(port_cstr);
>>>>>> - error = name_pipe_file.Read(port_cstr,
>>>>>> num_bytes);
>>>>>> - assert (error.Success());
>>>>>> - assert (num_bytes > 0 && port_cstr[num_bytes-1]
>>>>>> == '\0');
>>>>>> - out_port = Args::StringToUInt32(port_cstr, 0);
>>>>>> - name_pipe_file.Close();
>>>>>> + if (log)
>>>>>> + log->Printf("GDBRemoteCommunication::%s()
>>>>>> debugserver listens %u port", __FUNCTION__, out_port);
>>>>>> + }
>>>>>> + else
>>>>>> + {
>>>>>> + if (log)
>>>>>> + log->Printf("GDBRemoteCommunication::%s()
>>>>>> failed to read a port value from named pipe %s: %s", __FUNCTION__,
>>>>>> named_pipe_path, error.AsCString());
>>>>>> }
>>>>>> FileSystem::Unlink(named_pipe_path);
>>>>>> }
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> lldb-commits mailing list
>>>>>> lldb-commits at cs.uiuc.edu
>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>>>>>>
>>>>>
>>>
>>>
>>> --
>>> Oleksiy Vyalov | Software Engineer | ovyalov at google.com
>>>
>>
>
>
> --
> Oleksiy Vyalov | Software Engineer | ovyalov at google.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20141204/1f16a984/attachment.html>
More information about the lldb-commits
mailing list