Re: CGEN: RFA: CGEN_INT_INSN_P

[Dave Brolley <brolley at redhat dot com>]

Approved offline by Frank Eigler and Doug Evans -- commited.

Dave

Dave Brolley wrote:

> Hi,
>
> I ran into a problem with the cgen-based gas/opcodes/binutils/sim
> port that I am working on. The ISA has some 16 bit insns and some
> 32 bit insns, so I set default-insn-word-bitsize=16,
> default-insn-bitsize=16 and base-insn-bitsize=16.
>
> Now in <arch>-desc.h, CGEN_INT_INSN_P gets define to 1, since all
> of my insns are 32 bit or less. However, the current code enabled
> by CGEN_INT_INSN_P in cgen-ibld.in and cgen-dis.in does not allow
> for base-insn-bitsize != max-insn-bitsize.
>
> 1) It aborts in 'extract_normal' because when accessing a 16 bit
> insn field at offset 16 it is called with word_offset==16. Also
> start==0, length==16, word_length==16 and total_length==32. These
> value are generated by cgen in <arch>_cgen_insert_operand and I
> believe that they are correct.
>
> 2) Even if the abort did not occur the generated insn was wrong
> because the insns base value was not correctly onserted into the
> insn integer by insert_insn_normal.
>
> 3) A similar problem to 2) exists in print_insn
>
> This patch allows for CGEN_INT_INSN_P==1 with base-insn-bitsize
> != max-insn-bitsize. I have tested it against my correct work
> (which has this configuration) and against the fr30 (which has
> CGEN_INT_INSN_P==0) and against another port for which
> CGEN_INT_INSN_P==1 and base-insn-bitsize == max-insn-bitsize.
>
> OK to commit? ---- or was there a much easier way to handle
> this?  :-)
>
> Dave
>
>   ------------------------------------------------------------------------
> 2000-08-22  Dave Brolley  <brolley@redhat.com>
>
>         * cgen-ibld.in (cgen_put_insn_int_value): New function.
>         (insert_normal): Allow for non-zero word_offset with CGEN_INT_INSN_P.
>         (insert_insn_normal): Use cgen_put_insn_int_value with CGEN_INT_INSN_P.
>         (extract_normal): Allow for non-zero word_offset with CGEN_INT_INSN_P.
>         * cgen-dis.in (read_insn): New statis function.
>         (print_insn): Use read_insn to read the insn into the buffer and set
>         up for disassembly.
>         (print_insn): in CGEN_INT_INSN_P, make sure that the entire insn is
>         in the buffer.
>
>   ------------------------------------------------------------------------
> Index: opcodes/cgen-dis.in
> ===================================================================
> RCS file: /cvs/src/src/opcodes/cgen-dis.in,v
> retrieving revision 1.1
> diff -c -p -r1.1 cgen-dis.in
> *** cgen-dis.in 2000/08/04 02:21:43     1.1
> --- cgen-dis.in 2000/08/22 19:59:17
> *************** print_insn_normal (cd, dis_info, insn, f
> *** 187,229 ****
>       }
>   }
>
> ! /* Utility to print an insn.
> !    BUF is the base part of the insn, target byte order, BUFLEN bytes long.
> !    The result is the size of the insn in bytes or zero for an unknown insn
> !    or -1 if an error occurs fetching data (memory_error_func will have
> !    been called).  */
> !
>   static int
> ! print_insn (cd, pc, info, buf, buflen)
>        CGEN_CPU_DESC cd;
>        bfd_vma pc;
>        disassemble_info *info;
>        char *buf;
>        int buflen;
>   {
> !   unsigned long insn_value;
> !   const CGEN_INSN_LIST *insn_list;
> !   CGEN_EXTRACT_INFO ex_info;
>
> !   ex_info.dis_info = info;
> !   ex_info.valid = (1 << (cd->base_insn_bitsize / 8)) - 1;
> !   ex_info.insn_bytes = buf;
>
>     switch (buflen)
>       {
>       case 1:
> !       insn_value = buf[0];
>         break;
>       case 2:
> !       insn_value = info->endian == BFD_ENDIAN_BIG ? bfd_getb16 (buf) : bfd_getl16 (buf);
>         break;
>       case 4:
> !       insn_value = info->endian == BFD_ENDIAN_BIG ? bfd_getb32 (buf) : bfd_getl32 (buf);
>         break;
>       default:
>         abort ();
>       }
>
>     /* The instructions are stored in hash lists.
>        Pick the first one and keep trying until we find the right one.  */
>
> --- 187,256 ----
>       }
>   }
>
> ! /* Subroutine of print_insn. Reads an insn into the given buffers and updates
> !    the extract info.
> !    Returns 0 if all is well, non-zero otherwise.  */
>   static int
> ! read_insn (cd, pc, info, buf, buflen, ex_info, insn_value)
>        CGEN_CPU_DESC cd;
>        bfd_vma pc;
>        disassemble_info *info;
>        char *buf;
>        int buflen;
> +      CGEN_EXTRACT_INFO *ex_info;
> +      unsigned long *insn_value;
>   {
> !   int status = (*info->read_memory_func) (pc, buf, buflen, info);
> !   if (status != 0)
> !     {
> !       (*info->memory_error_func) (status, pc, info);
> !       return -1;
> !     }
>
> !   ex_info->dis_info = info;
> !   ex_info->valid = (1 << buflen) - 1;
> !   ex_info->insn_bytes = buf;
>
>     switch (buflen)
>       {
>       case 1:
> !       *insn_value = buf[0];
>         break;
>       case 2:
> !       *insn_value = info->endian == BFD_ENDIAN_BIG ? bfd_getb16 (buf) : bfd_getl16 (buf);
>         break;
>       case 4:
> !       *insn_value = info->endian == BFD_ENDIAN_BIG ? bfd_getb32 (buf) : bfd_getl32 (buf);
>         break;
>       default:
>         abort ();
>       }
>
> +   return 0;
> + }
> +
> + /* Utility to print an insn.
> +    BUF is the base part of the insn, target byte order, BUFLEN bytes long.
> +    The result is the size of the insn in bytes or zero for an unknown insn
> +    or -1 if an error occurs fetching data (memory_error_func will have
> +    been called).  */
> +
> + static int
> + print_insn (cd, pc, info, buf, buflen)
> +      CGEN_CPU_DESC cd;
> +      bfd_vma pc;
> +      disassemble_info *info;
> +      char *buf;
> +      int buflen;
> + {
> +   unsigned long insn_value;
> +   const CGEN_INSN_LIST *insn_list;
> +   CGEN_EXTRACT_INFO ex_info;
> +
> +   int rc = read_insn (cd, pc, info, buf, buflen, & ex_info, & insn_value);
> +   if (rc != 0)
> +     return rc;
> +
>     /* The instructions are stored in hash lists.
>        Pick the first one and keep trying until we find the right one.  */
>
> *************** print_insn (cd, pc, info, buf, buflen)
> *** 253,258 ****
> --- 280,301 ----
>           /* Printing is handled in two passes.  The first pass parses the
>              machine insn and extracts the fields.  The second pass prints
>              them.  */
> +
> + #if CGEN_INT_INSN_P
> +         /* Make sure the entire insn is loaded into insn_value.  */
> +         if (CGEN_INSN_BITSIZE (insn) > cd->base_insn_bitsize)
> +           {
> +             unsigned long full_insn_value;
> +             int rc = read_insn (cd, pc, info, buf,
> +                                 CGEN_INSN_BITSIZE (insn) / 8,
> +                                 & ex_info, & full_insn_value);
> +             if (rc != 0)
> +               return rc;
> +             length = CGEN_EXTRACT_FN (cd, insn)
> +               (cd, insn, &ex_info, full_insn_value, &fields, pc);
> +           }
> +         else
> + #endif
>
>           length = CGEN_EXTRACT_FN (cd, insn)
>             (cd, insn, &ex_info, insn_value, &fields, pc);
> Index: opcodes/cgen-ibld.in
> ===================================================================
> RCS file: /cvs/src/src/opcodes/cgen-ibld.in,v
> retrieving revision 1.1
> diff -c -p -r1.1 cgen-ibld.in
> *** cgen-ibld.in        2000/08/04 02:21:43     1.1
> --- cgen-ibld.in        2000/08/22 19:59:17
> *************** static int extract_normal
> *** 57,62 ****
> --- 57,65 ----
>   static int extract_insn_normal
>        PARAMS ((CGEN_CPU_DESC, const CGEN_INSN *, CGEN_EXTRACT_INFO *,
>               CGEN_INSN_INT, CGEN_FIELDS *, bfd_vma));
> + static void cgen_put_insn_int_value
> +      PARAMS ((CGEN_CPU_DESC, CGEN_INSN_BYTES_PTR, int, int, CGEN_INSN_INT));
> +
>
>   /* Operand insertion.  */
>
> *************** insert_normal (cd, value, attrs, word_of
> *** 183,191 ****
> --- 186,196 ----
>     if (length == 0)
>       return NULL;
>
> + #if 0
>     if (CGEN_INT_INSN_P
>         && word_offset != 0)
>       abort ();
> + #endif
>
>     if (word_length > 32)
>       abort ();
> *************** insert_normal (cd, value, attrs, word_of
> *** 237,245 ****
>       int shift;
>
>       if (CGEN_INSN_LSB0_P)
> !       shift = (start + 1) - length;
>       else
> !       shift = word_length - (start + length);
>       *buffer = (*buffer & ~(mask << shift)) | ((value & mask) << shift);
>     }
>
> --- 242,250 ----
>       int shift;
>
>       if (CGEN_INSN_LSB0_P)
> !       shift = (word_offset + start + 1) - length;
>       else
> !       shift = total_length - (word_offset + start + length);
>       *buffer = (*buffer & ~(mask << shift)) | ((value & mask) << shift);
>     }
>
> *************** insert_insn_normal (cd, insn, fields, bu
> *** 283,289 ****
>
>   #if CGEN_INT_INSN_P
>
> !   *buffer = value;
>
>   #else
>
> --- 288,295 ----
>
>   #if CGEN_INT_INSN_P
>
> !   cgen_put_insn_int_value (cd, buffer, cd->base_insn_bitsize,
> !                          CGEN_FIELDS_BITSIZE (fields), value);
>
>   #else
>
> *************** insert_insn_normal (cd, insn, fields, bu
> *** 313,318 ****
> --- 319,341 ----
>
>     return NULL;
>   }
> +
> + /* Cover function to store an insn value into an integral insn.  Must go here
> +  because it needs <prefix>-desc.h for CGEN_INT_INSN_P.  */
> +
> + void
> + cgen_put_insn_int_value (cd, buf, length, insn_length, value)
> +      CGEN_CPU_DESC cd;
> +      CGEN_INSN_BYTES_PTR buf;
> +      int length;
> +      int insn_length;
> +      CGEN_INSN_INT value;
> + {
> +   int shift = insn_length - length;
> +   /* Written this way to avoid undefined behaviour.  */
> +   CGEN_INSN_INT mask = (((1L << (length - 1)) - 1) << 1) | 1;
> +   *buf = (*buf & ~(mask << shift)) | ((value & mask) << shift);
> + }
>
>   /* Operand extraction.  */
>
> *************** extract_normal (cd, ex_info, insn_value,
> *** 469,477 ****
> --- 492,502 ----
>         return 1;
>       }
>
> + #if 0
>     if (CGEN_INT_INSN_P
>         && word_offset != 0)
>       abort ();
> + #endif
>
>     if (word_length > 32)
>       abort ();
> *************** extract_normal (cd, ex_info, insn_value,
> *** 487,501 ****
>
>     /* Does the value reside in INSN_VALUE?  */
>
> !   if (word_offset == 0)
>       {
>         /* Written this way to avoid undefined behaviour.  */
>         CGEN_INSN_INT mask = (((1L << (length - 1)) - 1) << 1) | 1;
>
>         if (CGEN_INSN_LSB0_P)
> !       value = insn_value >> ((start + 1) - length);
>         else
> !       value = insn_value >> (word_length - (start + length));
>         value &= mask;
>         /* sign extend? */
>         if (CGEN_BOOL_ATTR (attrs, CGEN_IFLD_SIGNED)
> --- 512,526 ----
>
>     /* Does the value reside in INSN_VALUE?  */
>
> !   if (CGEN_INT_INSN_P || word_offset == 0)
>       {
>         /* Written this way to avoid undefined behaviour.  */
>         CGEN_INSN_INT mask = (((1L << (length - 1)) - 1) << 1) | 1;
>
>         if (CGEN_INSN_LSB0_P)
> !       value = insn_value >> ((word_offset + start + 1) - length);
>         else
> !       value = insn_value >> (total_length - ( word_offset + start + length));
>         value &= mask;
>         /* sign extend? */
>         if (CGEN_BOOL_ATTR (attrs, CGEN_IFLD_SIGNED)