Re: RFA: AArch64 sim

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 4979 bytes --]

On 15 Jul 2015 17:58, Nick Clifton wrote:

it's looking pretty good.  comments inline.

> +++ include/gdb/sim-aarch64.h
>
> +/* sim-aarch64.h --- interface between AArch64  simulator and GDB.

only one space before "simulator"

> +#ifdef __cplusplus
> +extern "C" { // }
> +#endif

hmm, i see a few arches do this, but most don't.  is there any reason we should 
keep this ?  or should we scrub all targets to not do this ?

> +++ sim/aarch64/cpustate.c
>
> +#define reg_num(reg) ((reg == R31 && !r31_is_sp) ? 32 : reg)

prob doesn't really matter, but it should be (reg) in both places

> +  if (val != REG (sd)->gr[reg].u64)
> +    TRACE_REGISTER (STATE_CPU (sd, 0),

ideally you'd pass in the cpu instead of hardcoding CPU 0 everywhere via sd.  
you can get sd from the cpu via CPU_STATE().

> +/* Set the CPSR register as an int.  */
> +StatusCode
> +aarch64_set_CPSR (SIM_DESC sd, uint32_t new_flags)
> +{
> ...
> +    }	

should double check trailing whitespace

> +++ sim/aarch64/cpustate.h
>
> +#if __WORDSIZE == 64
> +#define PRINT_64 "l"
> +#else
> +#define PRINT_64 "ll"
> +#endif

you want inttypes.h and PRIi64/etc... and then delete PRINT_64 entirely

> +typedef enum
> +{
> +  STATUS_READY   = 0, /* May continue stepping or running.  */
> +  STATUS_RETURN  = 1, /* Via normal return from initial frame.  */
> +  STATUS_HALT    = 2, /* Via HALT pseudo-instruction.  */
> +  STATUS_BREAK   = 3, /* Via BRK instruction.  */
> +  STATUS_CALLOUT = 4, /* Via CALLOUT pseudo-instruction.  */
> +  STATUS_ERROR   = 5, /* Simulator detected problem.  */
> +  STATUS_MAX     = 6
> +} StatusCode;

a scan of the code indicates that most of this looks like you're setting state 
and then acting on it later yourself when you really should be calling 
sim_engine_halt directly.  any reason for doing it this way ?

> +#endif /* ifndef DECODE_H */

usually our comments don't include "ifndef" and such, just the macro name 
(which is _DECODE_H here i think)

> +++ sim/aarch64/interp.c
>
> +static char opbuf[1000];

should you have a sanity check on this to make sure it doesn't overflow ?

> +static int
> +op_printf (char *buf, char *fmt, ...)

should have a printf attribute on it

> +static int
> +sim_dis_read (bfd_vma                     memaddr,
> +	      bfd_byte *                  ptr,
> +	      unsigned int                length,
> +	      struct disassemble_info *   info)
> +{
> +  aarch64_get_mem_blk (NULL, memaddr, (char *) ptr, length);
> +  return 0;
> +}

shouldn't

> +compare_symbols (const void * ap, const void * bp)

no space after the *.  this comes up a few times in this patch.

> +sim_read (SIM_DESC sd, SIM_ADDR mem, unsigned char * buf, int length)
> +sim_write (SIM_DESC sd, SIM_ADDR mem, const unsigned char * buf, int length)

hmm, do you even need these anymore ?  now that you're using the sim core for 
memory handling, the common ones should just work.

> +++ sim/aarch64/Makefile.in
>
> +INCLUDE = cpustate.h memory.h decode.h

pretty sure this isn't needed so you can delete it

> +interp.o:    cpustate.h memory.h simulator.h sim-main.h
> +cpustate.o:  cpustate.h memory.h simulator.h sim-main.h
> +simulator.o: cpustate.h memory.h simulator.h sim-main.h
> +main.o:      cpustate.h memory.h simulator.h sim-main.h
> +memory.o:    cpustate.h memory.h simulator.h sim-main.h

you should be able to delete all these rules as they'll get automatically generated now

> +++ sim/aarch64/memory.c
>
> +static bfd * saved_prog = NULL;

this whole handling of saved_prog is so tortured.  it looks like you should 
change aarch64_init to accept the bfd directly from the sim func.

> +static inline void

> +StatusCode
> +aarch64_get_mem_blk (SIM_DESC sd, uint64_t address, char * buffer, unsigned length)
> +{
> +  char * addr = sim_core_trans_addr (sd, STATE_CPU (sd, 0), 0, address);

pretty sure map should be set to read_map instead of 0

> +
> +  if (addr == NULL)
> +    {
> +      memset (buffer, 0, length);
> +      if (sd)
> +	{
> +	  mem_error (sd, "read of non-existant mem block at", address);
> +	  return aarch64_set_status (sd, STATUS_ERROR, ERROR_EXCEPTION);
> +	}
> +      return STATUS_ERROR;
> +    }
> +
> +  memcpy (buffer, addr, length);
> +  return STATUS_READY;
> +}

seems like this whole func should be replaced with sim_core_read_buffer.

> +/* Accessor macro to obtain the aarch64 sim registers.  */
> +#define REG(sd)   STATE_CPU ((sd), 0)

would really really prefer the cpu get passed down into funcs instead of 
hardcoding 0

> --- /dev/null
> +++ sim/aarch64/simulator.c
>
> +  return aarch64_set_reg_u64 (sd, rd, NO_SP, aarch64_get_mem_u32 (sd, aarch64_get_PC (sd) + offset * 4));

there's a few lines in this file that need wrapping

largely scanned the rest as i'm not familiar with the aarch64 isa and don't 
have time to learn atm ;)
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]