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

Mike Frysinger
2017-02-14T18:52:23Z

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

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]