[PATCH 3/3] sim: msp430: start a test framework

[PATCH 3/3] sim: msp430: start a test framework

[Mike Frysinger]

The current sim lacks any sort of tests.  Start a basic framework and
add a simple one to test the add insn.

sim/:
2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* configure.tgt (msp430*-*-*): Set sim_testsuite to yes.
	* configure: Regenerate.

sim/testsuite/:
2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* configure: Regenerate.

sim/testsuite/sim/msp430/:
2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* add.s, allinsn.exp, testutils.inc: New files.
---
 sim/configure                          |  1 +
 sim/configure.tgt                      |  1 +
 sim/testsuite/configure                |  1 +
 sim/testsuite/sim/msp430/add.s         | 20 ++++++++++
 sim/testsuite/sim/msp430/allinsn.exp   | 15 ++++++++
 sim/testsuite/sim/msp430/testutils.inc | 70 ++++++++++++++++++++++++++++++++++
 6 files changed, 108 insertions(+)
 create mode 100644 sim/testsuite/sim/msp430/add.s
 create mode 100644 sim/testsuite/sim/msp430/allinsn.exp
 create mode 100644 sim/testsuite/sim/msp430/testutils.inc

diff --git a/sim/configure b/sim/configure
index ab98231b..36d356e 100755
--- a/sim/configure
+++ b/sim/configure
@@ -3775,6 +3775,7 @@ subdirs="$subdirs arm"
   subdirs="$subdirs msp430"
 
 
+       sim_testsuite=yes
        ;;
    rl78-*-*)
 
diff --git a/sim/configure.tgt b/sim/configure.tgt
index 39f92b6..d112e72 100644
--- a/sim/configure.tgt
+++ b/sim/configure.tgt
@@ -88,6 +88,7 @@ case "${target}" in
        ;;
    msp430*-*-*)
        SIM_ARCH(msp430)
+       sim_testsuite=yes
        ;;
    rl78-*-*)
        SIM_ARCH(rl78)
diff --git a/sim/testsuite/configure b/sim/testsuite/configure
index af18624..f90bd47 100755
--- a/sim/testsuite/configure
+++ b/sim/testsuite/configure
@@ -1895,6 +1895,7 @@ case "${target}" in
        ;;
    msp430*-*-*)
        sim_arch=msp430
+       sim_testsuite=yes
        ;;
    rl78-*-*)
        sim_arch=rl78
diff --git a/sim/testsuite/sim/msp430/add.s b/sim/testsuite/sim/msp430/add.s
new file mode 100644
index 0000000..76247ed
--- /dev/null
+++ b/sim/testsuite/sim/msp430/add.s
@@ -0,0 +1,20 @@
+# check that basic add insn works.
+# mach: msp430
+
+.include "testutils.inc"
+
+	start
+
+	mov #10, r4
+	add #23, r4
+	cmp #33, r4
+	jne 1f
+
+	cmp #32, r4
+	jlo 1f
+
+	cmp #34, r4
+	jhs 1f
+
+	pass
+1:	fail
diff --git a/sim/testsuite/sim/msp430/allinsn.exp b/sim/testsuite/sim/msp430/allinsn.exp
new file mode 100644
index 0000000..affa8ae
--- /dev/null
+++ b/sim/testsuite/sim/msp430/allinsn.exp
@@ -0,0 +1,15 @@
+# msp430 simulator testsuite
+
+if [istarget msp430-*] {
+    # all machines
+    set all_machs "msp430"
+
+    foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.s]] {
+	# If we're only testing specific files and this isn't one of them,
+	# skip it.
+	if ![runtest_file_p $runtests $src] {
+	    continue
+	}
+	run_sim_test $src $all_machs
+    }
+}
diff --git a/sim/testsuite/sim/msp430/testutils.inc b/sim/testsuite/sim/msp430/testutils.inc
new file mode 100644
index 0000000..6c540b1
--- /dev/null
+++ b/sim/testsuite/sim/msp430/testutils.inc
@@ -0,0 +1,70 @@
+# MACRO: start
+# All assembler tests should start with a call to "start"
+	.macro start
+	.text
+
+	# Skip over these inlined funcs.
+	jmp __start;
+
+	.global __pass
+	.type __pass, function
+__pass:
+	write 1, _passmsg, 5
+	exit 0
+
+	.global __fail
+	.type __fail, function
+__fail:
+	write 1, _failmsg, 5
+	exit 1
+
+	.data
+_passmsg:
+	.ascii "pass\n"
+	.align 4
+
+_failmsg:
+	.ascii "fail\n"
+	.align 4
+
+	.text
+	.global __start
+	.type __start, function
+__start:
+	.endm
+
+# MACRO: system_call
+# Make a libgloss/Linux system call
+	.macro system_call nr:req
+	call #(0x180|\nr);
+	.endm
+
+# MACRO: exit
+# Quit the current test
+	.macro exit rc:req
+	mov #\rc, r12
+	system_call 1
+	.endm
+
+# MACRO: pass
+# Write 'pass' to stdout via syscalls and quit;
+# meant for non-OS operating environments
+	.macro pass
+	jmp __pass;
+	.endm
+
+# MACRO: fail
+# Write 'fail' to stdout via syscalls and quit;
+# meant for non-OS operating environments
+	.macro fail
+	jmp __fail;
+	.endm
+
+# MACRO: write
+# Just like the write() C function; uses system calls
+	.macro write fd:req, buf:req, count:req
+	mov #\fd, r12;
+	mov #\buf, r13;
+	mov #\count, r14;
+	system_call 5
+	.endm
-- 
1.9.0

