Bug 30770 - serial.c does not preserve errno correctly
Summary: serial.c does not preserve errno correctly
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: remote (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 15.1
Assignee: Tom Tromey
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-16 14:11 UTC by Tom Tromey
Modified: 2024-01-19 18:10 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Tromey 2023-08-16 14:11:04 UTC
Internally at AdaCore there is a test that randomly fails.
The error message differs, though:

Remote communication error.  Target disconnected.: Arg list too long.
Remote communication error.  Target disconnected.: No error.
Remote communication error.  Target disconnected.: Not a directory.

Looking into this, it seems to me that serial.c doesn't
always preserve the correct errno, but remote.c seems to assume
that it does.

For example, ser-base.c:generic_readchar does a lot of work
after do_readchar fails.
Comment 1 Tom Tromey 2023-08-29 12:53:29 UTC
I think this isn't really solveable, as the ser-mingw
code doesn't even use errno -- it makes Windows API calls.
Comment 2 Tom Tromey 2023-08-29 13:48:31 UTC
I see now that the Windows serial_open implementation just
sets errno, so perhaps that can be done elsewhere.
Comment 3 Tom Tromey 2023-08-31 13:20:38 UTC
I have some patches to try to do this, but now I realize
that I've only ever seen this error on Windows hosts.
On Windows I had my patch just do 'errno = EIO' rather than
try to translate Windows error codes, but I think that's
not very good.
Maybe the serial API has to change to throw exceptions instead.
Comment 5 Sourceware Commits 2023-11-27 20:14:15 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit 4dda9cc4b03788d1cf0416b39a3ab3780caf27fd
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Sep 1 11:04:58 2023 -0600

    Fix latent bug in ser_windows_send_break
    
    The ClearCommBreak documentation says:
    
        If the function fails, the return value is zero.
    
    ser_windows_send_break inverts this check.  This has never been
    noticed because the caller doesn't check the result.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 6 Sourceware Commits 2023-11-27 20:14:20 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit 602971b3863dcecf2daa5ffc0853a75a3131446c
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Sep 1 11:01:33 2023 -0600

    Introduce throw_winerror_with_name
    
    This introduces throw_winerror_with_name, a Windows analog of
    perror_with_name, and changes various places in gdb to call it.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 7 Sourceware Commits 2023-11-27 20:14:25 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit ad3cf8c64e6e4794fc48d28c90f20cbbfdc51ca4
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Aug 29 07:20:22 2023 -0600

    Change serial_setbaudrate to throw exception
    
    remote.c has this code:
    
          if (serial_setbaudrate (rs->remote_desc, baud_rate))
            {
              /* The requested speed could not be set.  Error out to
                 top level after closing remote_desc.  Take care to
                 set remote_desc to NULL to avoid closing remote_desc
                 more than once.  */
              serial_close (rs->remote_desc);
              rs->remote_desc = NULL;
              perror_with_name (name);
    
    The perror here cannot be correct, because if serial_setbaudrate did
    set errno, it may be obscured by serial_close.
    
    This patch changes serial_setbaudrate to throw an exception instead.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 8 Sourceware Commits 2023-11-27 20:14:30 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit a2e0acea420cca881296c6fcf58920f3d7c05a45
Author: Tom Tromey <tromey@adacore.com>
Date:   Fri Sep 1 12:11:37 2023 -0600

    Change serial "open" functions to throw exception
    
    remote.c assumes that a failure to open the serial connection will set
    errno.  This is somewhat true, because the Windows code tries to set
    errno appropriately -- but only somewhat, because it isn't clear that
    the "pex" code sets it, and the tcp code seems to do the wrong thing.
    It seems better to simply have the serial open functions throw on
    error.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 9 Sourceware Commits 2023-11-27 20:14:36 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit d69939bded50d76179f97284df35879a385cf8c0
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Sep 6 08:33:46 2023 -0600

    Change serial_send_break and serial_write to throw
    
    This changes serial_send_break and serial_write to throw exceptions
    rather than attempt to set errno and return an error indicator.  This
    lets us correctly report failures on Windows.
    
    Both functions had to be converted in a single patch because one
    implementation of send_break works via write.
    
    This also introduces remote_serial_send_break to handle error checking
    when attempting to send a break.  This was previously ignored.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 10 Sourceware Commits 2023-11-27 20:14:41 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit 0da23004a064e7149373b484fa671f2a2105ec9b
Author: Tom Tromey <tromey@adacore.com>
Date:   Tue Aug 29 08:22:09 2023 -0600

    Change serial_readchar to throw
    
    This changes serial_readchar to throw an exception rather than trying
    to set and preserve errno.
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770
Comment 11 Tom Tromey 2023-11-27 23:29:03 UTC
Fixed.
Comment 12 Sourceware Commits 2024-01-19 18:10:41 UTC
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit 74d8fa2df7e7844f9b1b8fac27fe7d0b82100ab0
Author: СеÑгей ЧеÑнов <klen_s@mail.ru>
Date:   Fri Jan 19 11:01:18 2024 -0700

    Fix remote serial read
    
    After closing "Bug 30770 - serial.c does not preserve errno correctly"
    https://sourceware.org/bugzilla/show_bug.cgi?id=30770
    remote debugging became impossible due to an attempt to recv() by a call intended for the socket, and not for the character device file. The
    documentation implicitly states that it is possible to use the read() call to work with a socket. But this does not mean in the general case that it is
    permissible to use recv in the case of a non-socket.
    
    condition:
    os: Distributor ID:    Ubuntu
    Description:    Ubuntu 23.10
    Release:    23.10
    Codename:    mantic
    
    libc:
    ldd (Ubuntu GLIBC 2.38-1ubuntu6) 2.38
    kernel:
    Linux klen-dev-um790pro 6.5.0-14-generic #14-Ubuntu SMP PREEMPT_DYNAMIC Tue Nov 14 14:59:49 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
    gdb: build from trank at 15.0.50.20231226-git
    
    GDB output:
    $ arm-kgp-eabi-gdb
    GNU gdb (Klen's_GNU_package_(KGP)_for_target::arm-kgp-eabi<rmprofile/lto>_host::x86_64-kgp-linux-gnu_znver4-avx512<<ílex>>)
    15.0.50.20231226-git
    ....
    (gdb) tar ext /dev/ttyACM1
    Remote debugging using /dev/ttyACM1
    Remote communication error.  Target disconnected: error while reading: Socket operation on non-socket.
    (gdb)
    
    after fix gdb work fine
    
    $ arm-kgp-eabi-gdb -q
    (gdb) tar ext /dev/ttyACM0
    Remote debugging using /dev/ttyACM0
    (gdb) mon swd
    Target voltage: 0.0V
    Available Targets:
    No. Att Driver
           STM32F40x M4
    (gdb) att 1
    Attaching to Remote target
    warning: No executable has been specified and target does not support
    determining executable automatically.  Try using the "file" command.
    0x08020c80 in ?? ()
    (gdb)
    
    Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30770