[Lldb-commits] [lldb] r223251 - Use timeout when reading debugserver's port from a named pipe.
Oleksiy Vyalov
ovyalov at google.com
Wed Dec 3 18:10:01 PST 2014
Okay - I hope to have fix tomorrow, will ask you for review :).
On Wed, Dec 3, 2014 at 6:02 PM, Zachary Turner <zturner at google.com> wrote:
> 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
>>
>
--
Oleksiy Vyalov | Software Engineer | ovyalov at google.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20141203/c0aaeac8/attachment.html>
More information about the lldb-commits
mailing list