Re: [PATCH] Negative repeat count for 'x' command

[Pedro Alves <palves at redhat dot com>]

Hi Toshihito,

Many thanks again for following through with this, and my
apologies for taking so long to get to it.  I'm impressed on
how you made the patch cover alls kinds of formats, including
strings, and on the extend of test coverage you added.  Kudos.

See review comments below.

On 02/07/2016 11:18 PM, Toshihito Kikuchi wrote:
> Thank you for the feedbacks.  I updated the patch adding negative repeat count
> support to the 'x' command instead of a new format letter.  Also, this patch
> includes the update of document and NEWS file.
>
> With this, we can specify a negative repeat count for all the formats in the 'x'
> command to examine memory backward.
>
> Here's the example output:
> (gdb) bt
> #0  Func1 (n=42, p=0x40432e "hogehoge") at main.cpp:5
> #1  0x00000000004041fa in main (argc=1, argv=0x7fffffffdff8) at main.cpp:19
> (gdb) x/-4i 0x4041fa
>     0x4041e5 <main(int, char**)+11>:	mov    %rsi,-0x10(%rbp)
>     0x4041e9 <main(int, char**)+15>:	lea    0x13e(%rip),%rsi        # 0x40432e
>     0x4041f0 <main(int, char**)+22>:	mov    $0x2a,%edi
>     0x4041f5 <main(int, char**)+27>:	callq  0x404147 <Func1(int, char const*)>
> (gdb) x/-4xw 0x404200
> 0x4041f0 <main(int, char**)+22>:	0x00002abf	0xff4de800	0x76e8ffff	0xb8ffffff
> (gdb) x/-4
> 0x4041e0 <main(int, char**)+6>:	0x7d8910ec	0x758948fc	0x358d48f0	0x0000013e
>
> Tested on i386-linux and amd64-linux. I wrote a new testcase 'examine-backward'
> under gdb.base.  It mainly focuses on x/-#s and x/-#i.

Can we have something for e.g., x/x as well?  That one at least
could be tested on all archs.

> For disassembler
> testing, the new testcase covers only i386 and amd64, which are skipped onother
> platforms.
>
> gdb/ChangeLog:
>
> 	* NEWS: Mention that GDB now supports a negative repeat count in
> 	the 'x' command.
> 	* printcmd.c (decode_format): Allow '-' in the parameter
> 	"string_ptr" to accept a negative repeat count.
> 	(null_fprintf): New function.
> 	(null_print_address): New function.
> 	(find_instruction_backward): New function.
> 	(read_memory_backward): New function.
> 	(integer_is_zero): New function.
> 	(find_string_backward): New function.
> 	(do_examine): Use new functions to examine memory backward.
>
> gdb/doc/ChangeLog:
>
> 	* gdb.texinfo (Examining Memory): Document negative repeat
> 	count in the 'x' command.
>
> gdb/testsuite/ChangeLog:
>
> 	* gdb.base/examine-backward.c: New file.
> 	* gdb.base/examine-backward.exp: New file.
>
>
> 0001-Add-negative-repeat-count-to-x-command.patch
>
>
>  From aa4363c9cf6ad3c6ebcef3b7ea1c72e4180e89f2 Mon Sep 17 00:00:00 2001
> From: tokikuch<k.toshihito@yahoo.de>
> Date: Sun, 7 Feb 2016 13:24:53 -0800
> Subject: [PATCH] Add negative repeat count to 'x' command
>
> ---

Please maintain the patch description in the git log, as that's
what we'll end up pushing to the repo.


