gdb: unix: allow to use custom baud rate
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Test passed
|
Commit Message
Modern unix systems allow to set custom baud rate instead of predefined in termios.h.
- To use this in Linux it must have BOTHER macro in "asm/termio.h"
- MacOS could handle this using IOSSIOSPEED passed to ioctl()
---
gdb/config.in | 3 ++
gdb/configure | 37 +++++++++++++++++
gdb/configure.ac | 20 ++++++++++
gdb/ser-unix.c | 102 +++++++++++++++++++++++++++++++++++++++++++++--
4 files changed, 158 insertions(+), 4 deletions(-)
--
2.43.0
Comments
Hi,
On 9/26/24 11:00 AM, Alexey Lapshin wrote:
> Modern unix systems allow to set custom baud rate instead of predefined in termios.h.
>
> - To use this in Linux it must have BOTHER macro in "asm/termio.h"
> - MacOS could handle this using IOSSIOSPEED passed to ioctl()
Nice! Thank you for the patch!
Warning: I am by no means conversant in serial communications
on POSIX or any other system. Nonetheless, I hope I can help nudge
this patch forward.
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 8368fea0423..4ffdf2d61ff 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1487,6 +1487,26 @@ if test "$gdb_cv_have_pt_getdbregs" = yes; then
> [Define if sys/ptrace.h defines the PT_GETDBREGS request.])
> fi
>
> +# See if <asm/termios.h> provides the BOTHER request.
> +AC_MSG_CHECKING(for PT_GETDBREGS)
checking for PT_GETDBREGS ?
> +AC_CACHE_VAL(
> + [gdb_cv_have_termios_bother],
> + [AC_COMPILE_IFELSE(
> + [AC_LANG_PROGRAM(
> + [#include <asm/termios.h>],
> + [BOTHER; TCGETS2;]
> + )],
> + [gdb_cv_have_termios_bother=yes],
> + [gdb_cv_have_termios_bother=no]
> + )]
> +)
> +AC_MSG_RESULT($gdb_cv_have_termios_bother)
> +if test "$gdb_cv_have_termios_bother" = yes; then
> + AC_DEFINE(HAVE_TERMIOS_BOTHER, 1,
> + [Define if asm/termios.h defines the BOTHER request.])
> +fi
> +
> +
> # See if <sys/ptrace.h> supports LWP names on FreeBSD
> # Older FreeBSD versions don't have the pl_tdname member of
> # `struct ptrace_lwpinfo'.
> diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
> index 02845aa938b..fe6ca3f0b94 100644
> --- a/gdb/ser-unix.c
> +++ b/gdb/ser-unix.c
> @@ -30,6 +30,22 @@
> #include "gdbsupport/gdb_select.h"
> #include "cli/cli-cmds.h"
> #include "gdbsupport/filestuff.h"
> +#if HAVE_TERMIOS_BOTHER || __APPLE__
> +#include <sys/ioctl.h>
> +#endif
> +#if __APPLE__
> +#include <IOKit/serial/ioss.h>
> +#ifdef IOSSIOSPEED
> +#define HAVE_APPLE_IOSSIOSPEED
> +#endif
> +#endif
> +#if HAVE_TERMIOS_BOTHER
> +/* Workaround to resolve conflicting declarations of termios
> + * in <asm/termbits.h> and <termios.h */
[Yuck, <rhetorical>is this really how this interface was
designed?</rhetorical>] "<termios.h>". Please terminate
comment with period and two spaces. [I realize that's not
grammatically correct, but it appears to be the norm in
gdb.]
> +#define termios asmtermios
> +#include <asm/termbits.h>
> +#undef termios
> +#endif
> #include <termios.h>
> #include "gdbsupport/scoped_ignore_sigttou.h"
>
> @@ -448,6 +495,53 @@ hardwire_setbaudrate (struct serial *scb, int rate)
> perror_with_name ("could not set tty state");
> }
>
> +static void
> +set_custom_baudrate (int fd, int rate)
> +{
All functions should have some sort of description:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Document_Every_Subprogram
> +#if HAVE_TERMIOS_BOTHER
> + struct termios2 tio;
> +
> + if (ioctl (fd, TCGETS2, &tio) < 0)
> + {
> + error (_("Can not set custom baud rate %d. Error is %d (TCGETS2)."),
> + rate, errno);
> + }
The error messages in this function are all almost identical and display
a numerical errno. I would prefer a more user-friendly approach using
safe_strerror or perror_with_name instead (and more concise error
messages).
> + tio.c_cflag &= ~CBAUD;
> + tio.c_cflag |= BOTHER;
> + tio.c_ispeed = rate;
> + tio.c_ospeed = rate;
Warning: I only know what I've read in ioctl_tty(2). This manpage
includes an example of how to set a custom baud rate:
/* Clear the current output baud rate and fill a new value */
tio.c_cflag &= ~CBAUD;
tio.c_cflag |= BOTHER;
tio.c_ospeed = atoi(argv[2]);
/* Clear the current input baud rate and fill a new value */
tio.c_cflag &= ~(CBAUD << IBSHIFT);
tio.c_cflag |= BOTHER << IBSHIFT;
The patch does not include this IBSHIFT flag-setting. Should it?
> + if (ioctl (fd, TCSETS2, &tio) < 0)
> + {
> + error (_("Can not set custom baud rate %d. Error is %d (TCSETS2)."),
> + rate, errno);
> + }
> +
> +#elif HAVE_APPLE_IOSSIOSPEED
> + if (ioctl (fd, IOSSIOSPEED, &rate) < 0)
> + {
> + error (_("Can not set custom baud rate %d. Error is %d."),
> + rate, errno);
> + }
> +#endif
> +}
> +
> +static void
> +hardwire_setbaudrate (struct serial *scb, int rate)
Also missing a description.
> +{
> + int baud_code = rate_to_code (rate);
> +
> + if (baud_code < 0)
> + {
> + /* The baud rate code was not found.
> + Try to set a custom baud rate. */
> + set_custom_baudrate (scb->fd, rate);
> + }
> + else
> + {
> + set_baudcode_baudrate (scb, baud_code);
> + }
[silly] Braces not needed/desired here.
> +}
> +
> static int
> hardwire_setstopbits (struct serial *scb, int num)
> {
Thanks again for the patch!
Keith
PS. Do you have an assignment on file? If not, this patch is
sufficiently large enough to require one.
Hi Keith!
Thank you very much for such neat review, it helped me to rewrite the code in more proper way.
Hope it's okay now with formatting.
I have tested my changes on Linux and this works well. Have no ability to test on MacOS,
but it seems the code for macos is very simple and should work.
Please find the version 2 patch in the following message.
> PS. Do you have an assignment on file? If not, this patch issufficiently large enough to require one.
I dont.. Could you please point out what I should do regarding this?
On Fri, 4 Oct 2024, Keith Seitz wrote:
> > diff --git a/gdb/ser-unix.c b/gdb/ser-unix.c
> > index 02845aa938b..fe6ca3f0b94 100644
> > --- a/gdb/ser-unix.c
> > +++ b/gdb/ser-unix.c
> > @@ -30,6 +30,22 @@
> > #include "gdbsupport/gdb_select.h"
> > #include "cli/cli-cmds.h"
> > #include "gdbsupport/filestuff.h"
> > +#if HAVE_TERMIOS_BOTHER || __APPLE__
> > +#include <sys/ioctl.h>
> > +#endif
> > +#if __APPLE__
> > +#include <IOKit/serial/ioss.h>
> > +#ifdef IOSSIOSPEED
> > +#define HAVE_APPLE_IOSSIOSPEED
> > +#endif
> > +#endif
> > +#if HAVE_TERMIOS_BOTHER
> > +/* Workaround to resolve conflicting declarations of termios
> > + * in <asm/termbits.h> and <termios.h */
> [Yuck, <rhetorical>is this really how this interface was
> designed?</rhetorical>]
It really needs to be sorted properly in libc so as to BOTHER to come
from <termios.h> along with the rest of Bxxx baud rate flags; we have
other non-POSIX definitions already. We've had support for BOTHER in
Linux since 2006, and 18 years later still need to resort to such hacks:
> > +#define termios asmtermios
> > +#include <asm/termbits.h>
> > +#undef termios
> > +#endif
> > #include <termios.h>
! I've been complained to by fellow Linux kernel developers about
people hesitating to use BOTHER in their user software and this is
surely one reason.
CC-ing libc-alpha for awareness; we probably want a bug report here.
Maciej
On Okt 16 2024, Maciej W. Rozycki wrote:
> It really needs to be sorted properly in libc so as to BOTHER to come
> from <termios.h> along with the rest of Bxxx baud rate flags; we have
> other non-POSIX definitions already. We've had support for BOTHER in
> Linux since 2006, and 18 years later still need to resort to such hacks:
How is that different from CBAUDEX?
On Wed, 23 Oct 2024, Andreas Schwab wrote:
> > It really needs to be sorted properly in libc so as to BOTHER to come
> > from <termios.h> along with the rest of Bxxx baud rate flags; we have
> > other non-POSIX definitions already. We've had support for BOTHER in
> > Linux since 2006, and 18 years later still need to resort to such hacks:
>
> How is that different from CBAUDEX?
Semantics, cf. Linux code:
/* Magic token for arbitrary speed via c_ispeed/c_ospeed */
if (cbaud == BOTHER)
return termios->c_ospeed;
if (cbaud & CBAUDEX) {
cbaud &= ~CBAUDEX;
cbaud += 15;
}
There's no requirement/guarantee for the two macros to expand to the same
value.
Maciej
On Okt 23 2024, Maciej W. Rozycki wrote:
> On Wed, 23 Oct 2024, Andreas Schwab wrote:
>
>> > It really needs to be sorted properly in libc so as to BOTHER to come
>> > from <termios.h> along with the rest of Bxxx baud rate flags; we have
>> > other non-POSIX definitions already. We've had support for BOTHER in
>> > Linux since 2006, and 18 years later still need to resort to such hacks:
>>
>> How is that different from CBAUDEX?
>
> Semantics, cf. Linux code:
>
> /* Magic token for arbitrary speed via c_ispeed/c_ospeed */
> if (cbaud == BOTHER)
> return termios->c_ospeed;
>
> if (cbaud & CBAUDEX) {
> cbaud &= ~CBAUDEX;
> cbaud += 15;
> }
I see. Unfortunately, AFAICS it cannot be supported by
cf[sg]et[io]speed, which lowers its usefulness.
On Thu, 24 Oct 2024, Andreas Schwab wrote:
> >> How is that different from CBAUDEX?
> >
> > Semantics, cf. Linux code:
> >
> > /* Magic token for arbitrary speed via c_ispeed/c_ospeed */
> > if (cbaud == BOTHER)
> > return termios->c_ospeed;
> >
> > if (cbaud & CBAUDEX) {
> > cbaud &= ~CBAUDEX;
> > cbaud += 15;
> > }
>
> I see. Unfortunately, AFAICS it cannot be supported by
> cf[sg]et[io]speed, which lowers its usefulness.
Indeed, but it seems feasible to me to have our API updated such as to
support this transparently (and even match our documentation) and in a
POSIX-compliant manner.
I have filed BZ #32328 with hopefully all the necessarily details; cf.
<https://sourceware.org/bugzilla/show_bug.cgi?id=32328>. I can't commit
to implementing this at the moment though, but it doesn't appear to me to
be a terrible challenge; just a small project.
Maciej
@@ -562,6 +562,9 @@
/* Define to 1 if you have the <sys/wait.h> header file. */
#undef HAVE_SYS_WAIT_H
+/* Define if asm/termio.h defines the BOTHER request. */
+#undef HAVE_TERMIOS_BOTHER
+
/* Define to 1 if you have the <termios.h> header file. */
#undef HAVE_TERMIOS_H
@@ -30386,6 +30386,43 @@ $as_echo "#define HAVE_PT_GETDBREGS 1" >>confdefs.h
fi
+# See if <asm/termios.h> provides the BOTHER request.
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for PT_GETDBREGS" >&5
+$as_echo_n "checking for PT_GETDBREGS... " >&6; }
+if ${gdb_cv_have_termios_bother+:} false; then :
+ $as_echo_n "(cached) " >&6
+else
+ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h. */
+#include <asm/termios.h>
+int
+main ()
+{
+BOTHER; TCGETS2;
+
+ ;
+ return 0;
+}
+_ACEOF
+if ac_fn_c_try_compile "$LINENO"; then :
+ gdb_cv_have_termios_bother=yes
+else
+ gdb_cv_have_termios_bother=no
+
+fi
+rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+
+fi
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gdb_cv_have_termios_bother" >&5
+$as_echo "$gdb_cv_have_termios_bother" >&6; }
+if test "$gdb_cv_have_termios_bother" = yes; then
+
+$as_echo "#define HAVE_TERMIOS_BOTHER 1" >>confdefs.h
+
+fi
+
+
# See if <sys/ptrace.h> supports LWP names on FreeBSD
# Older FreeBSD versions don't have the pl_tdname member of
# `struct ptrace_lwpinfo'.
@@ -1487,6 +1487,26 @@ if test "$gdb_cv_have_pt_getdbregs" = yes; then
[Define if sys/ptrace.h defines the PT_GETDBREGS request.])
fi
+# See if <asm/termios.h> provides the BOTHER request.
+AC_MSG_CHECKING(for PT_GETDBREGS)
+AC_CACHE_VAL(
+ [gdb_cv_have_termios_bother],
+ [AC_COMPILE_IFELSE(
+ [AC_LANG_PROGRAM(
+ [#include <asm/termios.h>],
+ [BOTHER; TCGETS2;]
+ )],
+ [gdb_cv_have_termios_bother=yes],
+ [gdb_cv_have_termios_bother=no]
+ )]
+)
+AC_MSG_RESULT($gdb_cv_have_termios_bother)
+if test "$gdb_cv_have_termios_bother" = yes; then
+ AC_DEFINE(HAVE_TERMIOS_BOTHER, 1,
+ [Define if asm/termios.h defines the BOTHER request.])
+fi
+
+
# See if <sys/ptrace.h> supports LWP names on FreeBSD
# Older FreeBSD versions don't have the pl_tdname member of
# `struct ptrace_lwpinfo'.
@@ -30,6 +30,22 @@
#include "gdbsupport/gdb_select.h"
#include "cli/cli-cmds.h"
#include "gdbsupport/filestuff.h"
+#if HAVE_TERMIOS_BOTHER || __APPLE__
+#include <sys/ioctl.h>
+#endif
+#if __APPLE__
+#include <IOKit/serial/ioss.h>
+#ifdef IOSSIOSPEED
+#define HAVE_APPLE_IOSSIOSPEED
+#endif
+#endif
+#if HAVE_TERMIOS_BOTHER
+/* Workaround to resolve conflicting declarations of termios
+ * in <asm/termbits.h> and <termios.h */
+#define termios asmtermios
+#include <asm/termbits.h>
+#undef termios
+#endif
#include <termios.h>
#include "gdbsupport/scoped_ignore_sigttou.h"
@@ -289,10 +305,28 @@ baudtab[] =
4800, B4800
}
,
+#ifdef B7200
+ {
+ 7200, B7200
+ }
+ ,
+#endif
{
9600, B9600
}
,
+#ifdef B14400
+ {
+ 14400, B14400
+ }
+ ,
+#endif
+#ifdef B28800
+ {
+ 28800, B28800
+ }
+ ,
+#endif
{
19200, B19200
}
@@ -307,6 +341,12 @@ baudtab[] =
}
,
#endif
+#ifdef B76800
+ {
+ 76800, B76800
+ }
+ ,
+#endif
#ifdef B115200
{
115200, B115200
@@ -412,6 +452,7 @@ rate_to_code (int rate)
/* check if it is in between valid values. */
if (rate < baudtab[i].rate)
{
+#if !(HAVE_TERMIOS_BOTHER || HAVE_APPLE_IOSSIOSPEED)
if (i)
{
error (_("Invalid baud rate %d. "
@@ -423,21 +464,27 @@ rate_to_code (int rate)
error (_("Invalid baud rate %d. Minimum value is %d."),
rate, baudtab[0].rate);
}
+#else
+ return -1;
+#endif
}
}
}
-
+
+#if !(HAVE_TERMIOS_BOTHER || HAVE_APPLE_IOSSIOSPEED)
/* The requested speed was too large. */
error (_("Invalid baud rate %d. Maximum value is %d."),
rate, baudtab[i - 1].rate);
+#else
+ return -1;
+#endif
}
static void
-hardwire_setbaudrate (struct serial *scb, int rate)
+set_baudcode_baudrate (struct serial *scb, int baud_code)
{
struct hardwire_ttystate state;
- int baud_code = rate_to_code (rate);
-
+
if (get_tty_state (scb, &state))
perror_with_name ("could not get tty state");
@@ -448,6 +495,53 @@ hardwire_setbaudrate (struct serial *scb, int rate)
perror_with_name ("could not set tty state");
}
+static void
+set_custom_baudrate (int fd, int rate)
+{
+#if HAVE_TERMIOS_BOTHER
+ struct termios2 tio;
+
+ if (ioctl (fd, TCGETS2, &tio) < 0)
+ {
+ error (_("Can not set custom baud rate %d. Error is %d (TCGETS2)."),
+ rate, errno);
+ }
+ tio.c_cflag &= ~CBAUD;
+ tio.c_cflag |= BOTHER;
+ tio.c_ispeed = rate;
+ tio.c_ospeed = rate;
+ if (ioctl (fd, TCSETS2, &tio) < 0)
+ {
+ error (_("Can not set custom baud rate %d. Error is %d (TCSETS2)."),
+ rate, errno);
+ }
+
+#elif HAVE_APPLE_IOSSIOSPEED
+ if (ioctl (fd, IOSSIOSPEED, &rate) < 0)
+ {
+ error (_("Can not set custom baud rate %d. Error is %d."),
+ rate, errno);
+ }
+#endif
+}
+
+static void
+hardwire_setbaudrate (struct serial *scb, int rate)
+{
+ int baud_code = rate_to_code (rate);
+
+ if (baud_code < 0)
+ {
+ /* The baud rate code was not found.
+ Try to set a custom baud rate. */
+ set_custom_baudrate (scb->fd, rate);
+ }
+ else
+ {
+ set_baudcode_baudrate (scb, baud_code);
+ }
+}
+
static int
hardwire_setstopbits (struct serial *scb, int num)
{