Re: [PATCH v2 4/6] sim: or1k: add or1k target to sim

[Mike Frysinger <vapier at gentoo dot org>]

On 21 Jan 2017 12:03, Stafford Horne wrote:
> --- /dev/null
> +++ b/sim/or1k/Makefile.in
>
> +# Extra headers included by sim-main.h.

you shouldn't need to do any manual dep management.  i'd suggest
deleting all this logic and seeing if things still work.

> --- /dev/null
> +++ b/sim/or1k/configure.ac
> @@ -0,0 +1,41 @@
> +dnl Process this file with autoconf to produce a configure script.
> +AC_PREREQ(2.64)dnl
> +AC_INIT(Makefile.in)
> +sinclude(../common/acinclude.m4)
> +
> +  case "${target_alias}" in

drop the indentation

> +  or1k-linux*|or1knd-linux*)
> +    traps_obj=traps32-linux.o
> +    ;;
> +  or1k-*|or1knd-*)
> +    traps_obj=traps32.o
> +    ;;
> +  esac

we really don't like to see this kind of logic.  can't you do a single
build and then select the right details at runtime ?

--- /dev/null
> +++ b/sim/or1k/or1k.h
> @@ -0,0 +1,38 @@
> +#ifndef OR1K_H
> +#define OR1K_H

all files should have a header with copyright & license info

also, please use namespaced defines like SIM_OR1K_H

> --- /dev/null
> +++ b/sim/or1k/sim-if.c
> @@ -0,0 +1,318 @@
> +/* Main simulator entry points specific to the OR1K.
> +   Copyright (C) 1996-1999, 2003, 2007-2012 Free Software Foundation,
> +   Inc.
> +   Contributed by Cygnus Support.

looks like you need to double check where you copied & pasted things from.
cygnus isn't doing this port ;).

> +SIM_DESC
> +sim_open (kind, callback, abfd, argv)
> +     SIM_OPEN_KIND kind;
> +     host_callback *callback;
> +     struct bfd *abfd;
> +     char * const *argv;

you need to update all your prototypes.  we don't do old style ones like
this anymore.

> +#ifdef HAVE_DV_SOCKSER /* FIXME: was done differently before */
> +  if (dv_sockser_install (sd) != SIM_RC_OK)
> +    {
> +      free_state (sd);
> +      return 0;
> +    }
> +#endif

delete this.  common code handles it for you now.

> +  /* Allocate core managed memory if none specified by user.
> +     Use address 4 here in case the user wanted address 0 unmapped.  */
> +  if (sim_core_read_buffer (sd, NULL, read_map, &c, 4, 1) == 0) {
> +    sim_do_commandf (sd, "memory region 0,0x%x", OR1K_DEFAULT_MEM_SIZE);
> +  }

this isn't using GNU style for the braces/if body.  this comes up
multiple times in this patch, so please fix them all.

> +  /* check for/establish the reference program image */

this comment isn't using GNU style.  this comes up
multiple times in this patch, so please fix them all.

> +
> +  for (c = 0; c < MAX_NR_PROCESSORS; ++c)
> +    {
> +      SIM_CPU *cpu = STATE_CPU (sd, i);
> +      /* Only needed for profiling, but the structure member is small.  */
> +      memset (CPU_OR1K_MISC_PROFILE (cpu), 0,
> +	      sizeof (* CPU_OR1K_MISC_PROFILE (cpu)));
> +
> +#ifdef WANT_CPU_OR1K32BF
> +      or1k32bf_h_spr_set_raw(cpu, SPR_ADDR(SYS,VR),      or1k_vr);
> +      or1k32bf_h_spr_set_raw(cpu, SPR_ADDR(SYS,UPR),     or1k_upr);
> +      or1k32bf_h_spr_set_raw(cpu, SPR_ADDR(SYS,CPUCFGR), or1k_cpucfgr);
> +
> +      or1k32bf_cpu_init (sd, cpu);
> +#endif
> +    }

this loop has to call these funcs:
	CPU_REG_FETCH
	CPU_REG_STORE
	CPU_PC_FETCH
	CPU_PC_STORE

> +  /* Store in a global so things like sparc32_dump_regs can be invoked
> +     from the gdb command line.  */
> +  current_state = sd;

you must kill current_state.  we do not allow global state anymore.

> --- /dev/null
> +++ b/sim/or1k/sim-main.h
>
> +typedef IAI sim_cia;
> +#define CIA_GET(cpu) CPU_PC_GET (cpu)
> +#define CIA_SET(cpu,val) CPU_PC_SET ((cpu),(val))

these defines are dead -> delete

> +typedef struct _sim_cpu SIM_CPU;

delete this

> +#ifdef OR1K_LINUX

nothing defines this -> delete

> +#if (WITH_SMP)
> +#define STATE_CPU(sd, n) ((sd)->cpu[n])
> +#else
> +#define STATE_CPU(sd, n) ((sd)->cpu[0])
> +#endif

delete these -- common code does it for you now
-mike

Attachment: signature.asc
Description: Digital signature