>   gdb/NEWS                                    |  14 ++
>   gdb/doc/gdb.texinfo                         |  14 +-
>   gdb/printcmd.c                              | 259 +++++++++++++++++++++++++++-
>   gdb/testsuite/gdb.base/examine-backward.c   | 143 +++++++++++++++
>   gdb/testsuite/gdb.base/examine-backward.exp | 219 +++++++++++++++++++++++
>   5 files changed, 646 insertions(+), 3 deletions(-)
>   create mode 100644 gdb/testsuite/gdb.base/examine-backward.c
>   create mode 100644 gdb/testsuite/gdb.base/examine-backward.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 962eae4..072c11a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,20 @@
>
>   *** Changes since GDB 7.10
>
> +* GDB now supports a negative repeat count in the 'x' command to examine
> +  memory backward from the given address.  For example:

Please move this to the "since 7.11" section.

>
> +If a negative repeat count is specified for the formats @samp{s} or @samp{i},
> +the absolute value of the given number of null-terminated strings or
> +instructions before the address are displayed.  For the @samp{i} format,
> +we use line number information in the debug info to resolve a correct frame
> +while dissasembling backward. 

What does "a correct frame" mean?

> If line info is not available, the command
> +stops examining memory with the error message.

"with the error message" -> "with an error message".

> +
>   All the defaults for the arguments to @code{x} are designed to make it
>   easy to continue scanning memory with minimal specifications each time
>   you use @code{x}.  For example, after you have inspected three machine
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index f5c4211..e0ffcc0 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -47,6 +47,7 @@
>   #include "cli/cli-utils.h"
>   #include "format.h"
>   #include "source.h"
> +#include "linespec.h"
>
>   #ifdef TUI
>   #include "tui/tui.h"		/* For tui_active et al.   */
> @@ -186,8 +187,13 @@ decode_format (const char **string_ptr, int oformat, int osize)
>     val.count = 1;
>     val.raw = 0;
>
> +  if (*p == '-')
> +    {
> +      val.count = -1;
> +      p++;
> +    }
>     if (*p >= '0' && *p <= '9')
> -    val.count = atoi (p);
> +    val.count *= atoi (p);
>     while (*p >= '0' && *p <= '9')
>       p++;
>
> @@ -785,6 +791,221 @@ print_address_demangle (const struct value_print_options *opts,
>   }
>
>
> +/* These empty functions are used in find_instruction_backward to
> +   suppress standard output during the call to gdbarch_print_insn.  */
> +
> +static int
> +null_fprintf (void *stream, const char *format ATTRIBUTE_UNUSED, ...)
> +{
> +  return 0;
> +}
> +
> +static void
> +null_print_address (bfd_vma addr, struct disassemble_info *info)
> +{
> +  return;

Unnecessary return.  But see below.

> +}
> +
> +/* Rewind assembly from ADDR and return the start address after the given
> +   number of lines are disassembled. To avoid disassembling in a wrong frame,
> +   we get addresses in a correct frame using line information.

References to "wrong frame" here again.  What does this mean?  Also,
double-space after periods.

> +   If we go out of the symbol range during disassembling, we return
> +   the smallest address we've got so far and set the number of lines read
> +   to lines_read.  */

s/smallest/lowest.

> +
> +static CORE_ADDR
> +find_instruction_backward (struct gdbarch *gdbarch, CORE_ADDR addr,
> +          int count, int *lines_read)

"int count" should line up below "struct gdbarch".

