Bug 23158

Summary: gdbserver no longer functional on Windows
Product: gdb Reporter: Joel Brobecker <brobecker>
Component: serverAssignee: Joel Brobecker <brobecker>
Status: RESOLVED FIXED    
Severity: critical    
Priority: P2    
Version: 8.1   
Target Milestone: 8.1.1   
Host: Target:
Build: Last reconfirmed:

Description Joel Brobecker 2018-05-10 15:20:46 UTC
Trying to start GDBserver on Windows currently yields the following
error...

    $ gdbserver.exe --once :4444 simple_main.exe
    glob could not process pattern '(null)'.
    Exiting

... after which GDB terminates with a nonzero status.

But fixing that shows that we have more issues:

    $ gdbserver.exe --once :4444 simple_main.exe
    Killing process(es): 5008
    No program to debug
    Exiting

And once that one gets fixed, we now have to fix a third issue:

    $ gdbserver.exe --once :4444 simple_main.exe
    [SEGV]
Comment 1 Joel Brobecker 2018-05-10 15:21:25 UTC
Setting the target milestone to 8.1.1 as this one was identified as critical.
Comment 2 cvs-commit@gcc.gnu.org 2018-05-10 15:28:46 UTC
The master branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=906994d9d50eb40dc89a5bf0830e15ed10938641

commit 906994d9d50eb40dc89a5bf0830e15ed10938641
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:23:10 2018 -0500

    [gdbserver/win32] fatal "glob could not process pattern '(null)'" error
    
    Trying to start GDBserver on Windows currently yields the following
    error...
    
        $ gdbserver.exe --once :4444 simple_main.exe
        glob could not process pattern '(null)'.
        Exiting
    
    ... after which GDB terminates with a nonzero status.
    
    This is because create_process in win32-low.c calls gdb_tilde_expand
    with the result of a call to get_inferior_cwd without verifying that
    the returned directory is not NULL:
    
        | static BOOL
        | create_process (const char *program, char *args,
        |                 DWORD flags, PROCESS_INFORMATION *pi)
        | {
        |   const char *inferior_cwd = get_inferior_cwd ();
        |   std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);
    
    This patch avoids this by only calling gdb_tilde_expand when
    INFERIOR_CWD is not NULL, which is similar to what is done on
    GNU/Linux for instance.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * win32-low.c (create_process): Only call gdb_tilde_expand if
            inferior_cwd is not NULL.
Comment 3 cvs-commit@gcc.gnu.org 2018-05-10 15:28:51 UTC
The master branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7dbac825b09f0847e608b50c80db816ef20d9315

commit 7dbac825b09f0847e608b50c80db816ef20d9315
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:24:33 2018 -0500

    gdbserver/Windows: Fix "no program to debug" error
    
    Trying to start a program with GDBserver on Windows yields
    the following error:
    
        $ gdbserver.exe --once :4444 simple_main.exe
        Killing process(es): 5008
        No program to debug
        Exiting
    
    The error itself comes from the following code shortly after
    create_inferior gets called (in server.c::main):
    
        /* Wait till we are at first instruction in program.  */
        create_inferior (program_path.get (), program_args);
        [...]
    
        if (last_status.kind == TARGET_WAITKIND_EXITED
            || last_status.kind == TARGET_WAITKIND_SIGNALLED)
          was_running = 0;
        else
          was_running = 1;
    
        if (!was_running && !multi_mode)
          error ("No program to debug");
    
    What happens is that the "last_status" global starts initialized
    as zeroes, which means last_status.kind == TARGET_WAITKIND_EXITED,
    and we expect create_inferior to be waiting for the inferior to
    start until reaching the SIGTRAP, and to set the "last_status"
    global to match that last event we received.
    
    I suspect this is an unintended side-effect of the following change...
    
        commit 2090129c36c7e582943b7d300968d19b46160d84
        Date:   Thu Dec 22 21:11:11 2016 -0500
        Subject: Share fork_inferior et al with gdbserver
    
    ... which removes some code in server.c that was responsible for
    starting the inferior in a functin that was named start_inferior,
    and looked like this:
    
       signal_pid = create_inferior (new_argv[0], &new_argv[0]);
       [...]
       /* Wait till we are at 1st instruction in program, return new pid
          (assuming success).  */
       last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
    
    The code has been transitioned to using fork_inferior, but sadly,
    only for the targets that support it. On Windows, the calls to wait
    setting "last_status" simply disappeared.
    
    This patch adds it back in the Windows-specific implementation of
    create_inferior.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * win32-low.c (win32_create_inferior): Add call to my_wait
            setting last_status global.
