diff mbox series

gdb: unix: allow to use custom baud rate

Message ID 2966392a439ab9a16f8561030ad08e63e7c204be.camel@espressif.com
State New
Headers
Series 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

Alexey Lapshin Sept. 26, 2024, 6 p.m. UTC 
  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

Keith Seitz Oct. 4, 2024, 7:18 p.m. UTC | #1 
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.
diff mbox series

Patch

diff --git a/gdb/config.in b/gdb/config.in
index 57be033802e..b445fc3dd8f 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -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
 
diff --git a/gdb/configure b/gdb/configure
index 53eaad4f0e2..1016737c09a 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -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'.
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)
+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 */
+#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)
 {