[PATCH 2/3] sim: msp430: set initial PC to ELF entry if available

[Mike Frysinger]

If we want to run a simple ELF, it makes more sense to start at the
ELF's entry point instead of requiring the magic reset vector always
be set up.  This matches better what other sims do.

2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* msp430-sim.c (sim_create_inferior): Set new_pc to the result of
	bfd_get_start_address when abfd is not NULL.  Else, set new_pc to
	the existing reset vector.
---
 sim/msp430/msp430-sim.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
index c3fa4a9..3017080 100644
--- a/sim/msp430/msp430-sim.c
+++ b/sim/msp430/msp430-sim.c
@@ -249,9 +249,18 @@ sim_create_inferior (SIM_DESC sd,
   int c;
   int new_pc;
 
-  c = sim_core_read_buffer (sd, MSP430_CPU (sd), read_map, resetv, 0xfffe, 2);
+  /* Set the PC.  */
+  if (abfd != NULL)
+    {
+      new_pc = bfd_get_start_address (abfd);
+    }
+  else
+    {
+      c = sim_core_read_buffer (sd, MSP430_CPU (sd), read_map, resetv, 0xfffe,
+				2);
+      new_pc = resetv[0] + 256 * resetv[1];
+    }
 
-  new_pc = resetv[0] + 256 * resetv[1];
   sim_pc_set (MSP430_CPU (sd), new_pc);
   msp430_pc_store (MSP430_CPU (sd), new_pc);
 
-- 
1.9.0

[PATCH 1/3] sim: msp430: fix build time warnings

[Mike Frysinger]

This fix is simple:

msp430-sim.c: In function 'maybe_perform_syscall':
msp430-sim.c:898:10: warning: format '%d' expects argument of type 'int',
                     but argument 5 has type 'long int' [-Wformat]

This one we change to use casts like everyone else does in the code base:

msp430-sim.c: In function 'msp430_step_once':
msp430-sim.c:985:7: warning: passing argument 3 of 'init_disassemble_info'
                    from incompatible pointer type [enabled by default]
include/dis-asm.h:368:13: note: expected 'fprintf_ftype' but argument is
of type 'int (*)(struct FILE * __restrict__,  const char * __restrict__)'

2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* msp430-sim (maybe_perform_syscall): Change %d to %ld.
	(msp430_step_once): Cast fprintf to fprintf_ftype.
---
 sim/msp430/msp430-sim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
index 8cd2e1e..c3fa4a9 100644
--- a/sim/msp430/msp430-sim.c
+++ b/sim/msp430/msp430-sim.c
@@ -895,7 +895,7 @@ maybe_perform_syscall (SIM_DESC sd, int call_addr)
 
       if (TRACE_SYSCALL_P (MSP430_CPU (sd)))
 	trace_generic (sd, MSP430_CPU (sd), TRACE_SYSCALL_IDX,
-		       "returns %d", sc.result);
+		       "returns %ld", sc.result);
 
       MSP430_CPU (sd)->state.regs[12] = sc.result;
       return 1;