> +{
> +  char addrstr[64];
> +  struct symtabs_and_lines sals;
> +  CORE_ADDR start_pc, end_pc, ret;
> +
> +  start_pc = end_pc = 0;
> +  ret = addr;
> +  *lines_read = 0;
> +
> +  /* Get the start address of a line which includes the address (addr - 1).  */
> +  sprintf (addrstr, "*%p", (void *) (addr - 1));

Use paddress for target addresses.  %p is for host addresses (and we don't
use it, as it requires C99).

> +  sals = decode_line_with_last_displayed (addrstr, DECODE_LINE_FUNFIRSTLINE);

But in any case, why do we want to use ..._with_last_displayed here?
We have an address already, so shouldn't we use find_pc_sect_line or 
some such?

> +  if (sals.nelts >= 1)
> +    {
> +      if ((!sals.sals[0].line)
> +          || (!find_line_pc_range (sals.sals[0], &start_pc, &end_pc)))

Remove unnecessary parentheses. No implicit boolean conversion:

  if (sals.sals[0].line == 0
      || !find_line_pc_range (sals.sals[0], &start_pc, &end_pc))



> +        {
> +          printf_filtered (_("No line number information available "
> +            "for address "));

Align the "for" under "No".

> +          wrap_here ("  ");
> +          print_address (gdbarch, addr - 1, gdb_stdout);
> +          printf_filtered ("\n");
> +          start_pc = end_pc = 0;
> +        }
> +      xfree (sals.sals);

Should be released with a cleanup instead.

> +    }
> +
> +  if (start_pc)


  if (start_pc != 0)


> +    {
> +      VEC (CORE_ADDR) *pcs = NULL;
> +      struct disassemble_info di;
> +      CORE_ADDR p;
> +      int i;
> +
> +      VEC_reserve (CORE_ADDR, pcs, count);

Use a VEC_cleanup cleanup to release this.  But what's the
idea behind the VEC?  Should have a leading comment.
Looks like it can grow big if you try to disassemble a large
region.  Do we really need it?

> +
> +      di = gdb_disassemble_info (gdbarch, NULL);
> +      di.fprintf_func = null_fprintf;
> +      di.print_address_func = null_print_address;

Seems like you could just use gdb_insn_length instead.

> +
> +      /* Record each instruction address from START_PC to ADDR.  */
> +      p = start_pc;
> +      for (i = 0; p < addr; ++i)
> +        {
> +          VEC_safe_push (CORE_ADDR, pcs, p);
> +          p += gdbarch_print_insn (gdbarch, (CORE_ADDR) p, &di);
> +        }

Seems like you don't need 'i' -- use VEC_length instead ?

> +
> +      /* If we reach COUNT instructions, return the address recorded in
> +         VECTOR. Otherwise, make a recursive call to disassemble
> +         the previoius line.  */

Typo "previoius".  Double-space after periods.  This recursion sounds
nasty -- sounds like if you request a high number of addresses, crossing
many lines, we'll hit stack limits.  Can we do without recursion?


> +      if (i >= count)
> +        {
> +          ret = VEC_index (CORE_ADDR, pcs, i - count);
> +          *lines_read = count;
> +        }
> +        else
> +        {
> +          int linesread_internal = 0;
> +          ret = find_instruction_backward (gdbarch,
> +               VEC_index (CORE_ADDR, pcs, 0), count - i,
> +               &linesread_internal);

Alignment.

> +          *lines_read = i + linesread_internal;
> +
> +          /* Return the smallest valid address we've got

"lowest".

> +             if the recursive call above failed.  */
> +          if (!ret)
> +            ret = VEC_index (CORE_ADDR, pcs, 0);
> +        }
> +      VEC_free (CORE_ADDR, pcs);

Replace with cleanup.

> +    }
> +  return ret;
> +}
> +
> +/* This is backward version of partial_memory_read in valprint.c.
> +   Read LEN bytes of target memory at address MEMADDR, placing the
> +   results in GDB's memory at MYADDR.  Returns a count of the bytes
> +   actually read.  */
> +
> +static int
> +read_memory_backward (struct gdbarch *gdbarch,
> +          CORE_ADDR memaddr, gdb_byte *myaddr, int len)

Alignment.

