Re: [PATCH] target_read_memory&co: no longer return target_xfer_status

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 27 October 2015 at 10:44, Pedro Alves <palves@redhat.com> wrote:
> Years ago, these functions used to return errno/EIO.  Later, through a
> series of changes that intended to remove native/remote differences,
> they ended up returning a target_xfer_status in disguise.
>
> Unlike target_xfer_partial&co, the point of target_read_memory&co is
> to either fully succeed or fail.  On error, they always return
> TARGET_XFER_E_IO.  So there's no real point in casting the return of
> target_read_memory to a target_xfer_status to pass it to memory_error.
> Instead, it results in clearer code to simply decouple
> target_read_memory&co's return from target_xfer_status.
>
> This fixes build errors like this in C++ mode:
>
>  ../../src/gdb/corefile.c: In function âvoid read_stack(CORE_ADDR, gdb_byte*, ssize_t)â:
>  ../../src/gdb/corefile.c:276:34: error: invalid conversion from âintâ to âtarget_xfer_statusâ [-fpermissive]
>       memory_error (status, memaddr);
>                                    ^
>  ../../src/gdb/corefile.c:216:1: error:   initializing argument 1 of âvoid memory_error(target_xfer_status, CORE_ADDR)â [-fpermissive]
>
> gdb/ChangeLog:
> 2015-10-27  Pedro Alves  <palves@redhat.com>
>
>         * alpha-tdep.c (alpha_read_insn): Always pass TARGET_XFER_E_IO to
>         memory_error.  Rename local 'status' to 'res'.
>         * c-lang.c (c_get_string): Always pass TARGET_XFER_E_IO to
>         memory_error.
>         * corefile.c (read_stack, read_code, write_memory): Always pass
>         TARGET_XFER_E_IO to memory_error.
>         * disasm.c (dis_asm_memory_error): Always pass TARGET_XFER_E_IO to
>         memory_error.  Rename parameter 'status' to 'err'.
>         (dump_insns): Rename local 'status' to 'err'.
>         * mips-tdep.c (mips_fetch_instruction): Rename parameter 'statusp'
>         to 'errp'.  Rename local 'status' to 'err'.  Always pass
>         TARGET_XFER_E_IO to memory_error.
>         (mips_breakpoint_from_pc): Rename local 'status' to 'err'.
>         * target.c (target_read_memory, target_read_raw_memory)
>         (target_read_stack, target_read_code, target_write_memory)
>         (target_write_raw_memory): Return -1 on error instead of
>         TARGET_XFER_E_IO.
>         * valprint.c (val_print_string): Rename local 'errcode' to 'err'.
>         Always pass TARGET_XFER_E_IO to memory_error.  Update comment.
> ---
>  gdb/alpha-tdep.c |  8 ++++----
>  gdb/c-lang.c     |  4 ++--
>  gdb/corefile.c   |  6 +++---
>  gdb/disasm.c     | 12 ++++++------
>  gdb/mips-tdep.c  | 25 +++++++++++++------------
>  gdb/target.c     | 30 +++++++++++++++---------------
>  gdb/valprint.c   | 14 +++++++-------
>  7 files changed, 50 insertions(+), 49 deletions(-)
>
> diff --git a/gdb/alpha-tdep.c b/gdb/alpha-tdep.c
> index 0696b6e..ca0e109 100644
> --- a/gdb/alpha-tdep.c
> +++ b/gdb/alpha-tdep.c
> @@ -682,11 +682,11 @@ alpha_read_insn (struct gdbarch *gdbarch, CORE_ADDR pc)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    gdb_byte buf[ALPHA_INSN_SIZE];
> -  int status;
> +  int res;
>
> -  status = target_read_memory (pc, buf, sizeof (buf));
> -  if (status)
> -    memory_error (status, pc);
> +  res = target_read_memory (pc, buf, sizeof (buf));
> +  if (res != 0)
> +    memory_error (TARGET_XFER_E_IO, pc);
>    return extract_unsigned_integer (buf, sizeof (buf), byte_order);
>  }
>
> diff --git a/gdb/c-lang.c b/gdb/c-lang.c
> index 6731b43..384783c 100644
> --- a/gdb/c-lang.c
> +++ b/gdb/c-lang.c
> @@ -327,10 +327,10 @@ c_get_string (struct value *value, gdb_byte **buffer,
>
>        err = read_string (addr, *length, width, fetchlimit,
>                          byte_order, buffer, length);
> -      if (err)
> +      if (err != 0)
>         {
>           xfree (*buffer);
> -         memory_error (err, addr);
> +         memory_error (TARGET_XFER_E_IO, addr);
>         }
>      }
>
> diff --git a/gdb/corefile.c b/gdb/corefile.c
> index 31301cf..596da33 100644
> --- a/gdb/corefile.c
> +++ b/gdb/corefile.c
> @@ -273,7 +273,7 @@ read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>
>    status = target_read_stack (memaddr, myaddr, len);
>    if (status != 0)
> -    memory_error (status, memaddr);
> +    memory_error (TARGET_XFER_E_IO, memaddr);
>  }
>
>  /* Same as target_read_code, but report an error if can't read.  */
> @@ -285,7 +285,7 @@ read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>
>    status = target_read_code (memaddr, myaddr, len);
>    if (status != 0)
> -    memory_error (status, memaddr);
> +    memory_error (TARGET_XFER_E_IO, memaddr);
>  }
>
>  /* Read memory at MEMADDR of length LEN and put the contents in
> @@ -392,7 +392,7 @@ write_memory (CORE_ADDR memaddr,
>
>    status = target_write_memory (memaddr, myaddr, len);
>    if (status != 0)
> -    memory_error (status, memaddr);
> +    memory_error (TARGET_XFER_E_IO, memaddr);
>  }
>
>  /* Same as write_memory, but notify 'memory_changed' observers.  */
> diff --git a/gdb/disasm.c b/gdb/disasm.c
> index 6e3d6c1..c17574e 100644
> --- a/gdb/disasm.c
> +++ b/gdb/disasm.c
> @@ -129,10 +129,10 @@ dis_asm_read_memory (bfd_vma memaddr, gdb_byte *myaddr, unsigned int len,
>
>  /* Like memory_error with slightly different parameters.  */
>  static void
> -dis_asm_memory_error (int status, bfd_vma memaddr,
> +dis_asm_memory_error (int err, bfd_vma memaddr,
>                       struct disassemble_info *info)
>  {
> -  memory_error (status, memaddr);
> +  memory_error (TARGET_XFER_E_IO, memaddr);
>  }
>
>  /* Like print_address with slightly different parameters.  */
> @@ -230,7 +230,7 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>          {
>            CORE_ADDR old_pc = pc;
>            bfd_byte data;
> -          int status;
> +          int err;
>            const char *spacer = "";
>
>            /* Build the opcodes using a temporary stream so we can
> @@ -242,9 +242,9 @@ dump_insns (struct gdbarch *gdbarch, struct ui_out *uiout,
>            pc += gdbarch_print_insn (gdbarch, pc, di);
>            for (;old_pc < pc; old_pc++)
>              {
> -              status = (*di->read_memory_func) (old_pc, &data, 1, di);
> -              if (status != 0)
> -                (*di->memory_error_func) (status, old_pc, di);
> +              err = (*di->read_memory_func) (old_pc, &data, 1, di);
> +              if (err != 0)
> +                (*di->memory_error_func) (err, old_pc, di);
>                fprintf_filtered (opcode_stream, "%s%02x",
>                                  spacer, (unsigned) data);
>                spacer = " ";
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 7cea832..6b5a1bd 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -1429,12 +1429,12 @@ mips_write_pc (struct regcache *regcache, CORE_ADDR pc)
>
>  static ULONGEST
>  mips_fetch_instruction (struct gdbarch *gdbarch,
> -                       enum mips_isa isa, CORE_ADDR addr, int *statusp)
> +                       enum mips_isa isa, CORE_ADDR addr, int *errp)
>  {
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    gdb_byte buf[MIPS_INSN32_SIZE];
>    int instlen;
> -  int status;
> +  int err;
>
>    switch (isa)
>      {
> @@ -1450,13 +1450,13 @@ mips_fetch_instruction (struct gdbarch *gdbarch,
>        internal_error (__FILE__, __LINE__, _("invalid ISA"));
>        break;
>      }
> -  status = target_read_memory (addr, buf, instlen);
> -  if (statusp != NULL)
> -    *statusp = status;
> -  if (status)
> +  err = target_read_memory (addr, buf, instlen);
> +  if (errp != NULL)
> +    *errp = err;
> +  if (err != 0)
>      {
> -      if (statusp == NULL)
> -       memory_error (status, addr);
> +      if (errp == NULL)
> +       memory_error (TARGET_XFER_E_IO, addr);
>        return 0;
>      }
>    return extract_unsigned_integer (buf, instlen, byte_order);
> @@ -7118,12 +7118,13 @@ mips_breakpoint_from_pc (struct gdbarch *gdbarch,
>           static gdb_byte micromips16_big_breakpoint[] = { 0x46, 0x85 };
>           static gdb_byte micromips32_big_breakpoint[] = { 0, 0x5, 0, 0x7 };
>           ULONGEST insn;
> -         int status;
> +         int err;
>           int size;
>
> -         insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &status);
> -         size = status ? 2
> -                       : mips_insn_size (ISA_MICROMIPS, insn) == 2 ? 2 : 4;
> +         insn = mips_fetch_instruction (gdbarch, ISA_MICROMIPS, pc, &err);
> +         size = (err != 0
> +                 ? 2 : (mips_insn_size (ISA_MICROMIPS, insn) == 2
> +                        ? 2 : 4));
>           *pcptr = unmake_compact_addr (pc);
>           *lenptr = size;
>           return (size == 2) ? micromips16_big_breakpoint
> diff --git a/gdb/target.c b/gdb/target.c
> index d7653c4..7ad2330 100644
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -1380,7 +1380,7 @@ target_xfer_partial (struct target_ops *ops,
>
>  /* Read LEN bytes of target memory at address MEMADDR, placing the
>     results in GDB's memory at MYADDR.  Returns either 0 for success or
> -   TARGET_XFER_E_IO if any error occurs.
> +   -1 if any error occurs.
>
>     If an error occurs, no guarantee is made about the contents of the data at
>     MYADDR.  In particular, the caller should not depend upon partial reads
> @@ -1399,7 +1399,7 @@ target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>                    myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* See target/target.h.  */
> @@ -1431,7 +1431,7 @@ target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>                    myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* Like target_read_memory, but specify explicitly that this is a read from
> @@ -1446,7 +1446,7 @@ target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>                    myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* Like target_read_memory, but specify explicitly that this is a read from
> @@ -1461,14 +1461,14 @@ target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
>                    myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* Write LEN bytes from MYADDR to target memory at address MEMADDR.
> -   Returns either 0 for success or TARGET_XFER_E_IO if any
> -   error occurs.  If an error occurs, no guarantee is made about how
> -   much data got written.  Callers that can deal with partial writes
> -   should call target_write.  */
> +   Returns either 0 for success or -1 if any error occurs.  If an
> +   error occurs, no guarantee is made about how much data got written.
> +   Callers that can deal with partial writes should call
> +   target_write.  */
>
>  int
>  target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
> @@ -1479,14 +1479,14 @@ target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
>                     myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* Write LEN bytes from MYADDR to target raw memory at address
> -   MEMADDR.  Returns either 0 for success or TARGET_XFER_E_IO
> -   if any error occurs.  If an error occurs, no guarantee is made
> -   about how much data got written.  Callers that can deal with
> -   partial writes should call target_write.  */
> +   MEMADDR.  Returns either 0 for success or -1 if any error occurs.
> +   If an error occurs, no guarantee is made about how much data got
> +   written.  Callers that can deal with partial writes should call
> +   target_write.  */
>
>  int
>  target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
> @@ -1497,7 +1497,7 @@ target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
>                     myaddr, memaddr, len) == len)
>      return 0;
>    else
> -    return TARGET_XFER_E_IO;
> +    return -1;
>  }
>
>  /* Fetch the target's memory map.  */
> diff --git a/gdb/valprint.c b/gdb/valprint.c
> index 7e74856..7f891c9 100644
> --- a/gdb/valprint.c
> +++ b/gdb/valprint.c
> @@ -2737,7 +2737,7 @@ val_print_string (struct type *elttype, const char *encoding,
>                   const struct value_print_options *options)
>  {
>    int force_ellipsis = 0;      /* Force ellipsis to be printed if nonzero.  */
> -  int errcode;                 /* Errno returned from bad reads.  */
> +  int err;                     /* Non-zero if we got a bad read.  */
>    int found_nul;               /* Non-zero if we found the nul char.  */
>    unsigned int fetchlimit;     /* Maximum number of chars to print.  */
>    int bytes_read;
> @@ -2758,8 +2758,8 @@ val_print_string (struct type *elttype, const char *encoding,
>    fetchlimit = (len == -1 ? options->print_max : min (len,
>                                                       options->print_max));
>
> -  errcode = read_string (addr, len, width, fetchlimit, byte_order,
> -                        &buffer, &bytes_read);
> +  err = read_string (addr, len, width, fetchlimit, byte_order,
> +                    &buffer, &bytes_read);
>    old_chain = make_cleanup (xfree, buffer);
>
>    addr += bytes_read;
> @@ -2787,7 +2787,7 @@ val_print_string (struct type *elttype, const char *encoding,
>           && extract_unsigned_integer (peekbuf, width, byte_order) != 0)
>         force_ellipsis = 1;
>      }
> -  else if ((len >= 0 && errcode != 0) || (len > bytes_read / width))
> +  else if ((len >= 0 && err != 0) || (len > bytes_read / width))
>      {
>        /* Getting an error when we have a requested length, or fetching less
>           than the number of characters actually requested, always make us
> @@ -2798,17 +2798,17 @@ val_print_string (struct type *elttype, const char *encoding,
>    /* If we get an error before fetching anything, don't print a string.
>       But if we fetch something and then get an error, print the string
>       and then the error message.  */
> -  if (errcode == 0 || bytes_read > 0)
> +  if (err == 0 || bytes_read > 0)
>      {
>        LA_PRINT_STRING (stream, elttype, buffer, bytes_read / width,
>                        encoding, force_ellipsis, options);
>      }
>
> -  if (errcode != 0)
> +  if (err != 0)
>      {
>        char *str;
>
> -      str = memory_error_message (errcode, gdbarch, addr);
> +      str = memory_error_message (TARGET_XFER_E_IO, gdbarch, addr);
>        make_cleanup (xfree, str);
>
>        fprintf_filtered (stream, "<error: ");
> --
> 1.9.3
>

Looks good to me.