Comment 4 cvs-commit@gcc.gnu.org 2018-05-10 15:28:56 UTC
The master branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=190852c8ac75cb62a737c58edfadfb0e1fcef78a

commit 190852c8ac75cb62a737c58edfadfb0e1fcef78a
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:27:13 2018 -0500

    gdbserver/Windows: crash during connection establishment phase
    
    On Windows, starting a new process with GDBserver seems to work,
    in the sense that the program does get started, and GDBserver
    confirms that it is listening for GDB to connect. However, as soon as
    GDB establishes the connection with GDBserver, and starts discussing
    with it, GDBserver crashes, with a SEGV.
    
    This SEGV occurs in remote-utils.c::prepare_resume_reply...
    
      | regp = current_target_desc ()->expedite_regs;
      | [...]
      | while (*regp)
    
    ... because, in our case, REGP is NULL.
    
    This patches fixes the issues by adding a parameter to init_target_desc,
    in order to make sure that we always provide the list of registers when
    we initialize a target description.
    
    gdb/ChangeLog:
    
            PR server/23158:
            * regformats/regdat.sh: Adjust script, following the addition
            of the new expedite_regs parameter to init_target_desc.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * tdesc.h (init_target_desc) <expedite_regs>: New parameter.
            * tdesc.c (init_target_desc) <expedite_regs>: New parameter.
            Use it to set the expedite_regs field in the given tdesc.
            * x86-tdesc.h: New file.
            * linux-aarch64-tdesc.c (aarch64_linux_read_description):
            Adjust following the addition of the new expedite_regs parameter
            to init_target_desc.
            * linux-tic6x-low.c (tic6x_read_description): Likewise.
            * linux-x86-tdesc.c: #include "x86-tdesc.h".
            (i386_linux_read_description, amd64_linux_read_description):
            Adjust following the addition of the new expedite_regs parameter
            to init_target_desc.
            * lynx-i386-low.c: #include "x86-tdesc.h".
            (lynx_i386_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
            * nto-x86-low.c: #include "x86-tdesc.h".
            (nto_x86_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
            * win32-i386-low.c: #include "x86-tdesc.h".
            (i386_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
Comment 5 cvs-commit@gcc.gnu.org 2018-05-10 16:46:27 UTC
The gdb-8.1-branch branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e378993dd341b10571a7cb6e98d5d3b48a06e24c

commit e378993dd341b10571a7cb6e98d5d3b48a06e24c
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:23:10 2018 -0500

    [gdbserver/win32] fatal "glob could not process pattern '(null)'" error
    
    Trying to start GDBserver on Windows currently yields the following
    error...
    
        $ gdbserver.exe --once :4444 simple_main.exe
        glob could not process pattern '(null)'.
        Exiting
    
    ... after which GDB terminates with a nonzero status.
    
    This is because create_process in win32-low.c calls gdb_tilde_expand
    with the result of a call to get_inferior_cwd without verifying that
    the returned directory is not NULL:
    
        | static BOOL
        | create_process (const char *program, char *args,
        |                 DWORD flags, PROCESS_INFORMATION *pi)
        | {
        |   const char *inferior_cwd = get_inferior_cwd ();
        |   std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);
    
    This patch avoids this by only calling gdb_tilde_expand when
    INFERIOR_CWD is not NULL, which is similar to what is done on
    GNU/Linux for instance.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * win32-low.c (create_process): Only call gdb_tilde_expand if
            inferior_cwd is not NULL.
Comment 6 cvs-commit@gcc.gnu.org 2018-05-10 16:46:32 UTC
The gdb-8.1-branch branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=c0c0270a9a38e1e888e9d6f0185b465d3751f518

commit c0c0270a9a38e1e888e9d6f0185b465d3751f518
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:24:33 2018 -0500

    gdbserver/Windows: Fix "no program to debug" error
    
    Trying to start a program with GDBserver on Windows yields
    the following error:
    
        $ gdbserver.exe --once :4444 simple_main.exe
        Killing process(es): 5008
        No program to debug
        Exiting
    
    The error itself comes from the following code shortly after
    create_inferior gets called (in server.c::main):
    
        /* Wait till we are at first instruction in program.  */
        create_inferior (program_path.get (), program_args);
        [...]
    
        if (last_status.kind == TARGET_WAITKIND_EXITED
            || last_status.kind == TARGET_WAITKIND_SIGNALLED)
          was_running = 0;
        else
          was_running = 1;
    
        if (!was_running && !multi_mode)
          error ("No program to debug");
    
    What happens is that the "last_status" global starts initialized
    as zeroes, which means last_status.kind == TARGET_WAITKIND_EXITED,
    and we expect create_inferior to be waiting for the inferior to
    start until reaching the SIGTRAP, and to set the "last_status"
    global to match that last event we received.
    
    I suspect this is an unintended side-effect of the following change...
    
        commit 2090129c36c7e582943b7d300968d19b46160d84
        Date:   Thu Dec 22 21:11:11 2016 -0500
        Subject: Share fork_inferior et al with gdbserver
    
    ... which removes some code in server.c that was responsible for
    starting the inferior in a functin that was named start_inferior,
    and looked like this:
    
       signal_pid = create_inferior (new_argv[0], &new_argv[0]);
       [...]
       /* Wait till we are at 1st instruction in program, return new pid
          (assuming success).  */
       last_ptid = mywait (pid_to_ptid (signal_pid), &last_status, 0, 0);
    
    The code has been transitioned to using fork_inferior, but sadly,
    only for the targets that support it. On Windows, the calls to wait
    setting "last_status" simply disappeared.
    
    This patch adds it back in the Windows-specific implementation of
    create_inferior.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * win32-low.c (win32_create_inferior): Add call to my_wait
            setting last_status global.
Comment 7 cvs-commit@gcc.gnu.org 2018-05-10 16:46:38 UTC
The gdb-8.1-branch branch has been updated by Joel Brobecker <brobecke@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=572797ce5f4dc697120cfd07e95553118b4f105e

commit 572797ce5f4dc697120cfd07e95553118b4f105e
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Thu May 10 10:27:13 2018 -0500

    gdbserver/Windows: crash during connection establishment phase
    
    On Windows, starting a new process with GDBserver seems to work,
    in the sense that the program does get started, and GDBserver
    confirms that it is listening for GDB to connect. However, as soon as
    GDB establishes the connection with GDBserver, and starts discussing
    with it, GDBserver crashes, with a SEGV.
    
    This SEGV occurs in remote-utils.c::prepare_resume_reply...
    
      | regp = current_target_desc ()->expedite_regs;
      | [...]
      | while (*regp)
    
    ... because, in our case, REGP is NULL.
    
    This patches fixes the issues by adding a parameter to init_target_desc,
    in order to make sure that we always provide the list of registers when
    we initialize a target description.
    
    gdb/ChangeLog:
    
            PR server/23158:
            * regformats/regdat.sh: Adjust script, following the addition
            of the new expedite_regs parameter to init_target_desc.
    
    gdb/gdbserver/ChangeLog:
    
            PR server/23158:
            * tdesc.h (init_target_desc) <expedite_regs>: New parameter.
            * tdesc.c (init_target_desc) <expedite_regs>: New parameter.
            Use it to set the expedite_regs field in the given tdesc.
            * x86-tdesc.h: New file.
            * linux-aarch64-tdesc.c (aarch64_linux_read_description):
            Adjust following the addition of the new expedite_regs parameter
            to init_target_desc.
            * linux-tic6x-low.c (tic6x_read_description): Likewise.
            * linux-x86-tdesc.c: #include "x86-tdesc.h".
            (i386_linux_read_description, amd64_linux_read_description):
            Adjust following the addition of the new expedite_regs parameter
            to init_target_desc.
            * lynx-i386-low.c: #include "x86-tdesc.h".
            (lynx_i386_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
            * nto-x86-low.c: #include "x86-tdesc.h".
            (nto_x86_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
            * win32-i386-low.c: #include "x86-tdesc.h".
            (i386_arch_setup): Adjust following the addition of the new
            expedite_regs parameter to init_target_desc.
Comment 8 Joel Brobecker 2018-05-10 16:51:32 UTC
fixed on both master and gdb-8.1-branch.