@@ -982,7 +982,7 @@ msp430_step_once (SIM_DESC sd)
 
       sim_core_read_buffer (sd, MSP430_CPU (sd), 0, b, opcode_pc, opsize);
 
-      init_disassemble_info (&info, stderr, fprintf);
+      init_disassemble_info (&info, stderr, (fprintf_ftype) fprintf);
       info.private_data = sd;
       info.read_memory_func = msp430_dis_read;
       fprintf (stderr, "%#8x  ", opcode_pc);
-- 
1.9.0

Re: [PATCH 1/3] sim: msp430: fix build time warnings

[Nicholas Clifton]

Hi Mike,

> 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
>
> 	* msp430-sim (maybe_perform_syscall): Change %d to %ld.
> 	(msp430_step_once): Cast fprintf to fprintf_ftype.

This is fine by me, although you do not need my approval to commit it. 
Thanks for catching the problem though.

Cheers
   Nick

Re: [PATCH 2/3] sim: msp430: set initial PC to ELF entry if available

[Nicholas Clifton]

Hi Mike,

> 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
>
> 	* msp430-sim.c (sim_create_inferior): Set new_pc to the result of
> 	bfd_get_start_address when abfd is not NULL.  Else, set new_pc to
> 	the existing reset vector.

Looks good to me.

Cheers
   Nick

Re: [PATCH 3/3] sim: msp430: start a test framework

[Nicholas Clifton]

Hi  Mike,

> The current sim lacks any sort of tests.  Start a basic framework and
> add a simple one to test the add insn.
>
> sim/:
> 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
>
> 	* configure.tgt (msp430*-*-*): Set sim_testsuite to yes.
> 	* configure: Regenerate.
>
> sim/testsuite/:
> 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
>
> 	* configure: Regenerate.
>
> sim/testsuite/sim/msp430/:
> 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
>
> 	* add.s, allinsn.exp, testutils.inc: New files.

Most welcome, thanks for doing this.

Cheers
   Nick

Re: [PATCH 1/3] sim: msp430: fix build time warnings

[Mike Frysinger]

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

On Mon 10 Mar 2014 13:18:43 Nicholas Clifton wrote:
> > 2014-03-08  Mike Frysinger  <vapier@gentoo.org>
> > 
> > 	* msp430-sim (maybe_perform_syscall): Change %d to %ld.
> > 	(msp430_step_once): Cast fprintf to fprintf_ftype.
> 
> This is fine by me, although you do not need my approval to commit it.
> Thanks for catching the problem though.

you authored the sim so i cc-ed you :).  i guess you don't want to be listed 
as the MAINTAINER ?
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] sim: msp430: set initial PC to ELF entry if available

[DJ Delorie]

In the case where there *is* a valid reset vector, I think it would be
much better to honor it than to choose some other "arbitrary" (in
terms of hardware simulation) entry point.

Just because the other simulators don't accurately simulate the
hardware, doesn't mean they all shouldn't.

Also, the libgloss for msp430 always sets a valid reset vector, so all
the "simple ELFs" you create should already work.

Re: [PATCH 2/3] sim: msp430: set initial PC to ELF entry if available

[Mike Frysinger]

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

On Mon 10 Mar 2014 17:09:25 DJ Delorie wrote:
> In the case where there *is* a valid reset vector, I think it would be
> much better to honor it than to choose some other "arbitrary" (in
> terms of hardware simulation) entry point.

i'm fine with making the default virtual environment fallback to the entry 
point when the reset vector is 0.