> +{
> +  int errcode;
> +  int nread;      /* Number of bytes actually read.  */
> +
> +  /* First try a complete read.  */
> +  errcode = target_read_memory (memaddr, myaddr, len);
> +  if (errcode == 0)
> +    {
> +      /* Got it all.  */
> +      nread = len;
> +    }
> +  else
> +    {
> +      /* Loop, reading one byte at a time until we get as much as we can.  */
> +      for (errcode = 0, nread = 0, memaddr += len, myaddr += len;
> +           len > 0 && errcode == 0;
> +           ++nread, --len)
> +        {
> +          errcode = target_read_memory (--memaddr, --myaddr, 1);
> +        }
> +      /* If an error, the last read was unsuccessful, so adjust count.  */
> +      if (errcode != 0)
> +        {
> +          --nread;
> +          printf_filtered (_("Cannot access memory at address "));
> +          wrap_here ("  ");
> +          print_address (gdbarch, memaddr, gdb_stdout);
> +          printf_filtered ("\n");

Do we need that wrap_here ?  You could make this a single printf_filtered
with %s and paddress instead of print_address instead.

> +        }
> +    }
> +  return (nread);

Unnecessary parens.

> +}
> +
> +/* This is integer version of decimal_is_zero in dfp.c.
> +   Returns true if X (which is LEN bytes wide) is the number zero.  */
> +
> +static int
> +integer_is_zero (const gdb_byte *x, int len)
> +{
> +  int i = 0;

Empty line after decl.

> +  while (i < len && x[i] == 0)
> +    ++i;
> +  return (i == len);
> +}
> +
> +/* Find the start address of a string in which ADDR is included.
> +   Basically we search for '\0' and return the next address,
> +   but if OPTIONS->PRINT_MAX is smaller than the length of a string,
> +   we stop searching and return the address to print PRINT_MAX of
> +   characters from the string.  */
> +
> +static CORE_ADDR
> +find_string_backward (struct gdbarch *gdbarch,
> +          CORE_ADDR addr, int count, int char_size,
> +          const struct value_print_options *options,
> +          int *strings_counted)

Alignment.

> +{
> +  const int chunk_size = 0x20;
> +  gdb_byte *buffer = NULL;
> +  int read_error = 0;
> +  int chars_read = 0;
> +  int chars_to_read = chunk_size;
> +  int chars_counted = 0;
> +  int count_original = count;
> +  CORE_ADDR string_start_addr = addr;
> +
> +  gdb_assert (char_size == 1 || char_size == 2 || char_size == 4);
> +
> +  buffer = (gdb_byte *) xmalloc (chars_to_read * char_size);
> +  while (count > 0 && read_error == 0)
> +    {
> +      int i;

Empty line.

> +      addr -= (chars_to_read * char_size);

Unnecessary parens.

> +      chars_read = read_memory_backward (gdbarch,
> +           addr, buffer, chars_to_read * char_size) / char_size;

Alignment.

> +      read_error = (chars_read == chars_to_read) ? 0 : 1;
> +
> +      /* Searching for '\0' from the end of buffer in backward direction.  */
> +      for (i = 0; i < chars_read && count > 0 ; ++i, ++chars_counted)
> +        {
> +          int offset = (chars_to_read - i - 1) * char_size;

Empty line.

> +          if (integer_is_zero (buffer + offset, char_size)
> +              || (chars_counted == options->print_max))

Parens.

> +            {
> +              /* Found '\0' or reached print_max.  As OFFSET is the offset to
> +                 '\0', we add CHAR_SIZE to return the start address of
> +                 a string.  */
> +              --count;
> +              string_start_addr = addr + offset + char_size;
> +              chars_counted = 0;
> +            }
> +        }
> +    }
> +  xfree(buffer);

Free with a cleanup instead.

> +
> +  /* Update STRINGS_COUNTED with the actual number of loaded strings.  */
> +  *strings_counted = count_original - count;
> +
> +  if (read_error != 0)
> +    {
> +      /* In error case, STRING_START_ADDR is pointing to the string which
> +         is last successfully loaded.  Rewind the partially loaded string.  */
> +      string_start_addr -= (chars_counted * char_size);

Parens.

> +    }
> +
> +  return string_start_addr;
> +}
> +
>   /* Examine data at address ADDR in format FMT.
>      Fetch it from memory and print on gdb_stdout.  */
>
> @@ -798,6 +1019,7 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>     int i;
>     int maxelts;
>     struct value_print_options opts;
> +  CORE_ADDR addr_rewound = 0;
>
>     format = fmt.format;
>     size = fmt.size;
> @@ -868,6 +1090,35 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>
>     get_formatted_print_options (&opts, format);
>
> +  if (count < 0)
> +    {
> +      /* This is the negative repeat count case.
> +         We rewind the address based on the given repeat count and format,
> +         then examine memory from there in forward direction.  */
> +
> +      count = -count;
> +      if (format == 'i')
> +        {
> +          next_address = find_instruction_backward (gdbarch,
> +               addr, count, &count);

Alignment.

> +        }
> +      else if (format == 's')
> +        {
> +          next_address = find_string_backward (gdbarch,
> +               addr, count, val_type->length, &opts, &count);

Alignment.

> +        }
> +      else
> +        {
> +          next_address = addr - count * val_type->length;
> +        }
> +
> +      /* The following call to print_formatted updates next_address in every
> +         iteration. In backward case, we store the start address here
> +         and update next_address with it before exiting the function.  */
> +      addr_rewound = (format == 's') ?
> +        (next_address - val_type->length) : next_address;

Alignment -- remove unnecessary precedence-related parens, but wrap the
whole rhs expression in an extra set of parens for alignment:

      addr_rewound = (format == 's'
		      ? next_address - val_type->length
		      : next_address);

> +    }
> +
>     /* Print as many objects as specified in COUNT, at most maxelts per line,
>        with the address of the next one at the start of each line.  */
>
> @@ -913,6 +1164,9 @@ do_examine (struct format_data fmt, struct gdbarch *gdbarch, CORE_ADDR addr)
>         printf_filtered ("\n");
>         gdb_flush (gdb_stdout);
>       }
> +
> +  if (addr_rewound)

