Re: [PATCH v6 05/35] Hexagon (disas) disassembler

[Philippe Mathieu-Daudé]

Hi Taylor,

On 1/8/21 5:28 AM, Taylor Simpson wrote:
> Add hexagon to disas/meson.build
> Add disas/hexagon.c
> Add hexagon to include/disas/dis-asm.h
> 
> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> ---
>  include/disas/dis-asm.h |  1 +
>  disas/hexagon.c         | 68 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  disas/meson.build       |  1 +
>  3 files changed, 70 insertions(+)
>  create mode 100644 disas/hexagon.c
> 
> diff --git a/include/disas/dis-asm.h b/include/disas/dis-asm.h
> index 2164762..1f54fc0 100644
> --- a/include/disas/dis-asm.h
> +++ b/include/disas/dis-asm.h
> @@ -459,6 +459,7 @@ int print_insn_xtensa           (bfd_vma, 
> disassemble_info*);
>  int print_insn_riscv32          (bfd_vma, disassemble_info*);
>  int print_insn_riscv64          (bfd_vma, disassemble_info*);
>  int print_insn_rx(bfd_vma, disassemble_info *);
> +int print_insn_hexagon(bfd_vma, disassemble_info *);
>  
>  #ifdef CONFIG_CAPSTONE
>  bool cap_disas_target(disassemble_info *info, uint64_t pc, size_t size);
> diff --git a/disas/hexagon.c b/disas/hexagon.c
> new file mode 100644
> index 0000000..968f11c
> --- /dev/null
> +++ b/disas/hexagon.c
> @@ -0,0 +1,68 @@
> +/*
> + *  Copyright(c) 2019-2020 Qualcomm Innovation Center, Inc. All Rights 
> Reserved.

2019-2021 :)

> + *
> + *  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 2 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/>.

If possible please also include the SPDX identifier (or simply it):

SPDX-License-Identifier: GPL-2.0-or-later

> + */
> +
> +/*
> + * QEMU Hexagon Disassembler
> + */
> +
> +#include "qemu/osdep.h"
> +#include "disas/dis-asm.h"
> +#include "target/hexagon/cpu_bits.h"
> +
> +/*
> + * We will disassemble a packet with up to 4 instructions, so we need
> + * a hefty size buffer.
> + */
> +#define PACKET_BUFFER_LEN                   1028
> +
> +int print_insn_hexagon(bfd_vma memaddr, struct disassemble_info *info)
> +{
> +    uint32_t words[PACKET_WORDS_MAX];
> +    bool found_end = false;
> +    char buf[PACKET_BUFFER_LEN];
> +    int i;
> +
> +    for (i = 0; i < PACKET_WORDS_MAX && !found_end; i++) {
> +        int status = (*info->read_memory_func)(memaddr + i * 
> sizeof(uint32_t),
> +                                               (bfd_byte *)&words[i],
> +                                               sizeof(uint32_t), info);
> +        if (status) {
> +            if (i > 0) {
> +                break;
> +            }
> +            (*info->memory_error_func)(status, memaddr, info);
> +            return status;
> +        }
> +        if (is_packet_end(words[i])) {
> +            found_end = true;
> +        }
> +    }
> +
> +    if (!found_end) {
> +        (*info->fprintf_func)(info->stream, "<invalid>");
> +        return PACKET_WORDS_MAX * 4;

I suppose 4 is sizeof(uint32_t), right?

> +    }
> +
> +    int len = disassemble_hexagon(words, i, memaddr, buf, PACKET_BUFFER_LEN);
> +    int slen = strlen(buf);

Per QEMU's CODING_STYLE.rst:

  Declarations
  ============

  Mixed declarations (interleaving statements and declarations within
  blocks) are generally not allowed; declarations should be at the
  beginning of blocks.

> +    if (buf[slen - 1] == '\n') {
> +        buf[slen - 1] = '\0';
> +    }
> +    (*info->fprintf_func)(info->stream, "%s", buf);
> +
> +    return len;
> +}
> diff --git a/disas/meson.build b/disas/meson.build
> index 09a8527..b7b659b 100644
> --- a/disas/meson.build
> +++ b/disas/meson.build
> @@ -6,6 +6,7 @@ common_ss.add(when: 'CONFIG_ARM_A64_DIS', if_true: 
> files('arm-a64.cc'))
>  common_ss.add_all(when: 'CONFIG_ARM_A64_DIS', if_true: libvixl_ss)
>  common_ss.add(when: 'CONFIG_ARM_DIS', if_true: files('arm.c'))
>  common_ss.add(when: 'CONFIG_CRIS_DIS', if_true: files('cris.c'))
> +common_ss.add(when: 'CONFIG_HEXAGON_DIS', if_true: files('hexagon.c'))
>  common_ss.add(when: 'CONFIG_HPPA_DIS', if_true: files('hppa.c'))
>  common_ss.add(when: 'CONFIG_I386_DIS', if_true: files('i386.c'))
>  common_ss.add(when: 'CONFIG_LM32_DIS', if_true: files('lm32.c'))
> 

Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

With comments addressed:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>