> Just because the other simulators don't accurately simulate the
> hardware, doesn't mean they all shouldn't.

we're talking about the virtual environment here, not the operating 
environment.  seems a bit funny belittling other ports when the msp430 sim 
itself doesn't properly support simulating the hardware ;).  the few random 
hacks it has in place to try and emulate a few choice aspects of the hardware 
doesn't mean it accurately simulates things either.

> Also, the libgloss for msp430 always sets a valid reset vector, so all
> the "simple ELFs" you create should already work.

newlib/libgloss requires a compiler which isn't appropriate from most sim 
tests.  using C for higher level things is perfectly reasonable (especially 
when validating hardware peripheral simulation), but there should be pure 
assembly tests that focus on the ISA.

it's also how i tend to personally test things when it comes to the sim & low 
level hardware -- i use gas+ld only.  a C runtime is a luxury and just gets in 
the way :).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2] sim: msp430: set initial PC to ELF entry if available

[Mike Frysinger]

If we want to run a simple ELF, it makes more sense to start at the
ELF's entry point instead of requiring the magic reset vector always
be set up.  This matches better what other sims do.

2014-03-08  Mike Frysinger  <vapier@gentoo.org>

	* msp430-sim.c (sim_create_inferior): Set new_pc to the result of
	bfd_get_start_address when abfd is not NULL and new_pc is zero.
---
v2
	- use the ELF entry only when the reset vector is not set up.

 sim/msp430/msp430-sim.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sim/msp430/msp430-sim.c b/sim/msp430/msp430-sim.c
index c3fa4a9..2a648f5 100644
--- a/sim/msp430/msp430-sim.c
+++ b/sim/msp430/msp430-sim.c
@@ -249,9 +249,14 @@ sim_create_inferior (SIM_DESC sd,
   int c;
   int new_pc;
 
+  /* Set the PC to the default reset vector if available.  */
   c = sim_core_read_buffer (sd, MSP430_CPU (sd), read_map, resetv, 0xfffe, 2);
-
   new_pc = resetv[0] + 256 * resetv[1];
+
+  /* If the reset vector isn't initialized, then use the ELF entry.  */
+  if (abfd != NULL && !new_pc)
+    new_pc = bfd_get_start_address (abfd);
+
   sim_pc_set (MSP430_CPU (sd), new_pc);
   msp430_pc_store (MSP430_CPU (sd), new_pc);
 
-- 
1.9.0

Re: [PATCH v2] sim: msp430: set initial PC to ELF entry if available

[DJ Delorie]

Assuming that memory-region ram is initialized to zero (I think it is
based on reading the common/ code), this is fine.  Thanks!

Re: [PATCH v2] sim: msp430: set initial PC to ELF entry if available

[Mike Frysinger]

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

On Mon 10 Mar 2014 23:44:31 DJ Delorie wrote:
> Assuming that memory-region ram is initialized to zero (I think it is
> based on reading the common/ code), this is fine.  Thanks!

the default fill value is 0.  it can be changed by memory-fill, but no sim 
does that.  a user could change this with command line flags, but i guess 
you'd get what you pay for at that point :).
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] sim: msp430: set initial PC to ELF entry if available

[DJ Delorie]

> a user could change this with command line flags, but i guess you'd
> get what you pay for at that point :).

+1 on that :-)

Re: [PATCH 1/3] sim: msp430: fix build time warnings

[Nicholas Clifton]

Hi Mike,

> you authored the sim so i cc-ed you :).  i guess you don't want to be listed
> as the MAINTAINER ?

Actually I would be very happy to be listed as the maintainer.  If this 
is OK with you then I can update the MAINTAINERS file.

Cheers
   Nick

Re: [PATCH 1/3] sim: msp430: fix build time warnings

[Mike Frysinger]

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

On Tue 11 Mar 2014 16:07:58 Nicholas Clifton wrote:
> > you authored the sim so i cc-ed you :).  i guess you don't want to be
> > listed as the MAINTAINER ?
> 
> Actually I would be very happy to be listed as the maintainer.  If this
> is OK with you then I can update the MAINTAINERS file.

go for it
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]