addr_rewound != 0

But actually, please make sure that "address == 0" is treated as a
valid address (not just here, but in the whole patch), as it is valid
on non-MMU machines.  That may suggest using a separate boolean variable
to represent "have address".

> +    next_address = addr_rewound;
>   }
>
>   static void
> @@ -2522,7 +2776,8 @@ Format letters are o(octal), x(hex), d(decimal), u(unsigned decimal),\n\
>     and z(hex, zero padded on the left).\n\
>   Size letters are b(byte), h(halfword), w(word), g(giant, 8 bytes).\n\
>   The specified number of objects of the specified size are printed\n\
> -according to the format.\n\n\
> +according to the format.  If a negative number is specified, memory is\n\
> +examined backward from the address.\n\n\
>   Defaults for format and size letters are those previously used.\n\
>   Default count is 1.  Default address is following last thing printed\n\
>   with this command or \"print\"."));
> diff --git a/gdb/testsuite/gdb.base/examine-backward.c b/gdb/testsuite/gdb.base/examine-backward.c
> new file mode 100644
> index 0000000..3ce77d8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/examine-backward.c
> @@ -0,0 +1,143 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2015 Free Software Foundation, Inc.

2015-2016

> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see<http://www.gnu.org/licenses/>.  */
> +
> +/*
> +Define TestStrings, TestStringsH, and TestStringsW to test utf8, utf16,
> +and utf32 strings respectively.
> +To avoid compile error due to old compiler mode, we don't use string
> +literals. The content of each array is the same as followings:

Double-space after periods.

> +
> +  const char TestStrings[] = {
> +      "ABCD"
> +      "EFGHIJKLMNOPQRSTUVWXYZ\0"
> +      "\0"
> +      "\0"
> +      "\u307B\u3052\u307B\u3052\0"
> +      "012345678901234567890123456789\0"
> +      "!!!!!!\0"
> +  };
> +*/
> +
> +const char TestStrings[] = {
> +  0x41, 0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48,

I wonder whether 'A', 'B', etc. would work?

> +
> +static __attribute__((noinline)) void
> +TestFunction() {

static __attribute__((noinline)) void 
TestFunction (void)
{

> +
> +#if (defined __x86_64__)
> +  asm ("mov    %rdi,-0x18(%rbp)");
> +  asm ("movl   $0x0,-0x4(%rbp)");
> +  asm ("label1:");
> +  asm ("mov    -0x18(%rbp),%rax");
> +  asm ("mov    0x10(%rax),%eax");
> +  asm ("cmp    -0x4(%rbp),%eax");
> +  asm ("jle    label2");
> +  asm ("mov    -0x18(%rbp),%rax");
> +  asm ("mov    0x8(%rax),%rax");
> +  asm ("mov    -0x18(%rbp),%rdx");
> +  asm ("mov    0x8(%rdx),%rdx");
> +  asm ("vmovsd (%rdx),%xmm2");
> +  asm ("mov    -0x4(%rbp),%edx");
> +  asm ("add    $0x1,%edx");
> +  asm ("vxorpd %xmm0,%xmm0,%xmm0");
> +  asm ("vcvtsi2sd %edx,%xmm0,%xmm0");
> +  asm ("vmovsd 0x725(%rip),%xmm1");
> +  asm ("vdivsd %xmm0,%xmm1,%xmm0");
> +  asm ("vaddsd %xmm0,%xmm2,%xmm0");
> +  asm ("vmovsd %xmm0,(%rax)");
> +  asm ("addl   $0x1,-0x4(%rbp)");
> +  asm ("jmp    label1");
> +  asm ("label2:");
> +  asm ("nop");
> +#elif (defined __i386__)
> +  asm ("sub    $0x18,%esp");
> +  asm ("add    $0x327b,%edx");
> +  asm ("movl   $0x0,-0x4(%ebp)");
> +  asm ("label1:");
> +  asm ("mov    0x8(%ebp),%eax");
> +  asm ("mov    0x8(%eax),%eax");
> +  asm ("cmp    -0x4(%ebp),%eax");
> +  asm ("jle    label2");
> +  asm ("mov    0x8(%ebp),%eax");
> +  asm ("mov    0x4(%eax),%eax");
> +  asm ("mov    0x8(%ebp),%ecx");
> +  asm ("mov    0x4(%ecx),%ecx");
> +  asm ("fldl   (%ecx)");
> +  asm ("mov    -0x4(%ebp),%ecx");
> +  asm ("add    $0x1,%ecx");
> +  asm ("mov    %ecx,-0x14(%ebp)");
> +  asm ("fildl  -0x14(%ebp)");
> +  asm ("fld1   ");
> +  asm ("fdivp  %st,%st(1)");
> +  asm ("faddp  %st,%st(1)");
> +  asm ("fstpl  (%eax)");
> +  asm ("addl   $0x1,-0x4(%ebp)");
> +  asm ("jmp    label1");
> +  asm ("label2:");
> +  asm ("nop");
> +#endif
> +};
> +
> +int main () {
> +    return 42;
> +}
> diff --git a/gdb/testsuite/gdb.base/examine-backward.exp b/gdb/testsuite/gdb.base/examine-backward.exp
> new file mode 100644
> index 0000000..a3ee008
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/examine-backward.exp
> @@ -0,0 +1,219 @@
> +# Copyright 2015 Free Software Foundation, Inc.

2015-2016

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see<http://www.gnu.org/licenses/>.

Missing space in "see<http" ?

> +
> +standard_testfile
> +if { [prepare_for_testing "failed to prepare for examine-backward" \
> +        ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    untested "could not run to main"
> +    return -1
> +}
> +
> +proc get_first_mapped_address {} {

Needs intro comment.  Use of "info proc mappings" limits the
test to some ports only.  Why do we need this?  Etc.

> +    global gdb_prompt

Empty line after.

> +    set addr "0"
> +    gdb_test_multiple "info proc mappings" "info proc mappings" {
> +    -re "objfile\[\r\n\t \]+(0x\[0-9a-fA-F\]+).*\[\r\n\]*$gdb_prompt $" {
> +        set addr $expect_out(1,string)
> +    }
> +    timeout {
> +        fail "Fail to get process address spaces (timeout)."
> +    }

gdb_test_multiple already issues a FAIL on timeout, internally.

> +    }
> +    return ${addr}
> +}
> +
> +# invalid format (# of tests = 5)
> +
> +gdb_test "x/- 10xb main" "Invalid number \"10xb\"\."
> +gdb_test "x/--10xb main" "Invalid number \"10xb\"\."
> +gdb_test "x/-a10xb main" "Invalid number \"10xb\"\."
> +gdb_test_no_output "x/-0i main"
> +gdb_test_no_output "x/-0sh main"
> +
> +# memory page boundary test (# of tests = 5)
> +
> +gdb_test_no_output "set print elements 0"
> +set boundary [get_first_mapped_address]

> +gdb_test "x/3s ${boundary}"

This always passes; it is on purpose?  I see many of those
in the file.  Also, this one puts a program address in a gdb.sum
test message, but program addresses are not stable, so this
call should get an explicit test message.

> +gdb_test_sequence "x/-4s" "memory page boundary test" {
> +    "Cannot access memory at address 0x"
> +    "0x"
> +    "0x"
> +    "0x"
> +}

Please also consider:

- Ports that don't support "info proc mappings", and thus
${boundary} is 0.  May need to skip parts of the test on those.

- Non-MMU ports that can actually access any address, including 0.
You can use is_address_zero_readable to detect these.

Speaking of address 0, it'd be nice to test that these
scenarios behave as intended:

 - examining backwards from address 0.
 - examining M bytes from address N, where M > N.

The idea being to catch potential wraparound bugs in gdb
resulting in huge memory allocations.

> +gdb_test "x/3s ${boundary}"

Ditto re. always passing and address in gdb.sum.

> +gdb_test_sequence "x/-3s" "memory page boundary test" {
> +    "Cannot access memory at address 0x"
> +    "0x"
> +    "0x"
> +    "0x"
> +}
> +
> +# char-width=1, print-max=20 (# of tests = 12)

Please drop the "# of tests" bits.  They'll tend to end up
stale as the testcase evolves in the future.

> +
> +gdb_test_no_output "set print elements 20"
> +gdb_test "x/6s &TestStrings"
> +gdb_test "x/-1xb" "0x39"
> +gdb_test_sequence "x/-6s" "examine multiple strings backward" {
> +    "\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +    "\"UVWXYZ\""
> +    "\"\""
> +    "\"\""
> +    "\"[^\"]+\""
> +    "\"01234567890123456789\"\.\.\."
> +}
> +gdb_test "x/6s &TestStrings"
> +gdb_test "x/-xb" "0x39"
> +gdb_test "x/-s" "\"01234567890123456789\"\.\.\."
> +gdb_test "x/-s" "\".+\""
> +gdb_test "x/-s" "\"\""
> +gdb_test "x/-s" "\"\""
> +gdb_test "x/-s" "\"GHIJKLMNOPQRSTUVWXYZ\""
> +gdb_test "x/-s" "\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +
> +# char-width=2, print-max=20 (# of tests = 12)
> +
> +gdb_test_no_output "set print elements 20"
> +gdb_test "x/6sh &TestStringsH"
> +gdb_test "x/-1xh" "0x0039"
> +gdb_test_sequence "x/-6sh" "examine multiple strings backward" {
> +    "u\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +    "u\"UVWXYZ\""
> +    "u\"\""
> +    "u\"\""
> +    "u\"[^\"]+\""
> +    "u\"01234567890123456789\"\.\.\."
> +}
> +gdb_test "x/6sh &TestStringsH"
> +gdb_test "x/-xh" "0x0039"
> +gdb_test "x/-sh" "u\"01234567890123456789\"\.\.\."
> +gdb_test "x/-sh" "u\".+\""
> +gdb_test "x/-sh" "u\"\""
> +gdb_test "x/-sh" "u\"\""
> +gdb_test "x/-sh" "u\"GHIJKLMNOPQRSTUVWXYZ\""
> +gdb_test "x/-sh" "u\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +
> +# char-width=4, print-max=20 (# of tests = 12)
> +
> +gdb_test_no_output "set print elements 20"
> +gdb_test "x/6sw &TestStringsW"
> +gdb_test "x/-1xw" "0x00000039"
> +gdb_test_sequence "x/-6sw" "examine multiple strings backward" {
> +    "U\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +    "U\"UVWXYZ\""
> +    "U\"\""
> +    "U\"\""
> +    "U\"[^\"]+\""
> +    "U\"01234567890123456789\"\.\.\."
> +}
> +gdb_test "x/6sw &TestStringsW"
> +gdb_test "x/-xw" "0x00000039"
> +gdb_test "x/-sw" "U\"01234567890123456789\"\.\.\."
> +gdb_test "x/-sw" "U\".+\""
> +gdb_test "x/-sw" "U\"\""
> +gdb_test "x/-sw" "U\"\""
> +gdb_test "x/-sw" "U\"GHIJKLMNOPQRSTUVWXYZ\""
> +gdb_test "x/-sw" "U\"ABCDEFGHIJKLMNOPQRST\"\.\.\."
> +
> +# char-width=2, print-max=0 (# of tests = 12)
> +
> +gdb_test_no_output "set print elements 0"
> +gdb_test "x/6sh &TestStringsH"
> +gdb_test "x/-4xh" "0x0021\[\t \]+0x0021\[\t \]+0x0021\[\t \]+0x0000"
> +gdb_test_sequence "x/-6sh" "examine multiple strings backward" {
> +    "u\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\""
> +    "u\"\""
> +    "u\"\""
> +    "u\"[^\"]+\""
> +    "u\"012345678901234567890123456789\""
> +    "u\"!!!!!!\""
> +}
> +gdb_test "x/6sh &TestStringsH"
> +gdb_test "x/-xh"
> +gdb_test "x/-sh" "u\"!!!!!!\""
> +gdb_test "x/-sh" "u\"012345678901234567890123456789\""
> +gdb_test "x/-sh" "u\".+\""
> +gdb_test "x/-sh" "u\"\""
> +gdb_test "x/-sh" "u\"\""
> +gdb_test "x/-sh" "u\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\""
> +
> +# char-width=1, print-max=4 (# of tests = 5)
> +
> +gdb_test_no_output "set print elements 4"
> +gdb_test "x/9s &TestStrings"
> +gdb_test "x/-xb" "0x00"
> +gdb_test_sequence "x/-4s" "examine multiple strings backward" {
> +    "\"TUVW\"\.\.\."
> +    "\"XYZ\""
> +    "\"\""
> +    "\"\""
> +}
> +gdb_test_sequence "x/-4s" "examine multiple strings backward" {
> +    "\"CDEF\"\.\.\."
> +    "\"GHIJ\"\.\.\."
> +    "\"KLMN\"\.\.\."
> +    "\"OPQR\"\.\.\."
> +}
> +
> +# Disassemle backward test (# of tests = 10)
> +
> +gdb_test_no_output "set print elements 200"
> +if { [istarget x86_64-*-* ] } {

Please use is_amd64_regs_target / is_x86_like_target.

Finally, the test has several non-unique test messages.  Take a look
at:

  https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Make_sure_test_messages_are_unique

You'll probably need something like:

with_test_prefix "char-width=4, print-max=20" {
  ...
}

etc.

Thanks,
Pedro Alves